Skip to content

GitLab

T
tangled
Commit 49e7741e authored by tapted@chromium.org's avatar tapted@chromium.org
Browse files
Options

Revert of Remove deprecated extension notifications from ProcessManager... 

Revert of Remove deprecated extension notifications from ProcessManager (https://codereview.chromium.org/397743005/)

Reason for revert:
Suspected for Invalid Write / Unadressable / Uninitialized errors in 
Linux Tests (valgrind)(3) via ProcessManagerTest.ProcessGrouping and others starting 
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28valgrind%29%283%29/builds/38965

Errors like

Invalid write of size 8
  ObserverListBase<autofill::PersonalDataManagerObserver>::AddObserver(autofill::PersonalDataManagerObserver*) (base/observer_list.h:162)
  autofill::PersonalDataManager::AddObserver(autofill::PersonalDataManagerObserver*) (components/autofill/core/browser/personal_data_manager.cc:225)
  ...
  extensions::ProcessManagerTest::ProcessManagerTest() (extensions/browser/process_manager_unittest.cc:39)

and a range of uninitialized reads/jumps.

Original issue's description:
> Remove deprecated extension notifications from ProcessManager and use ExtensionRegistry instead.
> 
> BUG=354046
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284366

TBR=rdevlin.cronin@chromium.org,limasdf@gmail.com
NOTREECHECKS=true
NOTRY=true
BUG=354046

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284387 0039d316-1c4b-4281-b951-d872f2087c98
parent 6e4ce64d
Hide whitespace changes
Inline Side-by-side
Showing with 53 additions and 49 deletions
extensions/browser/process_manager.cc
View file @ 49e7741e
......@@ -229,11 +229,14 @@ ProcessManager::ProcessManager(BrowserContext* context,
devtools_callback_(base::Bind(&ProcessManager::OnDevToolsStateChanged,
base::Unretained(this))),
last_background_close_sequence_id_(0),
weak_ptr_factory_(this),
extension_registry_observer_(this) {
extension_registry_observer_.Add(ExtensionRegistry::Get(original_context));
weak_ptr_factory_(this) {
registrar_.Add(this, chrome::NOTIFICATION_EXTENSIONS_READY,
content::Source<BrowserContext>(original_context));
registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
content::Source<BrowserContext>(original_context));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED,
content::Source<BrowserContext>(original_context));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
content::Source<BrowserContext>(context));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE,
......@@ -657,6 +660,33 @@ void ProcessManager::Observe(int type,
break;
}
case chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED: {
BrowserContext* context = content::Source<BrowserContext>(source).ptr();
ExtensionSystem* system = ExtensionSystem::Get(context);
if (system->ready().is_signaled()) {
// The extension system is ready, so create the background host.
const Extension* extension =
content::Details<const Extension>(details).ptr();
CreateBackgroundHostForExtensionLoad(this, extension);
}
break;
}
case chrome::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED: {
const Extension* extension =
content::Details<UnloadedExtensionInfo>(details)->extension;
for (ExtensionHostSet::iterator iter = background_hosts_.begin();
iter != background_hosts_.end(); ++iter) {
ExtensionHost* host = *iter;
if (host->extension_id() == extension->id()) {
CloseBackgroundHost(host);
break;
}
}
UnregisterExtension(extension->id());
break;
}
case chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED: {
ExtensionHost* host = content::Details<ExtensionHost>(details).ptr();
if (background_hosts_.erase(host)) {
......@@ -730,30 +760,6 @@ void ProcessManager::Observe(int type,
}
}
void ProcessManager::OnExtensionLoaded(content::BrowserContext* browser_context,
const Extension* extension) {
if (ExtensionSystem::Get(browser_context)->ready().is_signaled()) {
// The extension system is ready, so create the background host.
CreateBackgroundHostForExtensionLoad(this, extension);
}
}
void ProcessManager::OnExtensionUnloaded(
content::BrowserContext* browser_context,
const Extension* extension,
UnloadedExtensionInfo::Reason reason) {
for (ExtensionHostSet::iterator iter = background_hosts_.begin();
iter != background_hosts_.end();
++iter) {
ExtensionHost* host = *iter;
if (host->extension_id() == extension->id()) {
CloseBackgroundHost(host);
break;
}
}
UnregisterExtension(extension->id());
}
void ProcessManager::OnDevToolsStateChanged(
content::DevToolsAgentHost* agent_host,
bool attached) {
......
extensions/browser/process_manager.h
View file @ 49e7741e
......@@ -14,11 +14,9 @@
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/scoped_observer.h"
#include "base/time/time.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "extensions/browser/extension_registry_observer.h"
#include "extensions/common/view_type.h"
class GURL;
......@@ -35,14 +33,12 @@ namespace extensions {
class Extension;
class ExtensionHost;
class ExtensionRegistry;
class ProcessManagerObserver;
// Manages dynamic state of running Chromium extensions. There is one instance
// of this class per Profile. OTR Profiles have a separate instance that keeps
// track of split-mode extensions only.
class ProcessManager : public content::NotificationObserver,
public ExtensionRegistryObserver {
class ProcessManager : public content::NotificationObserver {
public:
typedef std::set<extensions::ExtensionHost*> ExtensionHostSet;
typedef ExtensionHostSet::const_iterator const_iterator;
......@@ -162,14 +158,6 @@ class ProcessManager : public content::NotificationObserver,
const content::NotificationSource& source,
const content::NotificationDetails& details) OVERRIDE;
// ExtensionRegistryObserver:
virtual void OnExtensionLoaded(content::BrowserContext* browser_context,
const Extension* extension) OVERRIDE;
virtual void OnExtensionUnloaded(
content::BrowserContext* browser_context,
const Extension* extension,
UnloadedExtensionInfo::Reason reason) OVERRIDE;
// Load all background pages once the profile data is ready and the pages
// should be loaded.
void CreateBackgroundHostsForProfileStartup();
......@@ -276,9 +264,6 @@ class ProcessManager : public content::NotificationObserver,
// Must be last member, see doc on WeakPtrFactory.
base::WeakPtrFactory<ProcessManager> weak_ptr_factory_;
ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
extension_registry_observer_;
DISALLOW_COPY_AND_ASSIGN(ProcessManager);
};
......
extensions/browser/process_manager_unittest.cc
View file @ 49e7741e
......@@ -8,19 +8,20 @@
#include "content/public/browser/content_browser_client.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/site_instance.h"
#include "extensions/browser/extensions_test_browser_context.h"
#include "content/public/test/test_browser_context.h"
#include "extensions/browser/test_extensions_browser_client.h"
#include "testing/gtest/include/gtest/gtest.h"
using content::BrowserContext;
using content::SiteInstance;
using content::TestBrowserContext;
namespace extensions {
namespace {
// An incognito version of a ExtensionsTestBrowserContext.
class TestBrowserContextIncognito : public ExtensionsTestBrowserContext {
// An incognito version of a TestBrowserContext.
class TestBrowserContextIncognito : public TestBrowserContext {
public:
TestBrowserContextIncognito() {}
virtual ~TestBrowserContextIncognito() {}
......@@ -58,7 +59,7 @@ class ProcessManagerTest : public testing::Test {
}
private:
ExtensionsTestBrowserContext original_context_;
TestBrowserContext original_context_;
TestBrowserContextIncognito incognito_context_;
TestExtensionsBrowserClient extensions_browser_client_;
......@@ -78,6 +79,12 @@ TEST_F(ProcessManagerTest, ExtensionNotificationRegistration) {
EXPECT_TRUE(IsRegistered(manager1.get(),
chrome::NOTIFICATION_EXTENSIONS_READY,
original_context()));
EXPECT_TRUE(IsRegistered(manager1.get(),
chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
original_context()));
EXPECT_TRUE(IsRegistered(manager1.get(),
chrome::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED,
original_context()));
EXPECT_TRUE(IsRegistered(manager1.get(),
chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
original_context()));
......@@ -89,6 +96,11 @@ TEST_F(ProcessManagerTest, ExtensionNotificationRegistration) {
EXPECT_EQ(incognito_context(), manager2->GetBrowserContext());
EXPECT_EQ(0u, manager2->background_hosts().size());
// Some notifications are observed for the original context.
EXPECT_TRUE(IsRegistered(manager2.get(),
chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
original_context()));
// Some notifications are observed for the incognito context.
EXPECT_TRUE(IsRegistered(manager2.get(),
chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
......@@ -123,8 +135,9 @@ TEST_F(ProcessManagerTest, ProcessGrouping) {
// SiteInstances.
scoped_ptr<ProcessManager> manager1(
ProcessManager::Create(original_context()));
ExtensionsTestBrowserContext another_context;
// NOTE: This context is not associated with the TestExtensionsBrowserClient.
// That's OK because we're not testing regular vs. incognito behavior.
TestBrowserContext another_context;
scoped_ptr<ProcessManager> manager2(ProcessManager::Create(&another_context));
// Extensions with common origins ("scheme://id/") should be grouped in the
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment