cc: Cancel RenewTreePriorirty callback if LTHI is gone.

cc: Cancel RenewTreePriorirty callback if LTHI is gone.

This patch fixes a crash in which the smoothness notifier triggers
a RenewTreePriority call after LTHI disappears. In that case,
we cancel the callback when the LTHI is gone.

R=reveman, brianderson
BUG=411211
Committed: https://crrev.com/e2048063b52f0324ffe7aa6283926750c04ed293
Cr-Commit-Position: refs/heads/master@{#293835}

[vmpstr, 2014-09-08 16:04:57]

PTAL. Another way to fix this would be to use a weak ptr in the notifier, but I
feel like this is a more foolproof solution. Weak ptr solution would also need
us to use the factory that is created in the same constructor, so it would
depend on the order of the variables in the class, which is somewhat fragile.

[danakj, 2014-09-08 16:32:08]

On 2014/09/08 16:04:57, vmpstr wrote:
> PTAL. Another way to fix this would be to use a weak ptr in the notifier, but
I
> feel like this is a more foolproof solution. Weak ptr solution would also need
> us to use the factory that is created in the same constructor, so it would
> depend on the order of the variables in the class, which is somewhat fragile.

Can we cancel the callback when things shutdown instead?

[reveman, 2014-09-08 16:36:52]

On 2014/09/08 16:04:57, vmpstr wrote:
> PTAL. Another way to fix this would be to use a weak ptr in the notifier, but
I
> feel like this is a more foolproof solution. Weak ptr solution would also need
> us to use the factory that is created in the same constructor, so it would
> depend on the order of the variables in the class, which is somewhat fragile.

Would it make sense to move smoothness_priority_expiration_notifier and
RenewTreePriority to LTHI? Looks like all the state used belongs there.

[danakj, 2014-09-08 16:38:52]

On Mon, Sep 8, 2014 at 12:36 PM, <reveman@chromium.org> wrote:

> On 2014/09/08 16:04:57, vmpstr wrote:
>
>> PTAL. Another way to fix this would be to use a weak ptr in the notifier,
>> but
>>
> I
>
>> feel like this is a more foolproof solution. Weak ptr solution would also
>> need
>> us to use the factory that is created in the same constructor, so it would
>> depend on the order of the variables in the class, which is somewhat
>> fragile.
>>
>
FWIW there was a push to ensure WeakPtrFactory is always the last thing in
the class always, because of exactly what you say above. I think depending
on that ordering is okay.

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

[vmpstr, 2014-09-08 17:23:18]

On 2014/09/08 16:38:52, danakj wrote:
> On Mon, Sep 8, 2014 at 12:36 PM, <mailto:reveman@chromium.org> wrote:
> 
> > On 2014/09/08 16:04:57, vmpstr wrote:
> >
> >> PTAL. Another way to fix this would be to use a weak ptr in the notifier,
> >> but
> >>
> > I
> >
> >> feel like this is a more foolproof solution. Weak ptr solution would also
> >> need
> >> us to use the factory that is created in the same constructor, so it would
> >> depend on the order of the variables in the class, which is somewhat
> >> fragile.
> >>
> >
> FWIW there was a push to ensure WeakPtrFactory is always the last thing in
> the class always, because of exactly what you say above. I think depending
> on that ordering is okay.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Yeah that's the reason weak ptr thing won't work: the factory is initialized
last, but the notifier needs a weak ptr before that.

[vmpstr, 2014-09-08 17:29:28]

On 2014/09/08 16:36:52, reveman wrote:
> On 2014/09/08 16:04:57, vmpstr wrote:
> > PTAL. Another way to fix this would be to use a weak ptr in the notifier,
but
> I
> > feel like this is a more foolproof solution. Weak ptr solution would also
need
> > us to use the factory that is created in the same constructor, so it would
> > depend on the order of the variables in the class, which is somewhat
fragile.
> 
> Would it make sense to move smoothness_priority_expiration_notifier and
> RenewTreePriority to LTHI? Looks like all the state used belongs there.

I'll file a cleanup bug about that. I'd like this patch to land as a crash fix
though.

[vmpstr, 2014-09-08 17:30:00]

PTAL. I forgot that we can just cancel the notifier. This should be a proper fix
(either that or weak ptr stuff)

[danakj, 2014-09-08 17:33:13]

LGTM

https://codereview.chromium.org/548423002/diff/20001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/548423002/diff/20001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1257:
impl().smoothness_priority_expiration_notifier.Cancel();
Leave a comment saying that this isn't using WeakPtr and why? (or todo if you
think it should be using that)

[vmpstr, 2014-09-08 17:42:36]

https://codereview.chromium.org/548423002/diff/20001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/548423002/diff/20001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1257:
impl().smoothness_priority_expiration_notifier.Cancel();
On 2014/09/08 17:33:13, danakj wrote:
> Leave a comment saying that this isn't using WeakPtr and why? (or todo if you
> think it should be using that)

Done. Also, I filed a bug with reveman@'s suggestion and mentioned it in the
comment as a (possibly better) alternative to fix this.

[danakj, 2014-09-08 17:50:48]

Thanks! LGTM

[brianderson, 2014-09-08 18:13:50]

lgtm

[reveman, 2014-09-08 19:27:43]

lgtm

[vmpstr, 2014-09-08 19:28:09]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-09-08 19:30:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmpstr@chromium.org/548423002/40001

[commit-bot: I haz the power, 2014-09-09 01:56:23]

Committed patchset #3 (id:40001) as 948f7b3f52e22c737907e7e9ca6d227128b688d8

[commit-bot: I haz the power, 2014-09-10 03:49:46]

Patchset 3 (id:??) landed as
https://crrev.com/e2048063b52f0324ffe7aa6283926750c04ed293
Cr-Commit-Position: refs/heads/master@{#293835}