Detach the RulesRegistryWithCache::weak_ptr_factory_ on destruction.

chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc

 // Copyright (c) 2012 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 "chrome/browser/extensions/api/declarative/rules_registry_with_cache.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
@@ skipping 62 matching lines @@
 
 // RulesRegistryWithCache
 
 RulesRegistryWithCache::RulesRegistryWithCache(
     Profile* profile,
     const char* event_name,
     content::BrowserThread::ID owner_thread,
     bool log_storage_init_delay,
     scoped_ptr<RuleStorageOnUI>* ui_part)
     : RulesRegistry(owner_thread, event_name),
-      weak_ptr_factory_((profile) ? this : NULL),
+      weak_ptr_factory_(this),
       storage_on_ui_((profile
                           ? (new RuleStorageOnUI(profile,
                                                  GetDeclarativeRuleStorageKey(
                                                      event_name,
                                                      profile->IsOffTheRecord()),
                                                  owner_thread,
                                                  weak_ptr_factory_.GetWeakPtr(),
                                                  log_storage_init_delay))
                                 ->GetWeakPtr()
                           : base::WeakPtr<RuleStorageOnUI>())) {
@@ skipping 116 matching lines @@
 
 void RulesRegistryWithCache::OnExtensionUnloaded(
     const std::string& extension_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
   std::string error = RemoveAllRules(extension_id);
   if (!error.empty())
     LOG(ERROR) << error;
 }
 
 RulesRegistryWithCache::~RulesRegistryWithCache() {
+  weak_ptr_factory_.DetachFromThread();

[Jeffrey Yasskin, 2013/05/28 22:37:10]

This isn't safe, according to the thread-safety rules at
https://code.google.com/p/chromium/codesearch/#chromium/src/base/memory/weak_....
weak_ptr_factory_.Invalidate() has to be called on the thread that uses
WeakPtr::get(), and that's the IO thread for the WebRequestRulesRegistry.

The underlying problem looks to be that RulesRegistry is refcounted, and so
occasionally the last reference will happen to be on the UI thread instead of
the rules_registry_thread? Now that RulesRegistryWithCache has a
weak_ptr_factory, it might be easy to un-ref-count RulesRegistry.

It would be good to know where this last reference is held. Could you add a
DCHECK(CurrentlyOn(owner_thread())) here and run your test again to get a stack
trace for the culprit?

[Jeffrey Yasskin, 2013/05/28 22:48:49]

One way to work around this would be to add a method that calls
weak_ptr_factory_.Invalidate(), post a call to that onto the owner thread from
whatever point on the UI thread intends to release the RulesRegistry.

 }
 
 void RulesRegistryWithCache::MarkReady(base::Time storage_init_time) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
   if (!storage_init_time.is_null()) {
     UMA_HISTOGRAM_TIMES("Extensions.DeclarativeRulesStorageInitialization",
                         base::Time::Now() - storage_init_time);
   }
 
@@ skipping 201 matching lines @@
 void RulesRegistryWithCache::RuleStorageOnUI::SetDeclarativeRulesStored(
     const std::string& extension_id,
     bool rules_stored) {
   CHECK(profile_);
   ExtensionScopedPrefs* extension_prefs = ExtensionPrefs::Get(profile_);
   extension_prefs->UpdateExtensionPref(
       extension_id, kRulesStoredKey, new base::FundamentalValue(rules_stored));
 }
 
 }  // namespace extensions