Skip to content

Commit

Permalink
Re-land architecture for cross-process memory notification suppressing
Browse files Browse the repository at this point in the history
This patch re-lands https://codereview.chromium.org/1332583002/ after
it was reverted in https://codereview.chromium.org/1359873002/ because
it was causing failures on the Cast Linux buildbot.

The failures were caused by std::set operations being carried out
inside DCHECK macros, which are not evaluated on release builds:

  DCHECK(memory_message_filters_.insert(filter).second);

This patch moves the std::set operations outside the macros so that
they would always be evaluated:

  const bool success = memory_message_filters_.insert(filter).second;
  DCHECK(success);

BUG=516776

Review URL: https://codereview.chromium.org/1361963002

Cr-Commit-Position: refs/heads/master@{#350503}
  • Loading branch information
petrcermak authored and Commit bot committed Sep 24, 2015
1 parent 2fbfee5 commit 29bd405
Show file tree
Hide file tree
Showing 17 changed files with 425 additions and 2 deletions.
2 changes: 2 additions & 0 deletions content/browser/browser_child_process_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "base/synchronization/waitable_event.h"
#include "content/browser/histogram_message_filter.h"
#include "content/browser/loader/resource_message_filter.h"
#include "content/browser/memory/memory_message_filter.h"
#include "content/browser/profiler_message_filter.h"
#include "content/browser/tracing/trace_message_filter.h"
#include "content/common/child_process_host_impl.h"
Expand Down Expand Up @@ -120,6 +121,7 @@ BrowserChildProcessHostImpl::BrowserChildProcessHostImpl(
AddFilter(new TraceMessageFilter(data_.id));
AddFilter(new ProfilerMessageFilter(process_type));
AddFilter(new HistogramMessageFilter);
AddFilter(new MemoryMessageFilter);

g_child_process_list.Get().push_back(this);
GetContentClient()->browser()->BrowserChildProcessHostCreated(this);
Expand Down
34 changes: 34 additions & 0 deletions content/browser/memory/memory_message_filter.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "content/browser/memory/memory_message_filter.h"

#include "content/browser/memory/memory_pressure_controller.h"
#include "content/common/memory_messages.h"

namespace content {

MemoryMessageFilter::MemoryMessageFilter()
: BrowserMessageFilter(MemoryMsgStart) {}

MemoryMessageFilter::~MemoryMessageFilter() {}

void MemoryMessageFilter::OnFilterAdded(IPC::Sender* sender) {
MemoryPressureController::GetInstance()->OnMemoryMessageFilterAdded(this);
}

void MemoryMessageFilter::OnChannelClosing() {
MemoryPressureController::GetInstance()->OnMemoryMessageFilterRemoved(this);
}

bool MemoryMessageFilter::OnMessageReceived(const IPC::Message& message) {
return false;
}

void MemoryMessageFilter::SendSetPressureNotificationsSuppressed(
bool suppressed) {
Send(new MemoryMsg_SetPressureNotificationsSuppressed(suppressed));
}

} // namespace content
35 changes: 35 additions & 0 deletions content/browser/memory/memory_message_filter.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CONTENT_BROWSER_MEMORY_MEMORY_MESSAGE_FILTER_H_
#define CONTENT_BROWSER_MEMORY_MEMORY_MESSAGE_FILTER_H_

#include "content/common/content_export.h"
#include "content/public/browser/browser_message_filter.h"

namespace content {

// This class sends memory messages from the browser process.
// See also: child_memory_message_filter.h
class CONTENT_EXPORT MemoryMessageFilter : public BrowserMessageFilter {
public:
MemoryMessageFilter();

// BrowserMessageFilter implementation.
void OnFilterAdded(IPC::Sender* sender) override;
void OnChannelClosing() override;
bool OnMessageReceived(const IPC::Message& message) override;

void SendSetPressureNotificationsSuppressed(bool suppressed);

protected:
~MemoryMessageFilter() override;

private:
DISALLOW_COPY_AND_ASSIGN(MemoryMessageFilter);
};

} // namespace content

#endif // CONTENT_BROWSER_MEMORY_MEMORY_MESSAGE_FILTER_H_
73 changes: 73 additions & 0 deletions content/browser/memory/memory_pressure_controller.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "content/browser/memory/memory_pressure_controller.h"

#include "base/bind.h"
#include "base/memory/memory_pressure_listener.h"
#include "content/browser/memory/memory_message_filter.h"
#include "content/public/browser/browser_thread.h"

namespace content {

MemoryPressureController::MemoryPressureController() {}

MemoryPressureController::~MemoryPressureController() {}

void MemoryPressureController::OnMemoryMessageFilterAdded(
MemoryMessageFilter* filter) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);

// Add the message filter to the set of all memory message filters and check
// that it wasn't there beforehand.
const bool success = memory_message_filters_.insert(filter).second;
DCHECK(success);

// There's no need to send a message to the child process if memory pressure
// notifications are not suppressed.
if (base::MemoryPressureListener::AreNotificationsSuppressed())
filter->SendSetPressureNotificationsSuppressed(true);
}

void MemoryPressureController::OnMemoryMessageFilterRemoved(
MemoryMessageFilter* filter) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);

// Remove the message filter from the set of all memory message filters and
// check that it was there beforehand.
const bool success = memory_message_filters_.erase(filter) == 1u;
DCHECK(success);
}

// static
MemoryPressureController* MemoryPressureController::GetInstance() {
return base::Singleton<
MemoryPressureController,
base::LeakySingletonTraits<MemoryPressureController>>::get();
}

void MemoryPressureController::SetPressureNotificationsSuppressedInAllProcesses(
bool suppressed) {
if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
// Note that passing base::Unretained(this) is safe here because the
// controller is a leaky singleton (i.e. it is never deleted).
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&MemoryPressureController::
SetPressureNotificationsSuppressedInAllProcesses,
base::Unretained(this), suppressed));
return;
}

// Enable/disable suppressing memory notifications in the browser process.
base::MemoryPressureListener::SetNotificationsSuppressed(suppressed);

// Enable/disable suppressing memory notifications in all child processes.
for (MemoryMessageFilterSet::iterator it = memory_message_filters_.begin();
it != memory_message_filters_.end(); ++it) {
it->get()->SendSetPressureNotificationsSuppressed(suppressed);
}
}

} // namespace content
48 changes: 48 additions & 0 deletions content/browser/memory/memory_pressure_controller.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CONTENT_BROWSER_MEMORY_MEMORY_PRESSURE_CONTROLLER_H_
#define CONTENT_BROWSER_MEMORY_MEMORY_PRESSURE_CONTROLLER_H_

#include <set>

#include "base/callback.h"
#include "base/memory/singleton.h"
#include "content/common/content_export.h"

namespace content {

class MemoryMessageFilter;

class CONTENT_EXPORT MemoryPressureController {
public:
// These methods must be called on the IO thread.
void OnMemoryMessageFilterAdded(MemoryMessageFilter* filter);
void OnMemoryMessageFilterRemoved(MemoryMessageFilter* filter);

// This method can be called from any thread.
void SetPressureNotificationsSuppressedInAllProcesses(bool suppressed);

// This method can be called from any thread.
static MemoryPressureController* GetInstance();

protected:
virtual ~MemoryPressureController();

private:
friend struct base::DefaultSingletonTraits<MemoryPressureController>;

MemoryPressureController();

// Set of all memory message filters in the browser process. Always accessed
// on the IO thread.
typedef std::set<scoped_refptr<MemoryMessageFilter>> MemoryMessageFilterSet;
MemoryMessageFilterSet memory_message_filters_;

DISALLOW_COPY_AND_ASSIGN(MemoryPressureController);
};

} // namespace content

#endif // CONTENT_BROWSER_MEMORY_MEMORY_PRESSURE_CONTROLLER_H_
131 changes: 131 additions & 0 deletions content/browser/memory/memory_pressure_controller_browsertest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "base/bind.h"
#include "base/memory/memory_pressure_listener.h"
#include "content/browser/memory/memory_message_filter.h"
#include "content/browser/memory/memory_pressure_controller.h"
#include "content/common/memory_messages.h"
#include "content/public/test/content_browser_test.h"
#include "content/public/test/content_browser_test_utils.h"
#include "content/public/test/test_utils.h"
#include "ipc/ipc_message.h"
#include "testing/gmock/include/gmock/gmock.h"

namespace content {

MATCHER_P(IsSetSuppressedMessage, suppressed, "") {
// Ensure that the message is deleted upon return.
scoped_ptr<IPC::Message> message(arg);
if (message == nullptr)
return false;
MemoryMsg_SetPressureNotificationsSuppressed::Param param;
if (!MemoryMsg_SetPressureNotificationsSuppressed::Read(message.get(),
&param))
return false;
return suppressed == base::get<0>(param);
}

class MemoryMessageFilterForTesting : public MemoryMessageFilter {
public:
MOCK_METHOD1(Send, bool(IPC::Message* message));

void Add() {
// The filter must be added on the IO thread.
if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
base::Bind(&MemoryMessageFilterForTesting::Add,
base::Unretained(this)));
RunAllPendingInMessageLoop(BrowserThread::IO);
return;
}
OnChannelConnected(0);
OnFilterAdded(nullptr);
}

void Remove() {
// The filter must be removed on the IO thread.
if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
base::Bind(&MemoryMessageFilterForTesting::Remove,
base::Unretained(this)));
RunAllPendingInMessageLoop(BrowserThread::IO);
return;
}
OnChannelClosing();
OnFilterRemoved();
}

protected:
~MemoryMessageFilterForTesting() override {}
};

class MemoryPressureControllerBrowserTest : public ContentBrowserTest {
protected:
void SetPressureNotificationsSuppressedInAllProcessesAndWait(
bool suppressed) {
MemoryPressureController::GetInstance()
->SetPressureNotificationsSuppressedInAllProcesses(suppressed);
RunAllPendingInMessageLoop(BrowserThread::IO);
}
};

IN_PROC_BROWSER_TEST_F(MemoryPressureControllerBrowserTest,
SetPressureNotificationsSuppressedInAllProcesses) {
scoped_refptr<MemoryMessageFilterForTesting> filter1(
new MemoryMessageFilterForTesting);
scoped_refptr<MemoryMessageFilterForTesting> filter2(
new MemoryMessageFilterForTesting);

NavigateToURL(shell(), GetTestUrl("", "title.html"));

// Add the first filter. No messages should be sent because notifications are
// not suppressed.
EXPECT_CALL(*filter1, Send(testing::_)).Times(0);
EXPECT_CALL(*filter2, Send(testing::_)).Times(0);
filter1->Add();
EXPECT_FALSE(base::MemoryPressureListener::AreNotificationsSuppressed());

// Enable suppressing memory pressure notifications in all processes. The
// first filter should send a message.
EXPECT_CALL(*filter1, Send(IsSetSuppressedMessage(true))).Times(1);
EXPECT_CALL(*filter2, Send(testing::_)).Times(0);
SetPressureNotificationsSuppressedInAllProcessesAndWait(true);
EXPECT_TRUE(base::MemoryPressureListener::AreNotificationsSuppressed());

// Add the second filter. It should send a message because notifications are
// suppressed.
EXPECT_CALL(*filter1, Send(testing::_)).Times(0);
EXPECT_CALL(*filter2, Send(IsSetSuppressedMessage(true))).Times(1);
filter2->Add();

// Disable suppressing memory pressure notifications in all processes. Both
// filters should send a message.
EXPECT_CALL(*filter1, Send(IsSetSuppressedMessage(false))).Times(1);
EXPECT_CALL(*filter2, Send(IsSetSuppressedMessage(false))).Times(1);
SetPressureNotificationsSuppressedInAllProcessesAndWait(false);
EXPECT_FALSE(base::MemoryPressureListener::AreNotificationsSuppressed());

// Remove the first filter. No messages should be sent.
EXPECT_CALL(*filter1, Send(testing::_)).Times(0);
EXPECT_CALL(*filter2, Send(testing::_)).Times(0);
filter1->Remove();

// Enable suppressing memory pressure notifications in all processes. The
// second filter should send a message.
EXPECT_CALL(*filter1, Send(testing::_)).Times(0);
EXPECT_CALL(*filter2, Send(IsSetSuppressedMessage(true))).Times(1);
SetPressureNotificationsSuppressedInAllProcessesAndWait(true);
EXPECT_TRUE(base::MemoryPressureListener::AreNotificationsSuppressed());

// Remove the second filter and disable suppressing memory pressure
// notifications in all processes. No messages should be sent.
EXPECT_CALL(*filter1, Send(testing::_)).Times(0);
EXPECT_CALL(*filter2, Send(testing::_)).Times(0);
filter2->Remove();
SetPressureNotificationsSuppressedInAllProcessesAndWait(false);
EXPECT_FALSE(base::MemoryPressureListener::AreNotificationsSuppressed());
}

} // namespace content
2 changes: 2 additions & 0 deletions content/browser/renderer_host/render_process_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
#include "content/browser/media/capture/audio_mirroring_manager.h"
#include "content/browser/media/media_internals.h"
#include "content/browser/media/midi_host.h"
#include "content/browser/memory/memory_message_filter.h"
#include "content/browser/message_port_message_filter.h"
#include "content/browser/mime_registry_message_filter.h"
#include "content/browser/mojo/mojo_application_host.h"
Expand Down Expand Up @@ -960,6 +961,7 @@ void RenderProcessHostImpl::CreateMessageFilters() {
if (browser_command_line.HasSwitch(switches::kEnableMemoryBenchmarking))
AddFilter(new MemoryBenchmarkMessageFilter());
#endif
AddFilter(new MemoryMessageFilter());
AddFilter(new PushMessagingMessageFilter(
GetID(), storage_partition_impl_->GetServiceWorkerContext()));
#if defined(OS_ANDROID)
Expand Down
6 changes: 4 additions & 2 deletions content/child/child_thread_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "content/child/fileapi/file_system_dispatcher.h"
#include "content/child/fileapi/webfilesystem_impl.h"
#include "content/child/geofencing/geofencing_message_filter.h"
#include "content/child/memory/child_memory_message_filter.h"
#include "content/child/mojo/mojo_application.h"
#include "content/child/notifications/notification_dispatcher.h"
#include "content/child/power_monitor_broadcast_source.h"
Expand Down Expand Up @@ -434,10 +435,11 @@ void ChildThreadImpl::Init(const Options& options) {
channel_->AddFilter(geofencing_message_filter_->GetFilter());

if (!IsInBrowserProcess()) {
// In single process mode, browser-side tracing will cover the whole
// process including renderers.
// In single process mode, browser-side tracing and memory will cover the
// whole process including renderers.
channel_->AddFilter(new tracing::ChildTraceMessageFilter(
ChildProcess::current()->io_task_runner()));
channel_->AddFilter(new ChildMemoryMessageFilter());
}

// In single process mode we may already have a power monitor
Expand Down
Loading

0 comments on commit 29bd405

Please sign in to comment.