Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
236653002:
Oilpan: move mutation observers to the Oilpan heap. (Closed)
|
Created:
6 years, 8 months ago by sof Modified:
6 years, 8 months ago CC:
blink-reviews, eae+blinkwatch, apavlov+blink_chromium.org, adamk+blink_chromium.org, aandrey+blink_chromium.org, rune+blink, Nils Barth (inactive), caseq+blink_chromium.org, arv+blink, yurys+blink_chromium.org, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, devtools-reviews_chromium.org, rwlbuis, loislo+blink_chromium.org, lushnikov+blink_chromium.org, eustas+blink_chromium.org, paulirish+reviews_chromium.org, darktears, Nate Chapin, jsbell+bindings_chromium.org, alph+blink_chromium.org, kouhei+bindings_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, ed+blinkwatch_opera.com, Inactive, watchdog-blink-watchlist_google.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionOilpan: move mutation observers to the Oilpan heap.
R=haraken@chromium.org,erik.corry@gmail.com
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172148
Patch Set 1 #Patch Set 2 : Tidy up disposal #
Total comments: 15
Patch Set 3 : Have MutationObserver keep weak references to its registrations #
Total comments: 23
Patch Set 4 : Adjust dispose() methods a bit #
Total comments: 20
Patch Set 5 : Rebased + explicitly dispose() mutation observer registrations always (non-Oilpan also.) #
Total comments: 3
Messages
Total messages: 40 (0 generated)
Please take a look. This is arguably a stopping point to full Oilpan support in this area -- it does not attempt to move NodeRareData to the heap, and face up to how to release its NodeMutationObserverData in a timely, and safe, fashion.
First round of comments :) https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:178: return *activeObservers.get(); You can use 'return *activeObservers'. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:189: return *suspendedObservers.get(); Ditto. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > m_registrations; Maybe should this be a hash set of weak members? Currently, MutationObserverRegistration in m_registrations is removed in MutationObserverRegistration's destructor. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:74: m_observer->observationEnded(this); If you make MutationObserver::m_registration a hash set of weak members, you don't need to call observationEnded(). https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:72: Node& m_registrationNode; Is this raw pointer safe even if MutationObserverRegistration outlives the Node? I guess that MutationObserverRegistration cannot outlive the registration Node in the non-oilpan world, but it can outlive in the oilpan world because MutationObserverRegistration's destruction is delayed to the next GC timing. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:2134: registration->dispose(); This looks dangerous. As far as I read a comment in MutationObserverRegistration::unregister() (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), the registration object can already die before this line. Maybe do we need to call dispose() before MutationObserverRegistration calls unregisterMutationObserver() ? https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.cpp:44: #endif Can you simple write: OwnPtrWillBePersistent<NodeMutationObserverData> m_mutationObserverData; ?
Great feedback, thanks. Some issues remain, there's at least one lifetime dependency that I haven't understood yet. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:178: return *activeObservers.get(); On 2014/04/15 02:33:25, haraken wrote: > > You can use 'return *activeObservers'. Thanks, yes. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:189: return *suspendedObservers.get(); On 2014/04/15 02:33:25, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > m_registrations; On 2014/04/15 02:33:25, haraken wrote: > > Maybe should this be a hash set of weak members? Currently, > MutationObserverRegistration in m_registrations is removed in > MutationObserverRegistration's destructor. That will work, but there is an as-yet not understood reason why the registration object will still have to eagerly de-register/remove itself as a registration in its dispose(). That is, seeing some rarer and transient crashes on shutdown at times without it. Hence, I left that call to observationEnded() in. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:72: Node& m_registrationNode; On 2014/04/15 02:33:25, haraken wrote: > > Is this raw pointer safe even if MutationObserverRegistration outlives the Node? > > I guess that MutationObserverRegistration cannot outlive the registration Node > in the non-oilpan world, but it can outlive in the oilpan world because > MutationObserverRegistration's destruction is delayed to the next GC timing. If that should happen (the registration object outlives its m_registrationNode), then that reference can only be accessed by way of its MutationObserver (calling disconnect() or getObservedNodes() on it.) Which is a possibility, I'm quite sure. Should we turn it into a pointer reference and clear it in dispose() instead? https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:2134: registration->dispose(); On 2014/04/15 02:33:25, haraken wrote: > > This looks dangerous. As far as I read a comment in > MutationObserverRegistration::unregister() > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), > the registration object can already die before this line. > > Maybe do we need to call dispose() before MutationObserverRegistration calls > unregisterMutationObserver() ? This only applies in the Oilpan case and 'registration' is on the stack, so there should be no issue here. But I can move it up, that's arguably the tidier ordering. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.cpp:44: #endif On 2014/04/15 02:33:25, haraken wrote: > > Can you simple write: > > OwnPtrWillBePersistent<NodeMutationObserverData> m_mutationObserverData; > > ? That's a good suggestion. And as m_nodeLists is an OwnPtr<>, we will still verify that sizeof(OwnPtr<T>) == sizeof(void*).
On 2014/04/15 22:00:52, sof wrote: > Great feedback, thanks. Some issues remain, there's at least one lifetime > dependency that I haven't understood yet. > The bit I was missing was the MutationObserver's visitDOMWrapper() which samples the observed nodes and indirectly touches all registration objects. Which is why the observer's set of registrations needs to be precise -- if a V8 GC hits in between a registration object is unregistered (when clearing a Node's rare data) and the next Oilpan GC happens, we run the risk of adding to-be-dead Nodes. Hmm, non-trivial. I don't see a way around having an explicit dispose() for registration objects atm.
On 2014/04/16 06:48:38, sof wrote: > On 2014/04/15 22:00:52, sof wrote: > > Great feedback, thanks. Some issues remain, there's at least one lifetime > > dependency that I haven't understood yet. > > > > The bit I was missing was the MutationObserver's visitDOMWrapper() which samples > the observed nodes and indirectly touches all registration objects. Which is why > the observer's set of registrations needs to be precise -- if a V8 GC hits in > between a registration object is unregistered (when clearing a Node's rare data) > and the next Oilpan GC happens, we run the risk of adding to-be-dead Nodes. > > Hmm, non-trivial. I don't see a way around having an explicit dispose() for > registration objects atm. Erik: I think you moved MutationObserver to the heap in the experimental branch. Do you have any idea on this?
Sorry for the review delay. I'm happy to take another close look once the on-going issue is resolved. https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:72: Node& m_registrationNode; On 2014/04/15 22:00:53, sof wrote: > On 2014/04/15 02:33:25, haraken wrote: > > > > Is this raw pointer safe even if MutationObserverRegistration outlives the > Node? > > > > I guess that MutationObserverRegistration cannot outlive the registration Node > > in the non-oilpan world, but it can outlive in the oilpan world because > > MutationObserverRegistration's destruction is delayed to the next GC timing. > > If that should happen (the registration object outlives its m_registrationNode), > then that reference can only be accessed by way of its MutationObserver (calling > disconnect() or getObservedNodes() on it.) Which is a possibility, I'm quite > sure. > > Should we turn it into a pointer reference and clear it in dispose() instead? This sounds nicer.
https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > m_registrations; On 2014/04/15 22:00:53, sof wrote: > On 2014/04/15 02:33:25, haraken wrote: > > > > Maybe should this be a hash set of weak members? Currently, > > MutationObserverRegistration in m_registrations is removed in > > MutationObserverRegistration's destructor. > > That will work, but there is an as-yet not understood reason why the > registration object will still have to eagerly de-register/remove itself as a > registration in its dispose(). That is, seeing some rarer and transient crashes > on shutdown at times without it. Hence, I left that call to observationEnded() > in. I think you can have it eagerly deregister itself in dispose, but have it also be weak so that you don't have to call dispose from the destructor. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:61: // the current sweep, this dead registration object might This doesn't make any sense to me. The pointer from an observer to its registrations is now strong, so if the observer is alive then so is this observer registration.
Ah, oops, I didn't see you already uploaded a version that implemented my suggestions!
Please read these two comments in the reverse order (relative to each other, don't read the actual comment from last-word to first-word!) https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); I think if the transientRegistrations were a weak set, you would not have to call this from the destructor, and this would remove a use-other-objects-from-the-destructor issue from the future where nodes are on-heap and potentially die at the same time as the MutationObserverRegistration. Currently transientRegistrations is a vector but there is no reason it can't be a HashSet, making it possible to make it weak. You would still need to keep clearTransientRegistrations around for non-destructor use. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:111: (*iter)->unregisterTransientMutationObserver(this); If I understand this right, this MutationObserverRegistration has a set of nodes that is a set of OwnPtrs, so we can be sure that they are not dead yet (we are keeping them alive until at least the end of the destructor). Therefore it's OK to access them here from the destructor and to indirectly access their NodeRareData (that's where the transientMutationObservers are) even though that is on-heap. The node, and thus its on-heap NodeRareData are being kept alive until the next GC.
Thanks Erik, I hope my followup comments makes more sense when read LTR rather than RTL, but no guarantees :) https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); On 2014/04/16 08:20:48, Erik Corry wrote: > I think if the transientRegistrations were a weak set, you would not have to > call this from the destructor, and this would remove a > use-other-objects-from-the-destructor issue from the future where nodes are > on-heap and potentially die at the same time as the > MutationObserverRegistration. Currently transientRegistrations is a vector but > there is no reason it can't be a HashSet, making it possible to make it weak. > > You would still need to keep clearTransientRegistrations around for > non-destructor use. This seems very relevant to what I'm faced with (now and later), but I don't understand what it refers to -- m_transientRegistrationNodes is a HashSet of Nodes right now. What vector is this? But I think a/the reason for why explicit disposal is needed doesn't directly lie here. But how to destruct the MutationObserverRegistration objects that a NodeMutationObserverData keeps. If these don't promptly dispose and unregister themselves from their MutationObserver, that observer object can outlive them and refer to now-duff objects & nodes. Should a V8 GC then strike, the MutationObserver wrapper will sample all observed nodes, and fail trying to access a dead Node. iow, the dispose() is needed and I suggest we leave it here. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:111: (*iter)->unregisterTransientMutationObserver(this); On 2014/04/16 08:20:48, Erik Corry wrote: > If I understand this right, this MutationObserverRegistration has a set of nodes > that is a set of OwnPtrs, so we can be sure that they are not dead yet (we are > keeping them alive until at least the end of the destructor). Therefore it's OK > to access them here from the destructor and to indirectly access their > NodeRareData (that's where the transientMutationObservers are) even though that > is on-heap. The node, and thus its on-heap NodeRareData are being kept alive > until the next GC. I think that's correct and is safe. However, the NodeRareData has a reference to a NodeMutationObserverData object, which contains a vector and a set to other MutationObserverRegistrations (the registry and transient registry, respectively.) Since these objects are heap allocated also, they cannot be accessed in the destructor here, as they may or may not have been finalized already. This is currently "solved" by keeping a persistent in NodeRareData to NodeMutationObserverData to extend the lifetime of that object to the (N+1)th GC & allow safe access to it from here. I haven't worked out how to avoid that, but I don't like having to rely on a persistent reference like that.
On 2014/04/16 07:23:57, haraken wrote: > Sorry for the review delay. I'm happy to take another close look once the > on-going issue is resolved. > > https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... > File Source/core/dom/MutationObserverRegistration.h (right): > > https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... > Source/core/dom/MutationObserverRegistration.h:72: Node& m_registrationNode; > On 2014/04/15 22:00:53, sof wrote: > > On 2014/04/15 02:33:25, haraken wrote: > > > > > > Is this raw pointer safe even if MutationObserverRegistration outlives the > > Node? > > > > > > I guess that MutationObserverRegistration cannot outlive the registration > Node > > > in the non-oilpan world, but it can outlive in the oilpan world because > > > MutationObserverRegistration's destruction is delayed to the next GC timing. > > > > If that should happen (the registration object outlives its > m_registrationNode), > > then that reference can only be accessed by way of its MutationObserver > (calling > > disconnect() or getObservedNodes() on it.) Which is a possibility, I'm quite > > sure. > > > > Should we turn it into a pointer reference and clear it in dispose() instead? > > This sounds nicer. If MutationObserverRegistration objects are explicitly dispose()d, which I suggest we do as in the latest patchset here, then this won't be an issue -- they're not reachable from their (old) MutationObserver after that. Hence, m_registrationNode can stay a reference.
I believe this is ready, but if it helps for anyone having a look, a short summary of the open issues scattered among the comments here: - Is the use of a Persistent on NodeRareData acceptable? Acceptable today and later on when Node (I presume) will stop being a ref-counted heap object (i.e., is it just delaying solving a lifetime issue.) - Do these inter-related objects require an explicit dispose() method on MutationObserverRegistration to handle unregistration from each other? I'm reasonably confident by now that they do. [The extensive sequence of public holidays that is Norwegian Easter is upon me from today until Tuesday, but will do my best to follow up on any feedback.]
https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:67: dispose(); Can you restructure the code so that dispose() is not called in the destructor in non-oilpan builds? Currently dispose() is called in the destructor in non-oilpan builds. On the other hand, dispose() is called before the destructor in oilpan builds. Having two call paths is a bit confusing to verify correctness. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); On 2014/04/16 15:29:47, sof wrote: > On 2014/04/16 08:20:48, Erik Corry wrote: > > I think if the transientRegistrations were a weak set, you would not have to > > call this from the destructor, and this would remove a > > use-other-objects-from-the-destructor issue from the future where nodes are > > on-heap and potentially die at the same time as the > > MutationObserverRegistration. Currently transientRegistrations is a vector > but > > there is no reason it can't be a HashSet, making it possible to make it weak. > > > > You would still need to keep clearTransientRegistrations around for > > non-destructor use. > > This seems very relevant to what I'm faced with (now and later), but I don't > understand what it refers to -- m_transientRegistrationNodes is a HashSet of > Nodes right now. What vector is this? > > But I think a/the reason for why explicit disposal is needed doesn't directly > lie here. But how to destruct the MutationObserverRegistration objects that a > NodeMutationObserverData keeps. If these don't promptly dispose and unregister > themselves from their MutationObserver, that observer object can outlive them > and refer to now-duff objects & nodes. Should a V8 GC then strike, the > MutationObserver wrapper will sample all observed nodes, and fail trying to > access a dead Node. Just help me understand: At the point where V8's GC is triggered, those nodes are not yet dead (because Oilpan's GC is not yet executed). Thus there is no problem if the observer touches the observed nodes, isn't it? https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:111: (*iter)->unregisterTransientMutationObserver(this); On 2014/04/16 15:29:47, sof wrote: > On 2014/04/16 08:20:48, Erik Corry wrote: > > If I understand this right, this MutationObserverRegistration has a set of > nodes > > that is a set of OwnPtrs, so we can be sure that they are not dead yet (we are > > keeping them alive until at least the end of the destructor). Therefore it's > OK > > to access them here from the destructor and to indirectly access their > > NodeRareData (that's where the transientMutationObservers are) even though > that > > is on-heap. The node, and thus its on-heap NodeRareData are being kept alive > > until the next GC. > > I think that's correct and is safe. > > However, the NodeRareData has a reference to a NodeMutationObserverData object, > which contains a vector and a set to other MutationObserverRegistrations (the > registry and transient registry, respectively.) Since these objects are heap > allocated also, they cannot be accessed in the destructor here, as they may or > may not have been finalized already. This is currently "solved" by keeping a > persistent in NodeRareData to NodeMutationObserverData to extend the lifetime of > that object to the (N+1)th GC & allow safe access to it from here. I haven't > worked out how to avoid that, but I don't like having to rely on a persistent > reference like that. Agreed with sof@. It's nicer if we could move more things out of the destructor. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:333: #if ENABLE(OILPAN) Nit: You can drop the ENABLE(OILPAN) flag from here. You can just add the flag inside NodeRareData::dispose() so that non-oilpan builds do nothing in NodeRareData::dispose(). https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:2135: registration->dispose(); Help me understand: In my understanding, all MutaionObserverRegistration objects are registered in NodeMutationObserverData::registry. And NodeMutationObserverData::registry is responsible for calling dispose() for the MutaionObserverRegistration objects. If so, why do we need to call dispose() here too? https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.cpp:66: m_mutationObserverData.clear(); At this point, is it guaranteed that NodeMutationObserverData::transientRegistry is empty? I wonder if it's guaranteed that all registries are correctly unregistered from transientRegistry. Otherwise, it looks unsafe to call MutationObserverRegistry::dispose() here, because transientRegistry still holds a strong reference to the registries.
On 2014/04/16 15:58:19, sof wrote: > On 2014/04/16 07:23:57, haraken wrote: > > Sorry for the review delay. I'm happy to take another close look once the > > on-going issue is resolved. > > > > > https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... > > File Source/core/dom/MutationObserverRegistration.h (right): > > > > > https://codereview.chromium.org/236653002/diff/20001/Source/core/dom/Mutation... > > Source/core/dom/MutationObserverRegistration.h:72: Node& m_registrationNode; > > On 2014/04/15 22:00:53, sof wrote: > > > On 2014/04/15 02:33:25, haraken wrote: > > > > > > > > Is this raw pointer safe even if MutationObserverRegistration outlives the > > > Node? > > > > > > > > I guess that MutationObserverRegistration cannot outlive the registration > > Node > > > > in the non-oilpan world, but it can outlive in the oilpan world because > > > > MutationObserverRegistration's destruction is delayed to the next GC > timing. > > > > > > If that should happen (the registration object outlives its > > m_registrationNode), > > > then that reference can only be accessed by way of its MutationObserver > > (calling > > > disconnect() or getObservedNodes() on it.) Which is a possibility, I'm quite > > > sure. > > > > > > Should we turn it into a pointer reference and clear it in dispose() > instead? > > > > This sounds nicer. > > If MutationObserverRegistration objects are explicitly dispose()d, which I > suggest we do as in the latest patchset here, then this won't be an issue -- > they're not reachable from their (old) MutationObserver after that. Hence, > m_registrationNode can stay a reference. This is a good point. It sounds better to explicitly disposing MutationObserverRegistration objects, at least until we move the Node hierarchy to the heap. Note: > Just help me understand: At the point where V8's GC is triggered, those nodes > are not yet dead (because Oilpan's GC is not yet executed). Thus there is no > problem if the observer touches the observed nodes, isn't it? ^^^ The above comment in my previous review is just a clarifying question. I'm thinking that the explicit dispose() is a better idea than making transientRegistry a hash set of weak members.
The clarifying questions are really helpful in firming up understanding, many thanks. Comments below. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:67: dispose(); On 2014/04/17 05:37:12, haraken wrote: > > Can you restructure the code so that dispose() is not called in the destructor > in non-oilpan builds? > > Currently dispose() is called in the destructor in non-oilpan builds. On the > other hand, dispose() is called before the destructor in oilpan builds. Having > two call paths is a bit confusing to verify correctness. Certainly; have a look now. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); On 2014/04/17 05:37:12, haraken wrote: > On 2014/04/16 15:29:47, sof wrote: > > On 2014/04/16 08:20:48, Erik Corry wrote: > > > I think if the transientRegistrations were a weak set, you would not have to > > > call this from the destructor, and this would remove a > > > use-other-objects-from-the-destructor issue from the future where nodes are > > > on-heap and potentially die at the same time as the > > > MutationObserverRegistration. Currently transientRegistrations is a vector > > but > > > there is no reason it can't be a HashSet, making it possible to make it > weak. > > > > > > You would still need to keep clearTransientRegistrations around for > > > non-destructor use. > > > > This seems very relevant to what I'm faced with (now and later), but I don't > > understand what it refers to -- m_transientRegistrationNodes is a HashSet of > > Nodes right now. What vector is this? > > > > But I think a/the reason for why explicit disposal is needed doesn't directly > > lie here. But how to destruct the MutationObserverRegistration objects that a > > NodeMutationObserverData keeps. If these don't promptly dispose and unregister > > themselves from their MutationObserver, that observer object can outlive them > > and refer to now-duff objects & nodes. Should a V8 GC then strike, the > > MutationObserver wrapper will sample all observed nodes, and fail trying to > > access a dead Node. > > Just help me understand: At the point where V8's GC is triggered, those nodes > are not yet dead (because Oilpan's GC is not yet executed). Thus there is no > problem if the observer touches the observed nodes, isn't it? Consider what happens when a Node is destructed and it has rare data. Its persistent reference to a NodeMutationObserverData (holding MutationObserverRegistration references) is cleared, but it won't be GCed this time around. So, if a V8 GC then strikes, the MutationObserver can refer to the MutationObserverRegistration objects that the recently freed-but-not-yet-Oilpan-GCed NodeMutationObserverData holds, thus the dead Node can be accessed. By explicitly dipose()ing the registration objects, the MutationObserver's set of registrations is up-to-date at the end of an Oilpan GC, and the above situation is avoided. Does that clarify? https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:333: #if ENABLE(OILPAN) On 2014/04/17 05:37:12, haraken wrote: > > Nit: You can drop the ENABLE(OILPAN) flag from here. You can just add the flag > inside NodeRareData::dispose() so that non-oilpan builds do nothing in > NodeRareData::dispose(). ok, done that instead. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:2135: registration->dispose(); On 2014/04/17 05:37:12, haraken wrote: > > Help me understand: > > In my understanding, all MutaionObserverRegistration objects are registered in > NodeMutationObserverData::registry. And NodeMutationObserverData::registry is > responsible for calling dispose() for the MutaionObserverRegistration objects. > If so, why do we need to call dispose() here too? > If you explicitly disconnect() a MutationObserver, you clear out the observer's list of registrations. If we don't call dispose() here, but just clear out that list/vector, MutationObserverRegistration objects are in a state where when we later will call dispose(), they have a reference back to their observer, but not the other way around. And I'm not sure what state the transient registrations are in either, by that point. So, to keep things simpler and more regular, explicitly disposing the registration object here (to mimic what happens in the non-oilpan case), makes good sense. This question made me realize that the explicit clearing of m_registrations in MutationObserver::disconnect() is quite redundant -- it will be empty. I've replaced it with an assert. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.cpp:66: m_mutationObserverData.clear(); On 2014/04/17 05:37:12, haraken wrote: > > At this point, is it guaranteed that NodeMutationObserverData::transientRegistry > is empty? I wonder if it's guaranteed that all registries are correctly > unregistered from transientRegistry. Otherwise, it looks unsafe to call > MutationObserverRegistry::dispose() here, because transientRegistry still holds > a strong reference to the registries. That condition (an empty transient registry) is asserted for when starting to clear out a Node's rare data. Which is the only place we call NodeRareData::dispose() from.
https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); On 2014/04/17 12:41:55, sof wrote: > On 2014/04/17 05:37:12, haraken wrote: > > On 2014/04/16 15:29:47, sof wrote: > > > On 2014/04/16 08:20:48, Erik Corry wrote: > > > > I think if the transientRegistrations were a weak set, you would not have > to > > > > call this from the destructor, and this would remove a > > > > use-other-objects-from-the-destructor issue from the future where nodes > are > > > > on-heap and potentially die at the same time as the > > > > MutationObserverRegistration. Currently transientRegistrations is a > vector > > > but > > > > there is no reason it can't be a HashSet, making it possible to make it > > weak. > > > > > > > > You would still need to keep clearTransientRegistrations around for > > > > non-destructor use. > > > > > > This seems very relevant to what I'm faced with (now and later), but I don't > > > understand what it refers to -- m_transientRegistrationNodes is a HashSet of > > > Nodes right now. What vector is this? > > > > > > But I think a/the reason for why explicit disposal is needed doesn't > directly > > > lie here. But how to destruct the MutationObserverRegistration objects that > a > > > NodeMutationObserverData keeps. If these don't promptly dispose and > unregister > > > themselves from their MutationObserver, that observer object can outlive > them > > > and refer to now-duff objects & nodes. Should a V8 GC then strike, the > > > MutationObserver wrapper will sample all observed nodes, and fail trying to > > > access a dead Node. > > > > Just help me understand: At the point where V8's GC is triggered, those nodes > > are not yet dead (because Oilpan's GC is not yet executed). Thus there is no > > problem if the observer touches the observed nodes, isn't it? > > Consider what happens when a Node is destructed and it has rare data. Its > persistent reference to a NodeMutationObserverData (holding > MutationObserverRegistration references) is cleared, but it won't be GCed this > time around. So, if a V8 GC then strikes, the MutationObserver can refer to the > MutationObserverRegistration objects that the recently > freed-but-not-yet-Oilpan-GCed NodeMutationObserverData holds, thus the dead Node > can be accessed. > > By explicitly dipose()ing the registration objects, the MutationObserver's set > of registrations is up-to-date at the end of an Oilpan GC, and the above > situation is avoided. > > Does that clarify? Hmm, I think you're talking about the following situation: class Node : public RefCounted<Node> { Persistent<MutationObserverRegistration> m_a; }; class MutationObserverRegistration : public GarbageCollected<MutationObserverRegistration> { Node* m_b; // This can become a stale pointer in the situation you described above. }; class MutationObserver : public GarbageCollected<MutationObserver> { WeakMember<MutationObserverRegistration> m_c; }; Why isn't m_b cleared out when the Node object is destructed? Then even if V8 GC is triggered and access m_c->m_a, it will just return 0 and won't access the dead Node. https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:2135: registration->dispose(); On 2014/04/17 12:41:55, sof wrote: > On 2014/04/17 05:37:12, haraken wrote: > > > > Help me understand: > > > > In my understanding, all MutaionObserverRegistration objects are registered in > > NodeMutationObserverData::registry. And NodeMutationObserverData::registry is > > responsible for calling dispose() for the MutaionObserverRegistration objects. > > If so, why do we need to call dispose() here too? > > > > If you explicitly disconnect() a MutationObserver, you clear out the observer's > list of registrations. If we don't call dispose() here, but just clear out that > list/vector, MutationObserverRegistration objects are in a state where when we > later will call dispose(), they have a reference back to their observer, but not > the other way around. And I'm not sure what state the transient registrations > are in either, by that point. > > So, to keep things simpler and more regular, explicitly disposing the > registration object here (to mimic what happens in the non-oilpan case), makes > good sense. > > This question made me realize that the explicit clearing of m_registrations in > MutationObserver::disconnect() is quite redundant -- it will be empty. I've > replaced it with an assert. Makes sense! https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.cpp:66: m_mutationObserverData.clear(); On 2014/04/17 12:41:55, sof wrote: > On 2014/04/17 05:37:12, haraken wrote: > > > > At this point, is it guaranteed that > NodeMutationObserverData::transientRegistry > > is empty? I wonder if it's guaranteed that all registries are correctly > > unregistered from transientRegistry. Otherwise, it looks unsafe to call > > MutationObserverRegistry::dispose() here, because transientRegistry still > holds > > a strong reference to the registries. > > That condition (an empty transient registry) is asserted for when starting to > clear out a Node's rare data. Which is the only place we call > NodeRareData::dispose() from. Makes sense.
https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); On 2014/04/17 13:20:54, haraken wrote: > On 2014/04/17 12:41:55, sof wrote: > > On 2014/04/17 05:37:12, haraken wrote: > > > On 2014/04/16 15:29:47, sof wrote: > > > > On 2014/04/16 08:20:48, Erik Corry wrote: > > > > > I think if the transientRegistrations were a weak set, you would not > have > > to > > > > > call this from the destructor, and this would remove a > > > > > use-other-objects-from-the-destructor issue from the future where nodes > > are > > > > > on-heap and potentially die at the same time as the > > > > > MutationObserverRegistration. Currently transientRegistrations is a > > vector > > > > but > > > > > there is no reason it can't be a HashSet, making it possible to make it > > > weak. > > > > > > > > > > You would still need to keep clearTransientRegistrations around for > > > > > non-destructor use. > > > > > > > > This seems very relevant to what I'm faced with (now and later), but I > don't > > > > understand what it refers to -- m_transientRegistrationNodes is a HashSet > of > > > > Nodes right now. What vector is this? > > > > > > > > But I think a/the reason for why explicit disposal is needed doesn't > > directly > > > > lie here. But how to destruct the MutationObserverRegistration objects > that > > a > > > > NodeMutationObserverData keeps. If these don't promptly dispose and > > unregister > > > > themselves from their MutationObserver, that observer object can outlive > > them > > > > and refer to now-duff objects & nodes. Should a V8 GC then strike, the > > > > MutationObserver wrapper will sample all observed nodes, and fail trying > to > > > > access a dead Node. > > > > > > Just help me understand: At the point where V8's GC is triggered, those > nodes > > > are not yet dead (because Oilpan's GC is not yet executed). Thus there is no > > > problem if the observer touches the observed nodes, isn't it? > > > > Consider what happens when a Node is destructed and it has rare data. Its > > persistent reference to a NodeMutationObserverData (holding > > MutationObserverRegistration references) is cleared, but it won't be GCed this > > time around. So, if a V8 GC then strikes, the MutationObserver can refer to > the > > MutationObserverRegistration objects that the recently > > freed-but-not-yet-Oilpan-GCed NodeMutationObserverData holds, thus the dead > Node > > can be accessed. > > > > By explicitly dipose()ing the registration objects, the MutationObserver's set > > of registrations is up-to-date at the end of an Oilpan GC, and the above > > situation is avoided. > > > > Does that clarify? > > Hmm, I think you're talking about the following situation: > > class Node : public RefCounted<Node> { > Persistent<MutationObserverRegistration> m_a; > }; > There's a level of indirection, the persistent is on NodeMutationObserverData which has MutationObserverRegistration references. > class MutationObserverRegistration : public > GarbageCollected<MutationObserverRegistration> { > Node* m_b; // This can become a stale pointer in the situation you described > above. > }; > Yes. > class MutationObserver : public GarbageCollected<MutationObserver> { > WeakMember<MutationObserverRegistration> m_c; > }; > > Why isn't m_b cleared out when the Node object is destructed? Then even if V8 GC > is triggered and access m_c->m_a, it will just return 0 and won't access the > dead Node. If we limit ourselves to clearing out the persistent, why would it be? A persistent cleared during (Node) finalization won't cause whatever it points to be swept and finalized.
https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); On 2014/04/17 13:47:31, sof wrote: > On 2014/04/17 13:20:54, haraken wrote: > > On 2014/04/17 12:41:55, sof wrote: > > > On 2014/04/17 05:37:12, haraken wrote: > > > > On 2014/04/16 15:29:47, sof wrote: > > > > > On 2014/04/16 08:20:48, Erik Corry wrote: > > > > > > I think if the transientRegistrations were a weak set, you would not > > have > > > to > > > > > > call this from the destructor, and this would remove a > > > > > > use-other-objects-from-the-destructor issue from the future where > nodes > > > are > > > > > > on-heap and potentially die at the same time as the > > > > > > MutationObserverRegistration. Currently transientRegistrations is a > > > vector > > > > > but > > > > > > there is no reason it can't be a HashSet, making it possible to make > it > > > > weak. > > > > > > > > > > > > You would still need to keep clearTransientRegistrations around for > > > > > > non-destructor use. > > > > > > > > > > This seems very relevant to what I'm faced with (now and later), but I > > don't > > > > > understand what it refers to -- m_transientRegistrationNodes is a > HashSet > > of > > > > > Nodes right now. What vector is this? > > > > > > > > > > But I think a/the reason for why explicit disposal is needed doesn't > > > directly > > > > > lie here. But how to destruct the MutationObserverRegistration objects > > that > > > a > > > > > NodeMutationObserverData keeps. If these don't promptly dispose and > > > unregister > > > > > themselves from their MutationObserver, that observer object can outlive > > > them > > > > > and refer to now-duff objects & nodes. Should a V8 GC then strike, the > > > > > MutationObserver wrapper will sample all observed nodes, and fail trying > > to > > > > > access a dead Node. > > > > > > > > Just help me understand: At the point where V8's GC is triggered, those > > nodes > > > > are not yet dead (because Oilpan's GC is not yet executed). Thus there is > no > > > > problem if the observer touches the observed nodes, isn't it? > > > > > > Consider what happens when a Node is destructed and it has rare data. Its > > > persistent reference to a NodeMutationObserverData (holding > > > MutationObserverRegistration references) is cleared, but it won't be GCed > this > > > time around. So, if a V8 GC then strikes, the MutationObserver can refer to > > the > > > MutationObserverRegistration objects that the recently > > > freed-but-not-yet-Oilpan-GCed NodeMutationObserverData holds, thus the dead > > Node > > > can be accessed. > > > > > > By explicitly dipose()ing the registration objects, the MutationObserver's > set > > > of registrations is up-to-date at the end of an Oilpan GC, and the above > > > situation is avoided. > > > > > > Does that clarify? > > > > Hmm, I think you're talking about the following situation: > > > > class Node : public RefCounted<Node> { > > Persistent<MutationObserverRegistration> m_a; > > }; > > > > There's a level of indirection, the persistent is on NodeMutationObserverData > which has MutationObserverRegistration references. > > > class MutationObserverRegistration : public > > GarbageCollected<MutationObserverRegistration> { > > Node* m_b; // This can become a stale pointer in the situation you > described > > above. > > }; > > > > Yes. > > > class MutationObserver : public GarbageCollected<MutationObserver> { > > WeakMember<MutationObserverRegistration> m_c; > > }; > > > > Why isn't m_b cleared out when the Node object is destructed? Then even if V8 > GC > > is triggered and access m_c->m_a, it will just return 0 and won't access the > > dead Node. > > If we limit ourselves to clearing out the persistent, why would it be? A > persistent cleared during (Node) finalization won't cause whatever it points to > be swept and finalized. Hmm, this is a hard problem. It seems that the real problem is that we're leaving Node* a stale pointer when the Node is destructed. This is potentially dangerous even if V8 GC is not triggered. If someone else (other than a V8 wrapper of MutationObserverRegistration) retains a reference to MutationObserverRegistration, it can cause the same issue. In general: class X : public GarbageCollected<X> { Y* m_y; }; if we have a raw pointer inside a GarbageCollected object, we need to make sure that m_y is cleared when the Y object is destructed. Otherwise, X can outlive Y and causes use-after-free (since X's destruction is delayed to the next GC). In that sense, a right fix to our current problem would be to somehow guarantee that the Node* pointer in MutationObserverRegistration is cleared when the Node is destructed. However, it's hard to implement the logic, so you avoided the problem by explicitly disposing the registration objects. This is a bit hacky but would be OK. Once we move the Node hierarchy to the heap, we can make the Node* pointer a Member. Then the issue will be gone. Is my understanding correct?
https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); On 2014/04/17 14:01:37, haraken wrote: > On 2014/04/17 13:47:31, sof wrote: > > On 2014/04/17 13:20:54, haraken wrote: > > > On 2014/04/17 12:41:55, sof wrote: > > > > On 2014/04/17 05:37:12, haraken wrote: > > > > > On 2014/04/16 15:29:47, sof wrote: > > > > > > On 2014/04/16 08:20:48, Erik Corry wrote: > > > > > > > I think if the transientRegistrations were a weak set, you would not > > > have > > > > to > > > > > > > call this from the destructor, and this would remove a > > > > > > > use-other-objects-from-the-destructor issue from the future where > > nodes > > > > are > > > > > > > on-heap and potentially die at the same time as the > > > > > > > MutationObserverRegistration. Currently transientRegistrations is a > > > > vector > > > > > > but > > > > > > > there is no reason it can't be a HashSet, making it possible to make > > it > > > > > weak. > > > > > > > > > > > > > > You would still need to keep clearTransientRegistrations around for > > > > > > > non-destructor use. > > > > > > > > > > > > This seems very relevant to what I'm faced with (now and later), but I > > > don't > > > > > > understand what it refers to -- m_transientRegistrationNodes is a > > HashSet > > > of > > > > > > Nodes right now. What vector is this? > > > > > > > > > > > > But I think a/the reason for why explicit disposal is needed doesn't > > > > directly > > > > > > lie here. But how to destruct the MutationObserverRegistration objects > > > that > > > > a > > > > > > NodeMutationObserverData keeps. If these don't promptly dispose and > > > > unregister > > > > > > themselves from their MutationObserver, that observer object can > outlive > > > > them > > > > > > and refer to now-duff objects & nodes. Should a V8 GC then strike, the > > > > > > MutationObserver wrapper will sample all observed nodes, and fail > trying > > > to > > > > > > access a dead Node. > > > > > > > > > > Just help me understand: At the point where V8's GC is triggered, those > > > nodes > > > > > are not yet dead (because Oilpan's GC is not yet executed). Thus there > is > > no > > > > > problem if the observer touches the observed nodes, isn't it? > > > > > > > > Consider what happens when a Node is destructed and it has rare data. Its > > > > persistent reference to a NodeMutationObserverData (holding > > > > MutationObserverRegistration references) is cleared, but it won't be GCed > > this > > > > time around. So, if a V8 GC then strikes, the MutationObserver can refer > to > > > the > > > > MutationObserverRegistration objects that the recently > > > > freed-but-not-yet-Oilpan-GCed NodeMutationObserverData holds, thus the > dead > > > Node > > > > can be accessed. > > > > > > > > By explicitly dipose()ing the registration objects, the MutationObserver's > > set > > > > of registrations is up-to-date at the end of an Oilpan GC, and the above > > > > situation is avoided. > > > > > > > > Does that clarify? > > > > > > Hmm, I think you're talking about the following situation: > > > > > > class Node : public RefCounted<Node> { > > > Persistent<MutationObserverRegistration> m_a; > > > }; > > > > > > > There's a level of indirection, the persistent is on NodeMutationObserverData > > which has MutationObserverRegistration references. > > > > > class MutationObserverRegistration : public > > > GarbageCollected<MutationObserverRegistration> { > > > Node* m_b; // This can become a stale pointer in the situation you > > described > > > above. > > > }; > > > > > > > Yes. > > > > > class MutationObserver : public GarbageCollected<MutationObserver> { > > > WeakMember<MutationObserverRegistration> m_c; > > > }; > > > > > > Why isn't m_b cleared out when the Node object is destructed? Then even if > V8 > > GC > > > is triggered and access m_c->m_a, it will just return 0 and won't access the > > > dead Node. > > > > If we limit ourselves to clearing out the persistent, why would it be? A > > persistent cleared during (Node) finalization won't cause whatever it points > to > > be swept and finalized. > > Hmm, this is a hard problem. > > It seems that the real problem is that we're leaving Node* a stale pointer when > the Node is destructed. This is potentially dangerous even if V8 GC is not > triggered. If someone else (other than a V8 wrapper of > MutationObserverRegistration) retains a reference to > MutationObserverRegistration, it can cause the same issue. > Yes, it just so happens that the MutationObserver object will only access those stale pointers when its wrapper's visitDOMWrapper() is called here during a (V8) GC. > In general: > > class X : public GarbageCollected<X> { > Y* m_y; > }; > > if we have a raw pointer inside a GarbageCollected object, we need to make sure > that m_y is cleared when the Y object is destructed. Otherwise, X can outlive Y > and causes use-after-free (since X's destruction is delayed to the next GC). > > In that sense, a right fix to our current problem would be to somehow guarantee > that the Node* pointer in MutationObserverRegistration is cleared when the Node > is destructed. However, it's hard to implement the logic, so you avoided the > problem by explicitly disposing the registration objects. This is a bit hacky > but would be OK. > As I said in my followup to Erik's comment in https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... , the Persistent to extend lifetime just long enough is something I'm not thrilled about either. > Once we move the Node hierarchy to the heap, we can make the Node* pointer a > Member. Then the issue will be gone. > I'm not so sure, the unregistration of these four inter-connected heap objects (Node, MutationObserver, MutationObserverRegistration and NodeRegistrationData) will still need to happen somehow in the presence of no finalization order. > Is my understanding correct? It overlaps well with mine, fwiw. :)
If this CL is ready for full review, I'm happy to take another look.
On 2014/04/21 04:59:17, haraken wrote: > If this CL is ready for full review, I'm happy to take another look. That's a great offer, thanks, but haven't you done a full review and then some already? :) i.e., I thought the CL was waiting on other Oilpan members (or anyone else interested in MutationObservers) to go over the lifetime handling and comment on if there is a better way to encode same, or this is what we should go with. Alternatively, wait until Node is fully on the heap, if that's more efficient overall. (At this time, I have no outstanding CL changes.)
Double-checked, thanks for working on the complicated CL. Here are the final round of comments. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:156: for (WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> >::iterator iter = registrations.begin(); iter != registrations.end(); ++iter) Why do you need to copy the hash set for the iteration? https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> > m_registrations; Does this need to be a hash set of weak members? It looks like m_observer->observationEnded() is called explicitly when disposing MutationObserverRegistration. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:62: #endif As I commented previously, would it be possible to remove the #if branch? Currently dispose() is called before the destructor in oilpan builds, but dispose() is called inside the destructor. Having two call paths is not that nice. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:69: m_observer.clear(); Shall we also clear m_registrationNode in dispose() and add ASSERT(!m_registrationNode) in ~MutationObserverRegistration() ? https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:2129: #if ENABLE(OILPAN) So I think it would be nice if we could drop the #if branch. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.cpp:62: #if ENABLE(OILPAN) Ditto.
https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:156: for (WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> >::iterator iter = registrations.begin(); iter != registrations.end(); ++iter) On 2014/04/21 06:00:34, haraken wrote: > > Why do you need to copy the hash set for the iteration? The registration objects will signal the end of their interest in the observer by calling observationEnded() & remove themselves from m_registrations. So, to make iteration over the hashed set work out, it is needed. This shouldn't be a particularly large set, so I think it is acceptable. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> > m_registrations; On 2014/04/21 06:00:34, haraken wrote: > > Does this need to be a hash set of weak members? It looks like > m_observer->observationEnded() is called explicitly when disposing > MutationObserverRegistration. Doesn't have to be, but it was requested it be so earlier in this review. Change back to a strong reference? https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:62: #endif On 2014/04/21 06:00:34, haraken wrote: > > As I commented previously, would it be possible to remove the #if branch? > Currently dispose() is called before the destructor in oilpan builds, but > dispose() is called inside the destructor. Having two call paths is not that > nice. I can remove it, but why is the assert harmful to have around? https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.cpp:62: #if ENABLE(OILPAN) On 2014/04/21 06:00:34, haraken wrote: > > Ditto. Hmm, is it feasible to dispose early in the non-Oilpan case? I will have to look into again.
https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:156: for (WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> >::iterator iter = registrations.begin(); iter != registrations.end(); ++iter) On 2014/04/21 07:34:05, sof wrote: > On 2014/04/21 06:00:34, haraken wrote: > > > > Why do you need to copy the hash set for the iteration? > > The registration objects will signal the end of their interest in the observer > by calling observationEnded() & remove themselves from m_registrations. So, to > make iteration over the hashed set work out, it is needed. > > This shouldn't be a particularly large set, so I think it is acceptable. Makes sense. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> > m_registrations; On 2014/04/21 07:34:05, sof wrote: > On 2014/04/21 06:00:34, haraken wrote: > > > > Does this need to be a hash set of weak members? It looks like > > m_observer->observationEnded() is called explicitly when disposing > > MutationObserverRegistration. > > Doesn't have to be, but it was requested it be so earlier in this review. Change > back to a strong reference? Or can we keep it a weak member and comment out MutationObserver::observationEnded() in oilpan builds? https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:62: #endif On 2014/04/21 07:34:05, sof wrote: > On 2014/04/21 06:00:34, haraken wrote: > > > > As I commented previously, would it be possible to remove the #if branch? > > Currently dispose() is called before the destructor in oilpan builds, but > > dispose() is called inside the destructor. Having two call paths is not that > > nice. > > I can remove it, but why is the assert harmful to have around? I'm not concerned about having the assert. My point is just that if we could reduce the number of #if ENABLE(OILPAN), it would be better :) Having multiple call paths depending on oilpan/non-oilpan builds would make future development a bit harder.
https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> > m_registrations; On 2014/04/21 07:41:31, haraken wrote: > On 2014/04/21 07:34:05, sof wrote: > > On 2014/04/21 06:00:34, haraken wrote: > > > > > > Does this need to be a hash set of weak members? It looks like > > > m_observer->observationEnded() is called explicitly when disposing > > > MutationObserverRegistration. > > > > Doesn't have to be, but it was requested it be so earlier in this review. > Change > > back to a strong reference? > > Or can we keep it a weak member and comment out > MutationObserver::observationEnded() in oilpan builds? No, that won't work. Please see comments earlier, I won't repeat, but the observer need to have an accurate set at all times.
https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> > m_registrations; On 2014/04/21 07:43:51, sof wrote: > On 2014/04/21 07:41:31, haraken wrote: > > On 2014/04/21 07:34:05, sof wrote: > > > On 2014/04/21 06:00:34, haraken wrote: > > > > > > > > Does this need to be a hash set of weak members? It looks like > > > > m_observer->observationEnded() is called explicitly when disposing > > > > MutationObserverRegistration. > > > > > > Doesn't have to be, but it was requested it be so earlier in this review. > > Change > > > back to a strong reference? > > > > Or can we keep it a weak member and comment out > > MutationObserver::observationEnded() in oilpan builds? > > No, that won't work. Please see comments earlier, I won't repeat, but the > observer need to have an accurate set at all times. ah, remembered, sorry about the confusion. Then it would make sense to make it a hash set of Members. WeakMember implies that "it will be cleared out when a GC collects the target", but that's not a behavior we want here. I'm sorry for bothering you repeatedly.
Thanks again for the intrepid reviewing. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.h (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.h:102: WillBeHeapHashSet<RawPtrWillBeWeakMember<MutationObserverRegistration> > m_registrations; On 2014/04/21 07:45:46, haraken wrote: > On 2014/04/21 07:43:51, sof wrote: > > On 2014/04/21 07:41:31, haraken wrote: > > > On 2014/04/21 07:34:05, sof wrote: > > > > On 2014/04/21 06:00:34, haraken wrote: > > > > > > > > > > Does this need to be a hash set of weak members? It looks like > > > > > m_observer->observationEnded() is called explicitly when disposing > > > > > MutationObserverRegistration. > > > > > > > > Doesn't have to be, but it was requested it be so earlier in this review. > > > Change > > > > back to a strong reference? > > > > > > Or can we keep it a weak member and comment out > > > MutationObserver::observationEnded() in oilpan builds? > > > > No, that won't work. Please see comments earlier, I won't repeat, but the > > observer need to have an accurate set at all times. > > ah, remembered, sorry about the confusion. Then it would make sense to make it a > hash set of Members. WeakMember implies that "it will be cleared out when a GC > collects the target", but that's not a behavior we want here. I'm sorry for > bothering you repeatedly. I've switched away from weak members on MutationObserver.m_registrations to reflect that. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:62: #endif On 2014/04/21 07:41:31, haraken wrote: > On 2014/04/21 07:34:05, sof wrote: > > On 2014/04/21 06:00:34, haraken wrote: > > > > > > As I commented previously, would it be possible to remove the #if branch? > > > Currently dispose() is called before the destructor in oilpan builds, but > > > dispose() is called inside the destructor. Having two call paths is not that > > > nice. > > > > I can remove it, but why is the assert harmful to have around? > > I'm not concerned about having the assert. My point is just that if we could > reduce the number of #if ENABLE(OILPAN), it would be better :) Having multiple > call paths depending on oilpan/non-oilpan builds would make future development a > bit harder. ok, I've switched to explicit dispose()s for the non-Oilpan case also, and dropped this #if. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:69: m_observer.clear(); On 2014/04/21 06:00:34, haraken wrote: > > Shall we also clear m_registrationNode in dispose() and add > ASSERT(!m_registrationNode) in ~MutationObserverRegistration() ? m_registrationNode was left as a reference, as that was deemed safe by earlier discussions. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:2129: #if ENABLE(OILPAN) On 2014/04/21 06:00:34, haraken wrote: > > So I think it would be nice if we could drop the #if branch. Done + adjusted the comment. https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/236653002/diff/60001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.cpp:62: #if ENABLE(OILPAN) On 2014/04/21 07:34:05, sof wrote: > On 2014/04/21 06:00:34, haraken wrote: > > > > Ditto. > > Hmm, is it feasible to dispose early in the non-Oilpan case? I will have to look > into again. ..as we have identified all the places where we have to explicitly dispose() in the Oilpan case, also calling dispose() in the non-Oilpan case should be complete, and consequently there is no need to call in dispose() in the MutationObserverRegistration destructor. Done; i.e., removed the #if, here & elsewhere.
Thanks for being persistent to sometimes confusing comments from me! LGTM. I want to have one more oilpan reviewer though.
LGTM https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/236653002/diff/40001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:72: clearTransientRegistrations(); On 2014/04/16 15:29:47, sof wrote: > On 2014/04/16 08:20:48, Erik Corry wrote: > > I think if the transientRegistrations were a weak set, you would not have to > > call this from the destructor, and this would remove a > > use-other-objects-from-the-destructor issue from the future where nodes are > > on-heap and potentially die at the same time as the > > MutationObserverRegistration. Currently transientRegistrations is a vector > but > > there is no reason it can't be a HashSet, making it possible to make it weak. > > > > You would still need to keep clearTransientRegistrations around for > > non-destructor use. > > This seems very relevant to what I'm faced with (now and later), but I don't > understand what it refers to -- m_transientRegistrationNodes is a HashSet of > Nodes right now. What vector is this? Sorry, when I wrote transientRegistrations I really meant transientRegistry, which is a set of pointers to registrations. But I think really there's no reason to make it weak, because it's only raw to break a cycle. The registration keeps its transient registration nodes alive strongly and they have a set of registrations that they are currently registered with, which is transientRegistry. So in Oilpan it can just be strong and there is no need to clear transientRegistry at finalization time. > But I think a/the reason for why explicit disposal is needed doesn't directly > lie here. But how to destruct the MutationObserverRegistration objects that a > NodeMutationObserverData keeps. If these don't promptly dispose and unregister > themselves from their MutationObserver, that observer object can outlive them > and refer to now-duff objects & nodes. Should a V8 GC then strike, the > MutationObserver wrapper will sample all observed nodes, and fail trying to > access a dead Node. Good point. The m_transientRegistrationNodes and m_registrationNodeKeepAlive are RefPtr (soon Member), so those nodes can't die from under our feet, but the m_registrationNode field is supposed to have a weak pointer to the node according to the standard, and dispose() supports that. Since nodes are now on-heap I think we could handle it all with our standard weak processing of the registration->node pointer (m_registrationNode). This may require the MutationObserver to be able to handle registrations where the m_registrationNode field has been nulled out. But you don't have to make that change now. > iow, the dispose() is needed and I suggest we leave it here. Fine for now. A more general observation (this is probably not news or directly relevant, just me thinking aloud): Since nodes can be referenced from JS, it is unknown when they will die (depending on the V8 GC), so there cannot really be prompt finalization issues (at the API level) caused by the timing of node finalization. So if we can handle late finalization internally then it's OK from a web compatibility point of view. https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.h:218: WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > transientRegistry; I suppose these raw pointers were only cycle-breaking raw pointers, not weak pointers?
https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.h:218: WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > transientRegistry; On 2014/04/22 12:46:55, Erik Corry wrote: > I suppose these raw pointers were only cycle-breaking raw pointers, not weak > pointers? It would depend on how you see these pointers. In a sense that these pointers are just breaking cycles, they are weak poniters. On the other hand, in a sense that these pointers are always explicitly cleared, they can be viewed as strong pointers (Even if we make these pointers weak, there is no chance for GC to clear out the weak pointers). (Either is fine with me.)
https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... File Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... Source/core/dom/NodeRareData.h:218: WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > transientRegistry; On 2014/04/22 12:46:55, Erik Corry wrote: > I suppose these raw pointers were only cycle-breaking raw pointers, not weak > pointers? Ownership with these transients raw pointers lie with the registry of some child Node (see Node::notifyMutationObserversNodeWillDetach()). We don't have ref counting over these, so I'm not sure what that makes them.
On 2014/04/22 13:05:28, sof wrote: > https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... > File Source/core/dom/NodeRareData.h (right): > > https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... > Source/core/dom/NodeRareData.h:218: > WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > > transientRegistry; > On 2014/04/22 12:46:55, Erik Corry wrote: > > I suppose these raw pointers were only cycle-breaking raw pointers, not weak > > pointers? > > Ownership with these transients raw pointers lie with the registry of some child > Node (see Node::notifyMutationObserversNodeWillDetach()). We don't have ref > counting over these, so I'm not sure what that makes them. What I mean is: If they have to be WeakMember or RawPtr to avoid leaks in the oilpan build, then they are conceptually weak pointers. We should implement them as: WeakMember (best) WeakMember with explicit registration of a weak callback (not as nice) Raw pointer with explicit clearing from destructors using Persistent to manage lifetime issues (worst) If they have to be raw pointers to avoid cycle-caused leaks in the non-oilpan build, but the oilpan build can just have them as strong pointers, then they are not genuine weak pointers (conceptually). I think in this case they can just be strong pointers in the oilpan build, and that is how they have been implemented in this CL of which I approve.
On 2014/04/22 13:21:53, Erik Corry wrote: > On 2014/04/22 13:05:28, sof wrote: > > > https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... > > File Source/core/dom/NodeRareData.h (right): > > > > > https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... > > Source/core/dom/NodeRareData.h:218: > > WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > > > transientRegistry; > > On 2014/04/22 12:46:55, Erik Corry wrote: > > > I suppose these raw pointers were only cycle-breaking raw pointers, not weak > > > pointers? > > > > Ownership with these transients raw pointers lie with the registry of some > child > > Node (see Node::notifyMutationObserversNodeWillDetach()). We don't have ref > > counting over these, so I'm not sure what that makes them. > > What I mean is: If they have to be WeakMember or RawPtr to avoid leaks in the > oilpan build, then they are conceptually weak pointers. We should implement > them as: > > WeakMember (best) > WeakMember with explicit registration of a weak callback (not as nice) > Raw pointer with explicit clearing from destructors using Persistent to manage > lifetime issues (worst) > > If they have to be raw pointers to avoid cycle-caused leaks in the non-oilpan > build, but the oilpan build can just have them as strong pointers, then they > are not genuine weak pointers (conceptually). > > I think in this case they can just be strong pointers in the oilpan build, and > that is how they have been implemented in this CL of which I approve. Just to be clear: There is an overhead associated with weak processing, whereas cycles can be collected for free, so we should prefer strong pointers where possible.
On 2014/04/22 13:29:25, Erik Corry wrote: > On 2014/04/22 13:21:53, Erik Corry wrote: > > On 2014/04/22 13:05:28, sof wrote: > > > > > > https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... > > > File Source/core/dom/NodeRareData.h (right): > > > > > > > > > https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... > > > Source/core/dom/NodeRareData.h:218: > > > WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > > > > transientRegistry; > > > On 2014/04/22 12:46:55, Erik Corry wrote: > > > > I suppose these raw pointers were only cycle-breaking raw pointers, not > weak > > > > pointers? > > > > > > Ownership with these transients raw pointers lie with the registry of some > > child > > > Node (see Node::notifyMutationObserversNodeWillDetach()). We don't have ref > > > counting over these, so I'm not sure what that makes them. > > > > What I mean is: If they have to be WeakMember or RawPtr to avoid leaks in the > > oilpan build, then they are conceptually weak pointers. We should implement > > them as: > > > > WeakMember (best) > > WeakMember with explicit registration of a weak callback (not as nice) > > Raw pointer with explicit clearing from destructors using Persistent to manage > > lifetime issues (worst) > > > > If they have to be raw pointers to avoid cycle-caused leaks in the non-oilpan > > build, but the oilpan build can just have them as strong pointers, then they > > are not genuine weak pointers (conceptually). > > > > I think in this case they can just be strong pointers in the oilpan build, and > > that is how they have been implemented in this CL of which I approve. > > Just to be clear: There is an overhead associated with weak processing, whereas > cycles can be collected for free, so we should prefer strong pointers where > possible. Thanks, making explicit a general concern/question I had surrounding weak pointers in Oilpan. I associate overhead with weak pointer handling from other contexts; good to know.
On 2014/04/22 13:39:39, sof wrote: > On 2014/04/22 13:29:25, Erik Corry wrote: > > On 2014/04/22 13:21:53, Erik Corry wrote: > > > On 2014/04/22 13:05:28, sof wrote: > > > > > > > > > > https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... > > > > File Source/core/dom/NodeRareData.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/236653002/diff/80001/Source/core/dom/NodeRare... > > > > Source/core/dom/NodeRareData.h:218: > > > > WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > > > > > transientRegistry; > > > > On 2014/04/22 12:46:55, Erik Corry wrote: > > > > > I suppose these raw pointers were only cycle-breaking raw pointers, not > > weak > > > > > pointers? > > > > > > > > Ownership with these transients raw pointers lie with the registry of some > > > child > > > > Node (see Node::notifyMutationObserversNodeWillDetach()). We don't have > ref > > > > counting over these, so I'm not sure what that makes them. > > > > > > What I mean is: If they have to be WeakMember or RawPtr to avoid leaks in > the > > > oilpan build, then they are conceptually weak pointers. We should implement > > > them as: > > > > > > WeakMember (best) > > > WeakMember with explicit registration of a weak callback (not as nice) > > > Raw pointer with explicit clearing from destructors using Persistent to > manage > > > lifetime issues (worst) > > > > > > If they have to be raw pointers to avoid cycle-caused leaks in the > non-oilpan > > > build, but the oilpan build can just have them as strong pointers, then > they > > > are not genuine weak pointers (conceptually). > > > > > > I think in this case they can just be strong pointers in the oilpan build, > and > > > that is how they have been implemented in this CL of which I approve. > > > > Just to be clear: There is an overhead associated with weak processing, > whereas > > cycles can be collected for free, so we should prefer strong pointers where > > possible. > > Thanks, making explicit a general concern/question I had surrounding weak > pointers in Oilpan. I associate overhead with weak pointer handling from other > contexts; good to know. The nice thing about weak pointers is that they are free when pointer and pointee die together (you get no callback in that case). In that respect they are better then destructor-based handling. But strong pointers are better where possible.
Going ahead with this one now; thanks for the careful reviewing of all details.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/236653002/80001
Message was sent while issue was closed.
Change committed as 172148 |
