Don't post tasks from RulesRegistry c-tor

Don't post tasks from RulesRegistry c-tor

Because RR is refcounted, it is a bad idea to put |this| into temporary scoped_refptr during construction.

The first patchset here only contains a whitespace change to make sure the use-after-free is reproducible on the trybots.

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

[vabr (Chromium), 2013-12-12 16:23:27]

Hi Jeffrey,

Sorry to flood you with reviews, but you are really the expert here. :)
(Feel free to decline, though.)

I also added Fady in Cc because I'm changing code paths related to usage of the
RulesRegistry in WebView.

The use-after-free seems flaky, so I have no hard evidence of my hypothesis
(https://code.google.com/p/chromium/issues/detail?id=328051#c1), but I'm pretty
confident in that one.

Thanks,
Vaclav

[Fady Samuel, 2013-12-12 16:37:29]

IIRC this won't work. If there is no RulesCacheDelegate, and MarkReady is never
called then the WebRequest API will not work.

[Fady Samuel, 2013-12-12 16:39:42]

See here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[vabr (Chromium), 2013-12-12 17:02:27]

On 2013/12/12 16:39:42, Fady Samuel wrote:
> See here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Fady is right.

In the worst case, I will leave the post task in the constructor, and swap
|this| for the weak pointer (the WeakPtrFactory is already initialised at that
point).

I'll update this soon, until then, no need to review yet.

Cheers,
Vaclav

[vabr (Chromium), 2013-12-12 17:34:51]

All right, Jeffrey and Fady,
Please take a look again.

I tried once more without the weak pointer, because it would be awkward (would
have to check out for the profile_ being NULL), and doing this by allowing
ready() to return NULL made more sense to me.

Looking forward to you comments!

Thanks,
Vaclav

[Fady Samuel, 2013-12-12 17:54:39]

https://codereview.chromium.org/108193008/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/108193008/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.h:71: return
(cache_delegate_) ? &ready_ : NULL;
What about making ready_ a scoped_ptr<OneShotEvent>? That way we don't actually
keep this object around in RulesRegistry if we don't need it.

[vabr (Chromium), 2013-12-13 16:03:33]

Hi Fady, Jeffrey.

I addressed Fady's comment, and fixed a NULL de-reference I introduced.

PTAL.

Thanks,
Vaclav

https://codereview.chromium.org/108193008/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/108193008/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.h:71: return
(cache_delegate_) ? &ready_ : NULL;
On 2013/12/12 17:54:39, Fady Samuel wrote:
> What about making ready_ a scoped_ptr<OneShotEvent>? That way we don't
actually
> keep this object around in RulesRegistry if we don't need it.

Done.

[vabr (Chromium), 2013-12-13 16:46:37]

Quick update: added patch set 8 to fix dereferencing of cache_delegate_ on a
wrong thread. The non-NULL-ness of cache_delegate_ and ready_ should be
equivalent, and ready_ being not NULL is even more directly what we are
interested in in MaybeProcessChangedRules.

[Fady Samuel, 2013-12-16 16:54:33]

lgtm

[vabr (Chromium), 2013-12-17 10:30:47]

Thanks, Fady.

Jeffrey, do you have any comments?
If not, could you please give me an OWNERS approval?

Thanks,
Vaclav

[Jeffrey Yasskin, 2013-12-17 22:55:05]

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.cc (left):

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.cc:95:
base::Bind(&RulesRegistry::MarkReady, this, base::Time::Now()));
Would this CL be simpler if you just called ready_.Signal() right here, instead
of Posting a call to MarkReady()?
I'm not particularly happy about needing to check ready_ for NULL instead of
just posting tasks to it. Removing the double-check is why OneShotEvent accepts
Post()s even after it's signaled.

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.cc:93: // Note that if
|cache_delegate| is NULL, the Rulesregistry never signals its
capitalization: RulesRegistry

[vabr (Chromium), 2013-12-18 12:01:50]

Thanks, Jeffrey.

Please see my response to your comment about calling Signal() directly.

Cheers,
Vaclav

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.cc (left):

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.cc:95:
base::Bind(&RulesRegistry::MarkReady, this, base::Time::Now()));
On 2013/12/17 22:55:05, Jeffrey Yasskin wrote:
> Would this CL be simpler if you just called ready_.Signal() right here,
instead
> of Posting a call to MarkReady()?
> I'm not particularly happy about needing to check ready_ for NULL instead of
> just posting tasks to it. Removing the double-check is why OneShotEvent
accepts
> Post()s even after it's signaled.

I'm afraid calling ready_.Signal() here would lead to a DCHECK failing
CalledOnValidThread as soon as ready_.Post() is used. This is because the
constructor runs on UI thread, but the rest runs on owner_thread (IO for
WebRequestRulesRegistry).

Currently, OneShotEvent detaches from the thread it's constructed on just to
allow constructing it on another thread than it's used to.

What I can do is:
1) Allow to construct it as already signalled, or
2) provide a OneShotEvent method to detach on demand.

Does anything from the above sound good to you? Any other suggestions?

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.cc:93: // Note that if
|cache_delegate| is NULL, the Rulesregistry never signals its
On 2013/12/17 22:55:05, Jeffrey Yasskin wrote:
> capitalization: RulesRegistry

Done.

[Jeffrey Yasskin, 2013-12-18 16:39:27]

On Dec 18, 2013 4:01 AM, <vabr@chromium.org> wrote:
>
> Thanks, Jeffrey.
>
> Please see my response to your comment about calling Signal() directly.
>
> Cheers,
> Vaclav
>
>
>
>
https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
> File chrome/browser/extensions/api/declarative/rules_registry.cc (left):
>
>
https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
> chrome/browser/extensions/api/declarative/rules_registry.cc:95:
> base::Bind(&RulesRegistry::MarkReady, this, base::Time::Now()));
> On 2013/12/17 22:55:05, Jeffrey Yasskin wrote:
>>
>> Would this CL be simpler if you just called ready_.Signal() right
>
> here, instead
>>
>> of Posting a call to MarkReady()?
>> I'm not particularly happy about needing to check ready_ for NULL
>
> instead of
>>
>> just posting tasks to it. Removing the double-check is why
>
> OneShotEvent accepts
>>
>> Post()s even after it's signaled.
>
>
> I'm afraid calling ready_.Signal() here would lead to a DCHECK failing
> CalledOnValidThread as soon as ready_.Post() is used. This is because
> the constructor runs on UI thread, but the rest runs on owner_thread (IO
> for WebRequestRulesRegistry).
>
> Currently, OneShotEvent detaches from the thread it's constructed on
> just to allow constructing it on another thread than it's used to.
>
> What I can do is:
> 1) Allow to construct it as already signalled, or

> 2) provide a OneShotEvent method to detach on demand.

I think exposing Detach will work better for this case, since it won't
force you to keep it in a scoped_ptr.

> Does anything from the above sound good to you? Any other suggestions?
>
>
>
https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
> File chrome/browser/extensions/api/declarative/rules_registry.cc
> (right):
>
>
https://codereview.chromium.org/108193008/diff/220001/chrome/browser/extensio...
> chrome/browser/extensions/api/declarative/rules_registry.cc:93: // Note
> that if |cache_delegate| is NULL, the Rulesregistry never signals its
> On 2013/12/17 22:55:05, Jeffrey Yasskin wrote:
>>
>> capitalization: RulesRegistry
>
>
> Done.
>
>
> https://codereview.chromium.org/108193008/
>
> --
> You received this message because you are subscribed to the Google Groups
"Extensions reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to extensions-reviews+unsubscribe@chromium.org.
> To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/extensions-reviews/047d7b673...
.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vabr (Chromium), 2013-12-18 17:45:16]

Hi Jeffrey,

I know you suggested 2) (explicit detach method), but I'm not sure whether you
dismissed 1) as not getting rid of the pointer (which it does), or for some
other reason.

Currently I implemented 1) as a "weaker" change to the OneShotEvent. If you
don't like it, I'm of course happy to implement the explicit detach method
instead.

Looking forward to your comments,
Vaclav

[Jeffrey Yasskin, 2013-12-18 17:56:54]

lgtm

[Jeffrey Yasskin, 2013-12-18 18:19:17]

On Wed, Dec 18, 2013 at 9:45 AM,  <vabr@chromium.org> wrote:
> Hi Jeffrey,
>
> I know you suggested 2) (explicit detach method), but I'm not sure whether
> you
> dismissed 1) as not getting rid of the pointer (which it does), or for some
> other reason.

Yeah, I hadn't looked at the constructor in enough detail. I suspect
we'll eventually have to expose Detach anyway, but your change is
good. Thanks for pushing back.

> Currently I implemented 1) as a "weaker" change to the OneShotEvent. If you
> don't like it, I'm of course happy to implement the explicit detach method
> instead.
>
> Looking forward to your comments,
> Vaclav
>
> https://codereview.chromium.org/108193008/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vabr (Chromium), 2013-12-19 07:20:55]

On 2013/12/18 18:19:17, Jeffrey Yasskin wrote:
> On Wed, Dec 18, 2013 at 9:45 AM,  <mailto:vabr@chromium.org> wrote:
> > Hi Jeffrey,
> >
> > I know you suggested 2) (explicit detach method), but I'm not sure whether
> > you
> > dismissed 1) as not getting rid of the pointer (which it does), or for some
> > other reason.
> 
> Yeah, I hadn't looked at the constructor in enough detail. I suspect
> we'll eventually have to expose Detach anyway, but your change is
> good. Thanks for pushing back.
> 
> > Currently I implemented 1) as a "weaker" change to the OneShotEvent. If you
> > don't like it, I'm of course happy to implement the explicit detach method
> > instead.
> >
> > Looking forward to your comments,
> > Vaclav
> >
> > https://codereview.chromium.org/108193008/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks, Jeffrey, for your patient review! :)

Sending to the CQ now.

[commit-bot: I haz the power, 2013-12-19 07:21:14]

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

[commit-bot: I haz the power, 2013-12-19 09:46:03]

Change committed as 241831