Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1006253002:
Oilpan: have LifetimeNotifier<T> track its observers weakly. (Closed)
|
Created:
5 years, 9 months ago by sof Modified:
5 years, 9 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, arv+blink, vivekg_samsung, eae+blinkwatch, vivekg, dglazkov+blink, blink-reviews-bindings_chromium.org, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: have LifetimeNotifier<T> track its observers weakly.
Simplify the unregistration of lifetime observers upon finalization by
having LifetimeNotifier<> keep a WeakMember<> reference to them. As a
result the observers will no longer have to explicitly unregister
themselves when going away. Observers can still explicitly unregister,
should that be needed for other reasons.
R=haraken
BUG=462949, 467502
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192011
Patch Set 1 #Patch Set 2 : non-oilpan compile fix #Patch Set 3 : Update MIDIAccessInitializer's prefinalizer unregistration step #Patch Set 4 : Fill in GC_PLUGIN_IGNORE() #
Total comments: 20
Patch Set 5 : rebased #
Messages
Total messages: 25 (6 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... File Source/core/dom/ActiveDOMObject.cpp (right): https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... Source/core/dom/ActiveDOMObject.cpp:55: #if !ENABLE(OILPAN) Is this assert worth carrying over to Oilpan? I'm thinking not.
haraken@chromium.org changed reviewers: + haraken@chromium.org
Thanks for working on this! https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... File Source/core/dom/ActiveDOMObject.cpp (right): https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) Is it safe to touch executionContext() here? https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... Source/core/dom/ActiveDOMObject.cpp:55: #if !ENABLE(OILPAN) On 2015/03/16 15:44:34, sof wrote: > Is this assert worth carrying over to Oilpan? I'm thinking not. Agreed. https://codereview.chromium.org/1006253002/diff/60001/Source/modules/webmidi/... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/1006253002/diff/60001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:52: LifecycleObserver::contextDestroyed(); Why didn't we have this code previously? Was this a bug? https://codereview.chromium.org/1006253002/diff/60001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:73: ScriptPromiseResolver::dispose(); Why didn't we have this code previously? Was this a bug? https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... File Source/platform/LifecycleNotifier.h (right): https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:37: class LifecycleNotifier { Just help me understand: Why is it safe to use WillBeHeapHashSet without making LifecycleNotifier a GCMixin? https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:80: // bases, but cannot itself derive from a GC base class also. Regarding the diamond inheritance problem, can we use the approach in https://codereview.chromium.org/916033002/?
https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... File Source/core/dom/ActiveDOMObject.cpp (right): https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) On 2015/03/16 23:44:22, haraken wrote: > > Is it safe to touch executionContext() here? We've deemed previously that null checking a (Weak)Member but dead reference is safe; right? executionContext() fetches out m_lifecycleContext only. https://codereview.chromium.org/1006253002/diff/60001/Source/modules/webmidi/... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/1006253002/diff/60001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:52: LifecycleObserver::contextDestroyed(); On 2015/03/16 23:44:22, haraken wrote: > > Why didn't we have this code previously? Was this a bug? It was there before, but reached via LifecycleObserver::dispose(). It was a bug that we didn't delegate to ScriptPromiseResolver::dispose() in the assert-enabled case, however. https://codereview.chromium.org/1006253002/diff/60001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:73: ScriptPromiseResolver::dispose(); On 2015/03/16 23:44:22, haraken wrote: > > Why didn't we have this code previously? Was this a bug? See above comment. https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... File Source/platform/LifecycleNotifier.h (right): https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:37: class LifecycleNotifier { On 2015/03/16 23:44:22, haraken wrote: > > Just help me understand: Why is it safe to use WillBeHeapHashSet without making > LifecycleNotifier a GCMixin? It's kept safe by not handling LifecycleNotifier<>* references anywhere; it's unlikely that that will start to be done later also. Objects that implement this interface must call its trace method, but this isn't checked by the plugin (I think..haven't actually tested.) Not ideal, but that's my safety argument. https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:80: // bases, but cannot itself derive from a GC base class also. On 2015/03/16 23:44:22, haraken wrote: > > Regarding the diamond inheritance problem, can we use the approach in > https://codereview.chromium.org/916033002/ A big CL; what are you referring to?
https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... File Source/core/dom/ActiveDOMObject.cpp (right): https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) On 2015/03/17 06:24:06, sof wrote: > On 2015/03/16 23:44:22, haraken wrote: > > > > Is it safe to touch executionContext() here? > > We've deemed previously that null checking a (Weak)Member but dead reference is > safe; right? executionContext() fetches out m_lifecycleContext only. Haven't we ended up with the point that it is unsafe if the ActiveDOMObject and the ExecutionContext die together? 1) The ActiveDOMObject and the ExecutionContext die together. 2) ActiveDOMObject::m_executionContext is not cleared. 3) ~ExecutionContext() is called. 4) ~ActiveDOMObject() is called. https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... File Source/platform/LifecycleNotifier.h (right): https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:37: class LifecycleNotifier { On 2015/03/17 06:24:06, sof wrote: > On 2015/03/16 23:44:22, haraken wrote: > > > > Just help me understand: Why is it safe to use WillBeHeapHashSet without > making > > LifecycleNotifier a GCMixin? > > It's kept safe by not handling LifecycleNotifier<>* references anywhere; it's > unlikely that that will start to be done later also. Objects that implement this > interface must call its trace method, but this isn't checked by the plugin (I > think..haven't actually tested.) > > Not ideal, but that's my safety argument. Thanks for the clarification; makes sense. https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:80: // bases, but cannot itself derive from a GC base class also. On 2015/03/17 06:24:06, sof wrote: > On 2015/03/16 23:44:22, haraken wrote: > > > > Regarding the diamond inheritance problem, can we use the approach in > > https://codereview.chromium.org/916033002/ > > A big CL; what are you referring to? The public virtual inheritance in Supplementable.h and LifecycleNotifier.h. https://codereview.chromium.org/916033002/diff/1/Source/platform/Supplementab...
https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... File Source/core/dom/ActiveDOMObject.cpp (right): https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) On 2015/03/17 08:29:36, haraken wrote: > On 2015/03/17 06:24:06, sof wrote: > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > Is it safe to touch executionContext() here? > > > > We've deemed previously that null checking a (Weak)Member but dead reference > is > > safe; right? executionContext() fetches out m_lifecycleContext only. > > Haven't we ended up with the point that it is unsafe if the ActiveDOMObject and > the ExecutionContext die together? > > 1) The ActiveDOMObject and the ExecutionContext die together. > 2) ActiveDOMObject::m_executionContext is not cleared. > 3) ~ExecutionContext() is called. > 4) ~ActiveDOMObject() is called. > It wouldn't be null at step 4, so the check would fail. How do you see this as unsafe?
https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... File Source/core/dom/ActiveDOMObject.cpp (right): https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) On 2015/03/17 08:34:42, sof wrote: > On 2015/03/17 08:29:36, haraken wrote: > > On 2015/03/17 06:24:06, sof wrote: > > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > > > Is it safe to touch executionContext() here? > > > > > > We've deemed previously that null checking a (Weak)Member but dead reference > > is > > > safe; right? executionContext() fetches out m_lifecycleContext only. > > > > Haven't we ended up with the point that it is unsafe if the ActiveDOMObject > and > > the ExecutionContext die together? > > > > 1) The ActiveDOMObject and the ExecutionContext die together. > > 2) ActiveDOMObject::m_executionContext is not cleared. > > 3) ~ExecutionContext() is called. > > 4) ~ActiveDOMObject() is called. > > > > It wouldn't be null at step 4, so the check would fail. How do you see this as > unsafe? Yes, it wouldn't be null. So you'll call executionContext()->isContextThread() for an ExecutionContext that has been already destructed?
On 2015/03/17 08:36:23, haraken wrote: > https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... > File Source/core/dom/ActiveDOMObject.cpp (right): > > https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... > Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) > On 2015/03/17 08:34:42, sof wrote: > > On 2015/03/17 08:29:36, haraken wrote: > > > On 2015/03/17 06:24:06, sof wrote: > > > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > > > > > Is it safe to touch executionContext() here? > > > > > > > > We've deemed previously that null checking a (Weak)Member but dead > reference > > > is > > > > safe; right? executionContext() fetches out m_lifecycleContext only. > > > > > > Haven't we ended up with the point that it is unsafe if the ActiveDOMObject > > and > > > the ExecutionContext die together? > > > > > > 1) The ActiveDOMObject and the ExecutionContext die together. > > > 2) ActiveDOMObject::m_executionContext is not cleared. > > > 3) ~ExecutionContext() is called. > > > 4) ~ActiveDOMObject() is called. > > > > > > > It wouldn't be null at step 4, so the check would fail. How do you see this as > > unsafe? > > Yes, it wouldn't be null. So you'll call executionContext()->isContextThread() > for an ExecutionContext that has been already destructed? But that's not enabled with Oilpan?
On 2015/03/17 08:37:14, sof wrote: > On 2015/03/17 08:36:23, haraken wrote: > > > https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... > > File Source/core/dom/ActiveDOMObject.cpp (right): > > > > > https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... > > Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) > > On 2015/03/17 08:34:42, sof wrote: > > > On 2015/03/17 08:29:36, haraken wrote: > > > > On 2015/03/17 06:24:06, sof wrote: > > > > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > > > > > > > Is it safe to touch executionContext() here? > > > > > > > > > > We've deemed previously that null checking a (Weak)Member but dead > > reference > > > > is > > > > > safe; right? executionContext() fetches out m_lifecycleContext only. > > > > > > > > Haven't we ended up with the point that it is unsafe if the > ActiveDOMObject > > > and > > > > the ExecutionContext die together? > > > > > > > > 1) The ActiveDOMObject and the ExecutionContext die together. > > > > 2) ActiveDOMObject::m_executionContext is not cleared. > > > > 3) ~ExecutionContext() is called. > > > > 4) ~ActiveDOMObject() is called. > > > > > > > > > > It wouldn't be null at step 4, so the check would fail. How do you see this > as > > > unsafe? > > > > Yes, it wouldn't be null. So you'll call executionContext()->isContextThread() > > for an ExecutionContext that has been already destructed? > > But that's not enabled with Oilpan? Then how about having: ASSERT(m_suspendIfNeededCalled); #if !ENABLE(OILPAN) ASSERT(!executionContext() || executionContext()->isContextThread()); #endif ?
https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... File Source/platform/LifecycleNotifier.h (right): https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:80: // bases, but cannot itself derive from a GC base class also. On 2015/03/17 08:29:36, haraken wrote: > On 2015/03/17 06:24:06, sof wrote: > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > Regarding the diamond inheritance problem, can we use the approach in > > > https://codereview.chromium.org/916033002/ > > > > A big CL; what are you referring to? > > The public virtual inheritance in Supplementable.h and LifecycleNotifier.h. > > https://codereview.chromium.org/916033002/diff/1/Source/platform/Supplementab... Thanks; virtual inheritance brings overhead and complexity that needs to be considered with some care. crbug.com/467502 seems like a natural place for that.
On 2015/03/17 08:49:54, haraken wrote: > On 2015/03/17 08:37:14, sof wrote: > > On 2015/03/17 08:36:23, haraken wrote: > > > > > > https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... > > > File Source/core/dom/ActiveDOMObject.cpp (right): > > > > > > > > > https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... > > > Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) > > > On 2015/03/17 08:34:42, sof wrote: > > > > On 2015/03/17 08:29:36, haraken wrote: > > > > > On 2015/03/17 06:24:06, sof wrote: > > > > > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > > > > > > > > > Is it safe to touch executionContext() here? > > > > > > > > > > > > We've deemed previously that null checking a (Weak)Member but dead > > > reference > > > > > is > > > > > > safe; right? executionContext() fetches out m_lifecycleContext only. > > > > > > > > > > Haven't we ended up with the point that it is unsafe if the > > ActiveDOMObject > > > > and > > > > > the ExecutionContext die together? > > > > > > > > > > 1) The ActiveDOMObject and the ExecutionContext die together. > > > > > 2) ActiveDOMObject::m_executionContext is not cleared. > > > > > 3) ~ExecutionContext() is called. > > > > > 4) ~ActiveDOMObject() is called. > > > > > > > > > > > > > It wouldn't be null at step 4, so the check would fail. How do you see > this > > as > > > > unsafe? > > > > > > Yes, it wouldn't be null. So you'll call > executionContext()->isContextThread() > > > for an ExecutionContext that has been already destructed? > > > > But that's not enabled with Oilpan? > > Then how about having: > > ASSERT(m_suspendIfNeededCalled); > #if !ENABLE(OILPAN) > ASSERT(!executionContext() || executionContext()->isContextThread()); > #endif > > ? If you look back in git history, you'll find the assert as being just like that (well, without the Oilpan part :) ). I seem to remember we rephrased it as part of worker-related shutdown bugs. Does it really matter, overall?
On 2015/03/17 08:54:12, sof wrote: > On 2015/03/17 08:49:54, haraken wrote: > > On 2015/03/17 08:37:14, sof wrote: > > > On 2015/03/17 08:36:23, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... > > > > File Source/core/dom/ActiveDOMObject.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1006253002/diff/60001/Source/core/dom/ActiveD... > > > > Source/core/dom/ActiveDOMObject.cpp:51: if (!executionContext()) > > > > On 2015/03/17 08:34:42, sof wrote: > > > > > On 2015/03/17 08:29:36, haraken wrote: > > > > > > On 2015/03/17 06:24:06, sof wrote: > > > > > > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > > > > > > > > > > > Is it safe to touch executionContext() here? > > > > > > > > > > > > > > We've deemed previously that null checking a (Weak)Member but dead > > > > reference > > > > > > is > > > > > > > safe; right? executionContext() fetches out m_lifecycleContext only. > > > > > > > > > > > > Haven't we ended up with the point that it is unsafe if the > > > ActiveDOMObject > > > > > and > > > > > > the ExecutionContext die together? > > > > > > > > > > > > 1) The ActiveDOMObject and the ExecutionContext die together. > > > > > > 2) ActiveDOMObject::m_executionContext is not cleared. > > > > > > 3) ~ExecutionContext() is called. > > > > > > 4) ~ActiveDOMObject() is called. > > > > > > > > > > > > > > > > It wouldn't be null at step 4, so the check would fail. How do you see > > this > > > as > > > > > unsafe? > > > > > > > > Yes, it wouldn't be null. So you'll call > > executionContext()->isContextThread() > > > > for an ExecutionContext that has been already destructed? > > > > > > But that's not enabled with Oilpan? > > > > Then how about having: > > > > ASSERT(m_suspendIfNeededCalled); > > #if !ENABLE(OILPAN) > > ASSERT(!executionContext() || executionContext()->isContextThread()); > > #endif > > > > ? > > If you look back in git history, you'll find the assert as being just like that > (well, without the Oilpan part :) ). I seem to remember we rephrased it as part > of worker-related shutdown bugs. > > Does it really matter, overall? Now I understand it's not a big deal :) It just looks a bit weird that we access on-heap objects in a destructor.
https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... File Source/platform/LifecycleNotifier.h (right): https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:37: class LifecycleNotifier { On 2015/03/17 08:29:36, haraken wrote: > On 2015/03/17 06:24:06, sof wrote: > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > Just help me understand: Why is it safe to use WillBeHeapHashSet without > > making > > > LifecycleNotifier a GCMixin? > > > > It's kept safe by not handling LifecycleNotifier<>* references anywhere; it's > > unlikely that that will start to be done later also. Objects that implement > this > > interface must call its trace method, but this isn't checked by the plugin (I > > think..haven't actually tested.) > > > > Not ideal, but that's my safety argument. > > Thanks for the clarification; makes sense. Just to confirm: The reason it's safe not to make LifecycleNotifier GCMixin is that no one keeps a reference to the LifecycleNotifier and thus there is no case where we have to adjustAndMark a pointer to the LifecycleNotifier, right?
Forgot to say: LGTM.
https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... File Source/platform/LifecycleNotifier.h (right): https://codereview.chromium.org/1006253002/diff/60001/Source/platform/Lifecyc... Source/platform/LifecycleNotifier.h:37: class LifecycleNotifier { On 2015/03/17 09:15:36, haraken wrote: > On 2015/03/17 08:29:36, haraken wrote: > > On 2015/03/17 06:24:06, sof wrote: > > > On 2015/03/16 23:44:22, haraken wrote: > > > > > > > > Just help me understand: Why is it safe to use WillBeHeapHashSet without > > > making > > > > LifecycleNotifier a GCMixin? > > > > > > It's kept safe by not handling LifecycleNotifier<>* references anywhere; > it's > > > unlikely that that will start to be done later also. Objects that implement > > this > > > interface must call its trace method, but this isn't checked by the plugin > (I > > > think..haven't actually tested.) > > > > > > Not ideal, but that's my safety argument. > > > > Thanks for the clarification; makes sense. > > Just to confirm: The reason it's safe not to make LifecycleNotifier GCMixin is > that no one keeps a reference to the LifecycleNotifier and thus there is no case > where we have to adjustAndMark a pointer to the LifecycleNotifier, right? Yes, coupled with conservative GCs being locked out during construction of the objects that implement this notifier interface, an on-stack reference to this object will not be encountered. Not that it would be harmful, as the heap hash set would be empty then & without any allocations attached. Clearly this is not a satisfactory state to leave things at, longer-term.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006253002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
(Rebased to handle trivial r192001 conflict.)
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1006253002/#ps80001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006253002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192011 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
