Detach the RulesRegistryWithCache::weak_ptr_factory_ on destruction.

Detach the RulesRegistryWithCache::weak_ptr_factory_ on destruction.

In tests I observed that RulesRegistryWithCache can be destructed on the UI thread. The destructor of the registry does not check this, but if the registry init
ialised the WeakPtrFactory for itself, that factory expects to be destructed on
the registry's thread. In case of WebRequestRulesRegistry (a subclass of RulesRegistryWithCache), this thread is IO, as opposed to the UI thread where the Rules
RegistryService lives.

Here I propose that the WeakPtrFactory should not care where it is destructed:
 - The only piece of code from where the weak pointer to the registry is used is the UI part (running entirely on a UI thread).
 - The UI part is owned by the RulesRegistryService (ui_parts_of_registries_).
 - The registry is referenced by RulesRegistryService, i.e., it lives at least until the service's (and UI parts') destruction.

Based on the above, I don't think there is a moment when the registry would be destructed, and still there would be a pending task holding a weak pointer to it. Such task would need to be waiting on the UI thread, and coming from the UI part, but the UI part would get destructed (on UI) before that task could be scheduled. Destructing the UI part would invalidate the task (which would contain a weak pointer to the UI part as |this|).

BUG=218451
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203710

[vabr (Chromium), 2013-05-28 16:04:05]

Hi Jeffrey,

This is a follow-up to my recent refactoring under your supervision.

Currently, a DCHECK about a WeakPtr being on a wrong thread fires at me when I:
1. Apply the diff from https://codereview.chromium.org/15911005/#ps22001
2. GYP_DEFINES+=" chromeos=1" build/gyp_chromium && ninja -C out/Debug
browser_tests
3. ./browser_tests --gtest_filter='TrayAccessibilityTest.ShowMenu'
--gtest_repeat=15

(The repetition is needed, because this failure is flaky.)

I propose a relaxation of this particular check, please see this CL and the
reasoning from its description. I am grateful for any feedback you can give,
whether it is refusing this altogether, or pointing at good places where I
should add comments, or anything between.

Thanks,
Vaclav

[vabr (Chromium), 2013-05-28 16:39:52]

Patch set 2: In response to the failing tests -- I can only detach a
non-NULL-initialised WeakPtrFactory, and there is no way to tell what is |ptr_|
of the factory. To make things simpler, I now initialise it always with this.

Given the impossibility to learn whether |ptr_| is NULL, I guess that is also
the intended use.

[vabr (Chromium), 2013-05-28 16:47:23]

Another fix: jeffreyc->jyasskin for reviewer! :)

Sorry guys for that autocomplete-typo.

Vaclav

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

https://codereview.chromium.org/15688010/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
(right):

https://codereview.chromium.org/15688010/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc:220:
weak_ptr_factory_.DetachFromThread();
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]

https://codereview.chromium.org/15688010/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
(right):

https://codereview.chromium.org/15688010/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc:220:
weak_ptr_factory_.DetachFromThread();
On 2013/05/28 22:37:10, Jeffrey Yasskin wrote:
> 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?

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.

[vabr (Chromium), 2013-05-29 14:22:31]

Jeffrey, thanks for your answer, this is the direction to what I needed to be
sent!

My analysis:
All instances of RulesRegistryWithCache are held in a scoped_refptr by
RulesRegistryService.
In addition, those instances which are WebRequestRulesRegistries (WRRR :)), are
also held by ExtensionWebRequestEventRouter in scoped_refptr.

The RulesRegistryService itself is owned by ExtensionSystemImpl. Once
ExtensionSystemImpl gets destroyed, it explicitly calls
RulesRegistryService::Shutdown, and then destroys the RulesRegistryService.

During Shutdown, the RulesRegistryService posts a task to
ExtensionWebRequestEventRouter to release the reference to the WRRR. This is a
race with releasing the other reference held by RulesRegistryService itself. The
router releases it on IO thread, the service on UI thread. So the only good
result of this race is when the service releases first, and then the router
comes second and destructs the WRRR on IO.


Long-term solution:
As you suggest, getting rid of refcounting for objects which are RulesRegistry
is a good solution.


Short-term solution:
See patch set 3 here. I explicitly decide the race in RulesRegistryService's
favour. I believe this is consistent with the comment at the declaration of
Shutdown, which says: "Unregisters refptrs to concrete RulesRegistries at other
objects that were created by us so that the RulesRegistries can be released."


I propose to implement the short-term solution now, it looks harmless and will
unblock my work on https://codereview.chromium.org/15911005. As soon as time
permits, I will also implement the long-term solution. This will be tracked in
the re-opened http://crbug.com/218451 (or in a new bug if you prefer). While I
do not expect major delays or issues with the long-term solution, one never
knows, hence the 2-step approach.

Does that sound good to you?


P.S. (Just trivia:) Chromium moves very fast again. Just yesterday the
DetachFromThread got removed altogether
(https://chromiumcodereview.appspot.com/16007009), and the failing unit test got
re-named (https://chromiumcodereview.appspot.com/15086005). :)

[Jeffrey Yasskin, 2013-05-29 17:06:33]

I want Dominic to double-check that releasing the RulesRegistries earlier is ok,
but this short-term fix looks ok to me.

[vabr (Chromium), 2013-05-31 07:09:35]

On 2013/05/29 17:06:33, Jeffrey Yasskin wrote:
> I want Dominic to double-check that releasing the RulesRegistries earlier is
ok,
> but this short-term fix looks ok to me.

Thanks, Jeffrey.
Dominic, if you want to talk about this, feel free to grab me any time.
Vaclav

[battre, 2013-06-03 10:22:14]

https://codereview.chromium.org/15688010/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/15688010/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:77:
profile_, scoped_refptr<WebRequestRulesRegistry>(NULL)));
Wouldn't the correct way of solving this be:

void HoldReferenceUntilCalled(scoped_refptr<WebRequestRulesRegistry>
rules_registry) {
  // Do nothing. The purpose of this function is just to hold the ref-counted
reference until
  // it is called. This allows us to destruct the object on the right thread.
}

void RulesRegistryService::Shutdown() {
  RulesRegistryMap temp_rule_registries;
  std::swap(&temp_rule_registries, &rule_registries_);
  content::BrowserThread::PostTask(
      content::BrowserThread::IO, FROM_HERE,
      base::Bind(&RegisterToExtensionWebRequestEventRouterOnIO,
      profile_, scoped_refptr<WebRequestRulesRegistry>(NULL)));
  for (rule registry : temp_rule_registries) {
     post task HoldReferenceUntilCalled on owner thread of rule registry
  }
}

[vabr (Chromium), 2013-06-03 10:45:45]

https://codereview.chromium.org/15688010/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/15688010/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:77:
profile_, scoped_refptr<WebRequestRulesRegistry>(NULL)));
On 2013/06/03 10:22:15, battre wrote:
> Wouldn't the correct way of solving this be:
> 
> void HoldReferenceUntilCalled(scoped_refptr<WebRequestRulesRegistry>
> rules_registry) {
>   // Do nothing. The purpose of this function is just to hold the ref-counted
> reference until
>   // it is called. This allows us to destruct the object on the right thread.
> }
> 
> void RulesRegistryService::Shutdown() {
>   RulesRegistryMap temp_rule_registries;
>   std::swap(&temp_rule_registries, &rule_registries_);
>   content::BrowserThread::PostTask(
>       content::BrowserThread::IO, FROM_HERE,
>       base::Bind(&RegisterToExtensionWebRequestEventRouterOnIO,
>       profile_, scoped_refptr<WebRequestRulesRegistry>(NULL)));
>   for (rule registry : temp_rule_registries) {
>      post task HoldReferenceUntilCalled on owner thread of rule registry
>   }
> }

Could you please be more specific about why you prefer the suggested solution?
As far as I understood, the RegisterToExtensionWebRequestEventRouterOnIO is
already there to make sure WebRequestRulesRegistry objects are deleted on the
appropriate thread, only there was a bug in how it was used, which is what I'm
hoping to fix with the explicit clear() on |rules_registries_|.

Your solution introduces a couple of posted tasks, which means slowing down the
shutdown unnecessarily. It does not give any more guarantee that the
WebRequestRulesRegistry objects are deleted on the right thread, than the
solution from this patch set -- both depend on the fact that the registries are
not referenced anywhere else than in RulesRegistryService or
ExtensionWebRequestEventRouter.

[battre, 2013-06-03 11:42:42]

LGTM

https://codereview.chromium.org/15688010/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/15688010/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:77:
profile_, scoped_refptr<WebRequestRulesRegistry>(NULL)));
On 2013/06/03 10:45:45, vabr (Chromium) wrote:
> On 2013/06/03 10:22:15, battre wrote:
> > Wouldn't the correct way of solving this be:
> > 
> > void HoldReferenceUntilCalled(scoped_refptr<WebRequestRulesRegistry>
> > rules_registry) {
> >   // Do nothing. The purpose of this function is just to hold the
ref-counted
> > reference until
> >   // it is called. This allows us to destruct the object on the right
thread.
> > }
> > 
> > void RulesRegistryService::Shutdown() {
> >   RulesRegistryMap temp_rule_registries;
> >   std::swap(&temp_rule_registries, &rule_registries_);
> >   content::BrowserThread::PostTask(
> >       content::BrowserThread::IO, FROM_HERE,
> >       base::Bind(&RegisterToExtensionWebRequestEventRouterOnIO,
> >       profile_, scoped_refptr<WebRequestRulesRegistry>(NULL)));
> >   for (rule registry : temp_rule_registries) {
> >      post task HoldReferenceUntilCalled on owner thread of rule registry
> >   }
> > }
> 
> Could you please be more specific about why you prefer the suggested solution?
> As far as I understood, the RegisterToExtensionWebRequestEventRouterOnIO is
> already there to make sure WebRequestRulesRegistry objects are deleted on the
> appropriate thread, only there was a bug in how it was used, which is what I'm
> hoping to fix with the explicit clear() on |rules_registries_|.
> 
> Your solution introduces a couple of posted tasks, which means slowing down
the
> shutdown unnecessarily. It does not give any more guarantee that the
> WebRequestRulesRegistry objects are deleted on the right thread, than the
> solution from this patch set -- both depend on the fact that the registries
are
> not referenced anywhere else than in RulesRegistryService or
> ExtensionWebRequestEventRouter.

On second thought, your solution is equally good.

[commit-bot: I haz the power, 2013-06-03 11:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/15688010/24001

[commit-bot: I haz the power, 2013-06-03 14:05:24]

Change committed as 203710

[Jeffrey Yasskin, 2013-06-03 16:12:16]

https://codereview.chromium.org/15688010/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/15688010/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:77:
profile_, scoped_refptr<WebRequestRulesRegistry>(NULL)));
On 2013/06/03 10:22:15, battre wrote:
> Wouldn't the correct way of solving this be:
> 
> void HoldReferenceUntilCalled(scoped_refptr<WebRequestRulesRegistry>
> rules_registry) {
>   // Do nothing. The purpose of this function is just to hold the ref-counted
> reference until
>   // it is called. This allows us to destruct the object on the right thread.
> }
> 
> void RulesRegistryService::Shutdown() {
>   RulesRegistryMap temp_rule_registries;
>   std::swap(&temp_rule_registries, &rule_registries_);
>   content::BrowserThread::PostTask(
>       content::BrowserThread::IO, FROM_HERE,
>       base::Bind(&RegisterToExtensionWebRequestEventRouterOnIO,
>       profile_, scoped_refptr<WebRequestRulesRegistry>(NULL)));
>   for (rule registry : temp_rule_registries) {
>      post task HoldReferenceUntilCalled on owner thread of rule registry
>   }
> }

The above code is subtly incorrect because it's possible for any of those posted
tasks to be executed before Shutdown() finishes, or even before PostTask
returns, in which case the reference would be destroyed on the current thread
anyway.

To make this correct, if we had move semantics for Bind(), we could ensure the
reference was transferred over to the other thread instead of copied (we can
even do enough of this in C++98), or we could manually call AddRef in this
function and Release on the raw pointer in HoldReferenceUntilCalled. But I think
it's cleaner to move toward singly-owned RulesRegistries anyway instead of
refcounting them.