Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
265793017:
Oilpan: move node/element rare data objects to the heap. (Closed)
|
Created:
6 years, 7 months ago by sof Modified:
6 years, 7 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), webcomponents-bugzilla_chromium.org, rwlbuis, eae+blinkwatch, dglazkov+blink, dstockwell, Timothy Loh, adamk+blink_chromium.org, darktears, Steve Block, dino_apple.com, blink-reviews-html_chromium.org, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionOilpan: move node/element rare data objects to the heap.
Turn the rare data objects for Node and Elements into garbage
collected objects.
R=haraken@chromium.org,zerny@chromium.org,erik.corry@gmail.com
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173615
Patch Set 1 #Patch Set 2 : Rebased + EventHandler.cpp oilpan compile fix #
Total comments: 12
Patch Set 3 : Rebased + assert for Nodes having no renderer on destruction. #
Total comments: 11
Patch Set 4 : Tidy up rare data creation #
Total comments: 6
Patch Set 5 : Adjust MutationObserverRegistration::create() signature slightly #
Total comments: 18
Patch Set 6 : Rebased #Patch Set 7 : Implement a weak-like Node reference from a MO registration object #
Total comments: 9
Patch Set 8 : Fix MutationObserverRegistration's weak/strong registration Node reference impl #Patch Set 9 : Remove explicit dispose() #Patch Set 10 : Reinstate dispose() for safety #
Total comments: 6
Patch Set 11 : Simplify weak reference handling #Patch Set 12 : Rebased + have MutationObserver keep a weak ref to registrations #
Total comments: 13
Patch Set 13 : Rebased + add some asserts for registration Node being present. #
Messages
Total messages: 43 (0 generated)
Please take a look. This moves the "rare data" objects themselves to the heap. The objects they refer to will be separately converted to Oilpan transition types. The exception being the ActiveAnimations persistent reference which had to be converted. There are some delicate lifetime relationships in this area, I've definitely not understood them all yet, but believe this is ready.
https://codereview.chromium.org/265793017/diff/20001/Source/core/page/EventHa... File Source/core/page/EventHandler.cpp (left): https://codereview.chromium.org/265793017/diff/20001/Source/core/page/EventHa... Source/core/page/EventHandler.cpp:3643: RefPtrWillBeRawPtr<TouchList> emptyList = TouchList::create(); The compile fix from https://codereview.chromium.org/264773015/ included here; will rebase away once the tree opens & it lands.
Thanks for working on this! Here is the first round of comments. My first impression is that we might want to make the Node hierarchy traceable (Mads is working on that) before landing this CL. If the Node hierarchy is traceable, the lifetime in this CL will become much clearer. (I'm not intending to block your CL. It would be possible to work around this CL without making the Node hierarchy traceable.) https://codereview.chromium.org/265793017/diff/20001/Source/core/animation/Ac... File Source/core/animation/ActiveAnimations.h (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/animation/Ac... Source/core/animation/ActiveAnimations.h:101: Member<Element> m_target; I don't fully understand why we need to add m_target here. Instead, probably should we change Animation::m_target from a raw pointer to a Member? https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Element.... Source/core/dom/Element.cpp:172: elementRareData()->clearShadow(); I agree that you cannot access ElementRareData in ~Element(), but then who clears the ElementShadow when the Element dies? Probably do we need to move ElementShadow to the heap and change ElementRareData::m_shadow to a Member? (However, at a first glance, it's not clear how easy it is to move ElementShadow to the heap without making the Node hierarchy traceable.) https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:74: RefPtr<Node> m_registrationNodeKeepAlive; We won't be able to remove this until we make the Node hierarchy traceable (by the next change from Mads). Currently a Member to a Node X keeps X alive but does not keep other Nodes in the same tree alive. https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:273: RELEASE_ASSERT(!renderer()); Shouldn't this line still be valid in oilpan builds? https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/shadow/E... File Source/core/dom/shadow/ElementShadow.cpp (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/shadow/E... Source/core/dom/shadow/ElementShadow.cpp:184: #endif This change is not related to this CL, and should have been committed when Mads removed guardRefs? Then let's land this part before landing this CL. https://codereview.chromium.org/265793017/diff/20001/Source/core/html/forms/T... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/html/forms/T... Source/core/html/forms/TextFieldInputType.cpp:121: #endif This part is not related to this CL, and should have been committed when Mads removed guardRefs? Then let's comment this part before landing this CL.
Thanks, great comments - will think about the others some more first. As you have rightly picked up on, ElementShadow is the problematic one here. Spent quite a bit of time moving it over also, without arriving at something that was safe & didn't leak. So, I backed out and hoped to leave that for later. https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:74: RefPtr<Node> m_registrationNodeKeepAlive; On 2014/05/04 01:46:24, haraken wrote: > > We won't be able to remove this until we make the Node hierarchy traceable (by > the next change from Mads). Currently a Member to a Node X keeps X alive but > does not keep other Nodes in the same tree alive. Hmm, quite right. It might be an odd thing to test for, but no problems directly relating to this seen. Leaving it in with a FIXME is an alternative without waiting on that larger change, I think.
https://codereview.chromium.org/265793017/diff/20001/Source/core/animation/Ac... File Source/core/animation/ActiveAnimations.h (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/animation/Ac... Source/core/animation/ActiveAnimations.h:101: Member<Element> m_target; On 2014/05/04 01:46:24, haraken wrote: > > I don't fully understand why we need to add m_target here. Instead, probably > should we change Animation::m_target from a raw pointer to a Member? With it, you'll be able to accurately notify the Animations that the Element has been destroyed in ~ActiveAnimations. I didn't want to make more changes to animations/ than needed, given that its Oilpan changes hasn't settled yet (and object relationships appear non-trivial :) ) https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:273: RELEASE_ASSERT(!renderer()); On 2014/05/04 01:46:24, haraken wrote: > > Shouldn't this line still be valid in oilpan builds? We shouldn't let go of that assert, but it needs to be split up into two - one in the rare data finalizer, and one here (if no rare data.) Done now. https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/shadow/E... File Source/core/dom/shadow/ElementShadow.cpp (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/dom/shadow/E... Source/core/dom/shadow/ElementShadow.cpp:184: #endif On 2014/05/04 01:46:24, haraken wrote: > > This change is not related to this CL, and should have been committed when Mads > removed guardRefs? Then let's land this part before landing this CL. The need to be careful on what happens in the ElementShadow destructor is made more prominent here. https://codereview.chromium.org/265793017/diff/20001/Source/core/html/forms/T... File Source/core/html/forms/TextFieldInputType.cpp (right): https://codereview.chromium.org/265793017/diff/20001/Source/core/html/forms/T... Source/core/html/forms/TextFieldInputType.cpp:121: #endif On 2014/05/04 01:46:24, haraken wrote: > > This part is not related to this CL, and should have been committed when Mads > removed guardRefs? Then let's comment this part before landing this CL. The reason why it's here in this CL is that it causes TextFieldInputType to access its element's UA shadow root element, which might be finalized already. The unordered finalization of an Element and its ElementRareData (holding the ElementShadow) is behind this; hence its inclusion here.
It is a bit hard to follow the clearing/non-clearing implied by the two modes, but it looks like a necessary step. lgtm. https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:74: ASSERT(m_registrations.isEmpty()); Did this assert actually change? (can it be non empty?). If not, I think it should still be safe to call isEmpty. (We can use collection methods if they not require the backing be alive.)
https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:74: ASSERT(m_registrations.isEmpty()); On 2014/05/05 07:50:18, zerny-chromium wrote: > Did this assert actually change? (can it be non empty?). If not, I think it > should still be safe to call isEmpty. (We can use collection methods if they not > require the backing be alive.) It has changed as a result of Node (and NodeRareData) no longer being allowed to dispose of NodeMutationObserverData when finalized. Consequently, any MutationObserverRegistration therein will not unregister with its Observer either & the above assert will now not hold in an Oilpan setting.
https://codereview.chromium.org/265793017/diff/30001/Source/core/animation/Ac... File Source/core/animation/ActiveAnimations.h (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/animation/Ac... Source/core/animation/ActiveAnimations.h:97: // Keep a back reference to the target Element, so that we know Confusing comment. Having a reference to the element does not ensure it is dead when we are finalized, only that it is not dead before. The reason they die together is that there is a strong cycle between the Element, the ElementRareData and the ActiveAnimations. But for the oilpan build it is not necessary that they die together, only that the element lives at least as long as the animations, which is what this member guarantees. https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (left): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Element.... Source/core/dom/Element.cpp:174: activeAnimations()->dispose(); It seems this could be kept inside the ifdef instead of being deleted. By removing it, you are forcing the reviewer to check that the active animations can never outlive the element in non-oilpan mode. This is hard work, and if we get it wrong we have introduced a security issue. Whereas if we just leave this in it will disappear anyway when Oilpan lands. https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:74: ASSERT(m_registrations.isEmpty()); On 2014/05/05 08:48:51, sof wrote: > On 2014/05/05 07:50:18, zerny-chromium wrote: > > Did this assert actually change? (can it be non empty?). If not, I think it > > should still be safe to call isEmpty. (We can use collection methods if they > not > > require the backing be alive.) > > It has changed as a result of Node (and NodeRareData) no longer being allowed to > dispose of NodeMutationObserverData when finalized. Consequently, any > MutationObserverRegistration therein will not unregister with its Observer > either & the above assert will now not hold in an Oilpan setting. But you removed the dispose of this from NodeRareData even in non-oilpan mode, so how does this assert pass in non-oilpan mode? https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:333: OwnPtrWillBeRawPtr<NodeRareData> data; I can't see the point of this OwnPtr. Things would be simpler if this was just a raw pointer, and you just immediately leakPtr after create. If RawPtr had a leakPtr method you could avid the ifdef completely, otherwise you have to move it to the create() calls.
https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Mutation... File Source/core/dom/MutationObserver.cpp (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Mutation... Source/core/dom/MutationObserver.cpp:74: ASSERT(m_registrations.isEmpty()); On 2014/05/05 09:31:27, Erik Corry wrote: > On 2014/05/05 08:48:51, sof wrote: > > On 2014/05/05 07:50:18, zerny-chromium wrote: > > > Did this assert actually change? (can it be non empty?). If not, I think it > > > should still be safe to call isEmpty. (We can use collection methods if they > > not > > > require the backing be alive.) > > > > It has changed as a result of Node (and NodeRareData) no longer being allowed > to > > dispose of NodeMutationObserverData when finalized. Consequently, any > > MutationObserverRegistration therein will not unregister with its Observer > > either & the above assert will now not hold in an Oilpan setting. > > But you removed the dispose of this from NodeRareData even in non-oilpan mode, > so how does this assert pass in non-oilpan mode? The rare data is deleted => the rare data OwnPtr deletes its NodeMutationObserverData => MutationObserverRegistration is destructed => dispose() is called on the attached observer & it derefs its MutationObserver => the MutationObserver is destructed on ref count hitting 0 => no registrations left. IOW, there are no one-step dependencies when it comes to mutation observers.
https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:333: OwnPtrWillBeRawPtr<NodeRareData> data; On 2014/05/05 09:31:27, Erik Corry wrote: > I can't see the point of this OwnPtr. Things would be simpler if this was just > a raw pointer, and you just immediately leakPtr after create. If RawPtr had a > leakPtr method you could avid the ifdef completely, otherwise you have to move > it to the create() calls. It minimized the #if'ery required, but I'll take another look.
https://codereview.chromium.org/265793017/diff/30001/Source/core/animation/Ac... File Source/core/animation/ActiveAnimations.h (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/animation/Ac... Source/core/animation/ActiveAnimations.h:97: // Keep a back reference to the target Element, so that we know On 2014/05/05 09:31:27, Erik Corry wrote: > Confusing comment. Having a reference to the element does not ensure it is dead > when we are finalized, only that it is not dead before. > > The reason they die together is that there is a strong cycle between the > Element, the ElementRareData and the ActiveAnimations. But for the oilpan build > it is not necessary that they die together, only that the element lives at least > as long as the animations, which is what this member guarantees. Tried to clarify the comment along those lines. https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (left): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Element.... Source/core/dom/Element.cpp:174: activeAnimations()->dispose(); On 2014/05/05 09:31:27, Erik Corry wrote: > It seems this could be kept inside the ifdef instead of being deleted. By > removing it, you are forcing the reviewer to check that the active animations > can never outlive the element in non-oilpan mode. This is hard work, and if we > get it wrong we have introduced a security issue. Whereas if we just leave this > in it will disappear anyway when Oilpan lands. Sorry, but this explicit dispose() call was a very recent addition for Oilpan purposes, which I'm now just undoing. As the CL stands here, I don't see the value of having dispose() around any longer, as we would have to call it from the dtor also in the Oilpan case (but cannot call it from here in that case, of course.) https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/265793017/diff/30001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:333: OwnPtrWillBeRawPtr<NodeRareData> data; On 2014/05/05 09:41:38, sof wrote: > On 2014/05/05 09:31:27, Erik Corry wrote: > > I can't see the point of this OwnPtr. Things would be simpler if this was > just > > a raw pointer, and you just immediately leakPtr after create. If RawPtr had a > > leakPtr method you could avid the ifdef completely, otherwise you have to move > > it to the create() calls. > > It minimized the #if'ery required, but I'll take another look. Jumped ahead a bit and made the create() return a raw pointer (in the form of a RawPtr<>) to avoid the #if's here, which is its only call site. Acceptable type for create() to have in a non-Oilpan setting?
LGTM https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:47: , m_registrationNode(®istrationNode) This is called in exactly one place where we have a pointer (this pointer) for the registration node. There seems no benefit in having the argument be a reference, we can just change this and the create() method to use pointers. https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: if (m_observer) { I wonder why we did not need this 'if' before?
https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: if (m_observer) { On 2014/05/05 14:28:46, Erik Corry wrote: > I wonder why we did not need this 'if' before? We explicitly dispose() in Node::unregisterMutationObserver() when explicitly disconnect()ing the observer registrations, as we have to. Which means we will end up calling dispose() twice for those registration objects in the non-Oilpan case. The use of #if in unregisterMutationObserver() was nay'ed in the MutationObserver CL, but perhaps we should reconsider that?
https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: if (m_observer) { On 2014/05/05 14:57:36, sof wrote: > On 2014/05/05 14:28:46, Erik Corry wrote: > > I wonder why we did not need this 'if' before? > > We explicitly dispose() in Node::unregisterMutationObserver() when explicitly > disconnect()ing the observer registrations, as we have to. Which means we will > end up calling dispose() twice for those registration objects in the non-Oilpan > case. > > The use of #if in unregisterMutationObserver() was nay'ed in the > MutationObserver CL, but perhaps we should reconsider that? I think it's cool as it is.
Thanks for the careful reviewing. (Will wait with this CL until ager's Node hierarchy CL has landed safely.) https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:47: , m_registrationNode(®istrationNode) On 2014/05/05 14:28:46, Erik Corry wrote: > This is called in exactly one place where we have a pointer (this pointer) for > the registration node. There seems no benefit in having the argument be a > reference, we can just change this and the create() method to use pointers. Yes, impedance matching now done. https://codereview.chromium.org/265793017/diff/50001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: if (m_observer) { On 2014/05/05 14:58:43, Erik Corry wrote: > On 2014/05/05 14:57:36, sof wrote: > > On 2014/05/05 14:28:46, Erik Corry wrote: > > > I wonder why we did not need this 'if' before? > > > > We explicitly dispose() in Node::unregisterMutationObserver() when explicitly > > disconnect()ing the observer registrations, as we have to. Which means we will > > end up calling dispose() twice for those registration objects in the > non-Oilpan > > case. > > > > The use of #if in unregisterMutationObserver() was nay'ed in the > > MutationObserver CL, but perhaps we should reconsider that? > > I think it's cool as it is. Thinking about it a bit more, it is preferable to do away with the explicit null check on m_observer here. It avoids having to rediscover that the check is redundant once the codebase no longer supports both non-Oilpan and Oilpan. IOW, made the call to dispose() in unregisterMutationObserver() #if-conditional.
It looks like this CL deeply depends on the assumption that the Node hierarchy is traceable, so we might want to wait for Mads' CL to land first. Given that a lot of complicated things are going on in the Node hierarchy, we might not want to accept too many wrong behaviors and pile up unsolved problems even if they are transitive. https://codereview.chromium.org/265793017/diff/70001/Source/core/animation/Ac... File Source/core/animation/ActiveAnimations.h (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/animation/Ac... Source/core/animation/ActiveAnimations.h:101: // the Element as destroyed to the above vector of Animations in the the Element => the Element https://codereview.chromium.org/265793017/diff/70001/Source/core/animation/Ac... Source/core/animation/ActiveAnimations.h:103: Member<Element> m_target; Until Mads lands the CL that makes the Node hierarchy traceable, the Member<Element> just keeps the Element alive and doesn't keep other Nodes in the tree alive. This will lead to wrong behavior, but I'm OK with it since the issue will be resolved shortly once Mads's CL lands. Let's add a FIXME about it. (A bunch of complicated things are going on around the Node hierarchy and core/animations/, so we want to be clear about what we're missing :-) https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Element.... Source/core/dom/Element.cpp:172: elementRareData()->clearShadow(); Sorry for a repeated question: In oilpan builds, no one clears the ElementShadow. What happens then? https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: m_observer->observationEnded(this); After this CL, dispose() is not called when the NodeRareData and the MutationObserverRegistration die together. This means that observationEnded() is not called when the MutationObserverRegistration dies. Thus I think you'll need to change MutationObserver::m_registration to a hash set of weak members. (Otherwise, the MutationObserverRegistration objects will never be destructed and leak.) https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:100: (*iter)->unregisterTransientMutationObserver(this); After this CL, clearTransientRegistrations() is not called when the NodeRareData and the MutationObserverRegistration die together. This means that unregisterTransientMutationObserver() is not called when the MutationObserverRegistration dies. Thus I think you'll need to change NodeMutationObserverData::transientRegistry to a hash set of weak members. (Otherwise, the MutationObserverRegistration objects will never be destructed and leak.) https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:76: RefPtr<Node> m_registrationNodeKeepAlive; I'd remove m_registrationNodeKeepAlive, even though it will lead to wrong behavior because m_registrationNode does not keep the Node tree alive. If you add RefPtr<Node> to the registration Node, I guess that the MutationObserverRegistartion object will be never destructed because we have a cycle: Node ==Member==> NodeRareData ==Member==> MutationObserverRegistartion ==RefPtr==> Node. Rather than having an always leak, it would be better to allow wrong behavior which will be fixed shortly once Mads' CL lands. (A better idea would be to wait for Mads' CL to land.) https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:348: ASSERT(!transientMutationObserverRegistry() || transientMutationObserverRegistry()->isEmpty()); I'd like to keep this ASSERT somewhere. The ASSERT about transientMutationObserverRegistry would be worth having in oilpan builds. https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/shadow/E... File Source/core/dom/shadow/ElementShadow.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/shadow/E... Source/core/dom/shadow/ElementShadow.cpp:177: InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get()); Don't we need this line in oilpan builds as well?
https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Node.cpp... Source/core/dom/Node.cpp:348: ASSERT(!transientMutationObserverRegistry() || transientMutationObserverRegistry()->isEmpty()); On 2014/05/05 16:54:52, haraken wrote: > > I'd like to keep this ASSERT somewhere. The ASSERT about > transientMutationObserverRegistry would be worth having in oilpan builds. ^^^ Ignore this comment. This comment is inconsistent with my other comment saying that transientRegistry should be a hash set of weak members.
https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: m_observer->observationEnded(this); On 2014/05/05 16:54:52, haraken wrote: > > After this CL, dispose() is not called when the NodeRareData and the > MutationObserverRegistration die together. This means that observationEnded() is > not called when the MutationObserverRegistration dies. Thus I think you'll need > to change MutationObserver::m_registration to a hash set of weak members. > (Otherwise, the MutationObserverRegistration objects will never be destructed > and leak.) observationEnded() clears the MutationObserver's recording of this observation registration. We won't finalize this object until the MutationObserver and the registration are both dead, so that book-keeping isn't required, hence not done. Why do we need weak references here?
https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:76: RefPtr<Node> m_registrationNodeKeepAlive; On 2014/05/05 16:54:52, haraken wrote: > > I'd remove m_registrationNodeKeepAlive, even though it will lead to wrong > behavior because m_registrationNode does not keep the Node tree alive. > > If you add RefPtr<Node> to the registration Node, I guess that the > MutationObserverRegistartion object will be never destructed because we have a > cycle: Node ==Member==> NodeRareData ==Member==> MutationObserverRegistartion > ==RefPtr==> Node. > > Rather than having an always leak, it would be better to allow wrong behavior > which will be fixed shortly once Mads' CL lands. > > (A better idea would be to wait for Mads' CL to land.) Definitely, this will wait until it has settled on trunk. This is one reason why I've held off on following up on some of your questions surrounding ElementShadow -- the handling there will change once that CL has landed. (I don't quite know how yet.. :) )
https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: m_observer->observationEnded(this); On 2014/05/05 18:00:54, sof wrote: > On 2014/05/05 16:54:52, haraken wrote: > > > > After this CL, dispose() is not called when the NodeRareData and the > > MutationObserverRegistration die together. This means that observationEnded() > is > > not called when the MutationObserverRegistration dies. Thus I think you'll > need > > to change MutationObserver::m_registration to a hash set of weak members. > > (Otherwise, the MutationObserverRegistration objects will never be destructed > > and leak.) > > observationEnded() clears the MutationObserver's recording of this observation > registration. We won't finalize this object until the MutationObserver and the > registration are both dead, so that book-keeping isn't required, hence not done. > > Why do we need weak references here? Sorry if I'm still misunderstanding: - Conceptually MutationObserver::m_registrations should be weak members. - Before this CL, MutationObserver::m_registrations was implemented as strong members, because we were able to explicitly clear the members when the MutationObservers die. (i.e., NodeRareData::dispose() was calling MutationObserverRegistration::dispose().) - In this CL, NodeRareData::dispose() is removed. Then I guess we need to change MutationObserver::m_registrations to weak members. https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:76: RefPtr<Node> m_registrationNodeKeepAlive; On 2014/05/05 19:35:43, sof wrote: > On 2014/05/05 16:54:52, haraken wrote: > > > > I'd remove m_registrationNodeKeepAlive, even though it will lead to wrong > > behavior because m_registrationNode does not keep the Node tree alive. > > > > If you add RefPtr<Node> to the registration Node, I guess that the > > MutationObserverRegistartion object will be never destructed because we have a > > cycle: Node ==Member==> NodeRareData ==Member==> MutationObserverRegistartion > > ==RefPtr==> Node. > > > > Rather than having an always leak, it would be better to allow wrong behavior > > which will be fixed shortly once Mads' CL lands. > > > > (A better idea would be to wait for Mads' CL to land.) > > Definitely, this will wait until it has settled on trunk. This is one reason why > I've held off on following up on some of your questions surrounding > ElementShadow -- the handling there will change once that CL has landed. (I > don't quite know how yet.. :) ) We're trying to land Mads' CL by the end of this week :)
https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: m_observer->observationEnded(this); On 2014/05/06 06:32:23, haraken wrote: > On 2014/05/05 18:00:54, sof wrote: > > On 2014/05/05 16:54:52, haraken wrote: > > > > > > After this CL, dispose() is not called when the NodeRareData and the > > > MutationObserverRegistration die together. This means that > observationEnded() > > is > > > not called when the MutationObserverRegistration dies. Thus I think you'll > > need > > > to change MutationObserver::m_registration to a hash set of weak members. > > > (Otherwise, the MutationObserverRegistration objects will never be > destructed > > > and leak.) > > > > observationEnded() clears the MutationObserver's recording of this observation > > registration. We won't finalize this object until the MutationObserver and the > > registration are both dead, so that book-keeping isn't required, hence not > done. > > > > Why do we need weak references here? > > Sorry if I'm still misunderstanding: > > - Conceptually MutationObserver::m_registrations should be weak members. > > - Before this CL, MutationObserver::m_registrations was implemented as strong > members, because we were able to explicitly clear the members when the > MutationObservers die. (i.e., NodeRareData::dispose() was calling > MutationObserverRegistration::dispose().) > > - In this CL, NodeRareData::dispose() is removed. Then I guess we need to change > MutationObserver::m_registrations to weak members. Consider the reason why we do explicit dispose()s when clearing NodeRareData currently; it is explained in https://codereview.chromium.org/236653002/#msg16 i.e., the back reference to a dead Node will now not be possible as the registration object has the registration Node as a traced Member. And the Node has the NodeRareData as a Member, so they'll disappear together. The MutationObserver will not exist with an un-sync set of registration objects, which caused trouble wrt V8 GCs striking in between Oilpan GCs. https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:76: RefPtr<Node> m_registrationNodeKeepAlive; On 2014/05/06 06:32:23, haraken wrote: > On 2014/05/05 19:35:43, sof wrote: > > On 2014/05/05 16:54:52, haraken wrote: > > > > > > I'd remove m_registrationNodeKeepAlive, even though it will lead to wrong > > > behavior because m_registrationNode does not keep the Node tree alive. > > > > > > If you add RefPtr<Node> to the registration Node, I guess that the > > > MutationObserverRegistartion object will be never destructed because we have > a > > > cycle: Node ==Member==> NodeRareData ==Member==> > MutationObserverRegistartion > > > ==RefPtr==> Node. > > > > > > Rather than having an always leak, it would be better to allow wrong > behavior > > > which will be fixed shortly once Mads' CL lands. > > > > > > (A better idea would be to wait for Mads' CL to land.) > > > > Definitely, this will wait until it has settled on trunk. This is one reason > why > > I've held off on following up on some of your questions surrounding > > ElementShadow -- the handling there will change once that CL has landed. (I > > don't quite know how yet.. :) ) > > We're trying to land Mads' CL by the end of this week :) Thanks for the update, whenever it is ready :) This CL will just get in the way, so better to wait.
https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:75: // removed for Oilpan builds. The intended semantics is that m_registrationNode is sometimes strong and sometimes weak. The way that's realized in the pre-oilpan code is that it's normally a raw pointer or C++ reference with clearing from the node destructor, but sometimes we put the same pointer in the m_registrationNodeKeepAlive field which is a RefPtr and keeps the node alive. We could have the same thing in the oilpan build by making m_registrationNode a WeakMember and m_registrationNodeKeepAlive a Member. You probably need an explicit callback for the weak member in order to remove the registration. Pre-oilpan the way it works is that we call dispose() on the registrations when the node dies (via NodeRareData::dispose()). Since the registration does not keep the node alive, this makes it a weak observation.
https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.cpp:64: m_observer->observationEnded(this); On 2014/05/06 07:13:10, sof wrote: > On 2014/05/06 06:32:23, haraken wrote: > > On 2014/05/05 18:00:54, sof wrote: > > > On 2014/05/05 16:54:52, haraken wrote: > > > > > > > > After this CL, dispose() is not called when the NodeRareData and the > > > > MutationObserverRegistration die together. This means that > > observationEnded() > > > is > > > > not called when the MutationObserverRegistration dies. Thus I think you'll > > > need > > > > to change MutationObserver::m_registration to a hash set of weak members. > > > > (Otherwise, the MutationObserverRegistration objects will never be > > destructed > > > > and leak.) > > > > > > observationEnded() clears the MutationObserver's recording of this > observation > > > registration. We won't finalize this object until the MutationObserver and > the > > > registration are both dead, so that book-keeping isn't required, hence not > > done. > > > > > > Why do we need weak references here? > > > > Sorry if I'm still misunderstanding: > > > > - Conceptually MutationObserver::m_registrations should be weak members. > > > > - Before this CL, MutationObserver::m_registrations was implemented as strong > > members, because we were able to explicitly clear the members when the > > MutationObservers die. (i.e., NodeRareData::dispose() was calling > > MutationObserverRegistration::dispose().) > > > > - In this CL, NodeRareData::dispose() is removed. Then I guess we need to > change > > MutationObserver::m_registrations to weak members. > > Consider the reason why we do explicit dispose()s when clearing NodeRareData > currently; it is explained in https://codereview.chromium.org/236653002/#msg16 > > i.e., the back reference to a dead Node will now not be possible as the > registration object has the registration Node as a traced Member. And the Node > has the NodeRareData as a Member, so they'll disappear together. The > MutationObserver will not exist with an un-sync set of registration objects, > which caused trouble wrt V8 GCs striking in between Oilpan GCs. Thanks, this makes sense. I agree that MutationObserver::m_registration should be strong members.
Rebased to work with the traced Node hierarchy + weakened the reference that MutationObserverRegistration keeps to its Node. How to dispose of a ElementShadow and its detached shadow roots is still an open question. https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/70001/Source/core/dom/Mutation... Source/core/dom/MutationObserverRegistration.h:75: // removed for Oilpan builds. On 2014/05/06 07:32:09, Erik Corry wrote: > The intended semantics is that m_registrationNode is sometimes strong and > sometimes weak. The way that's realized in the pre-oilpan code is that it's > normally a raw pointer or C++ reference with clearing from the node destructor, > but sometimes we put the same pointer in the m_registrationNodeKeepAlive field > which is a RefPtr and keeps the node alive. We could have the same thing in the > oilpan build by making m_registrationNode a WeakMember and > m_registrationNodeKeepAlive a Member. You probably need an explicit callback > for the weak member in order to remove the registration. > > Pre-oilpan the way it works is that we call dispose() on the registrations when > the node dies (via NodeRareData::dispose()). Since the registration does not > keep the node alive, this makes it a weak observation. Excellent information & description. Weakened the reference accordingly; let me know if that captures what you had in mind.
LGTM with a couple of clarifying questions about MutationObserver. I'll let the final decision to Erik, Adamk and you. (cc-ing Adam for the MutationObserver stuff.) > How to dispose of a ElementShadow and its detached shadow roots is still an open > question. Mads' CL removed the destructor, so I hope this is no longer an issue. https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/Element... File Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/Element... Source/core/dom/ElementRareData.h:44: static RawPtr<ElementRareData> create(RenderObject* renderer) This could be ElementRareData*. https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:162: dispose(); Would you elaborate on why we need to call dispose() here? I expected that this line is 'm_registrationNode = nullptr'. It seems a bit strange to me that you need to call dispose() in clearWeakMember(), because the fact that m_keepAlive is false implies that clearTransientRegistrations() is already called via dispose(). If the above observation is correct, you won't need clearWeakMembers. Alternately, you can change trace() like this: void trace(Visitor* visitor) { if (m_keepAlive) visitor->trace(m_registrationNode); } https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/NodeRar... File Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/NodeRar... Source/core/dom/NodeRareData.h:218: WillBeHeapHashSet<RawPtrWillBeMember<MutationObserverRegistration> > transientRegistry; Just to double-confirm: transientRegistry is really a strong pointer, right? It seems that: - NodeMutationObserverData outlives MutationObserverRegistrations in the transientRegistry. - transientRegistry just keeps a back pointer to MutationObserverRegistration. - These facts imply that transientRegistry is a weak pointer. However, I don't think I'm understanding the MutationObserver relationship very well, so I'll let the decision to Erik and you instead of asking you a repeated question :) https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/NodeRar... Source/core/dom/NodeRareData.h:239: static RawPtr<NodeRareData> create(RenderObject* renderer) This could be NodeRareData*. https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/shadow/... File Source/core/dom/shadow/ElementShadow.cpp (right): https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/shadow/... Source/core/dom/shadow/ElementShadow.cpp:170: // necessary/allowed, just clearing the list of roots. ElementShadow::removeDetachedShadowRoots() is not called in oilpan builds. You can remove the comment and add !ENABLE(OILPAN) to ElementShadow::removeDetachedShadowRoots(). https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/shadow/... Source/core/dom/shadow/ElementShadow.cpp:176: InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get()); It's not clear if we can remove this line, and a FIXME will be added by a follow-up CL from Mads.
Just realized that the weak-to-strong reference added for MutationObserverRegistration is wrong (it's no different than a strong ref); addressing.
Thanks again for the careful reviewing, haraken. The weak'ish Node reference in MutationObserverRegistration should now be correct, I allege.. https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:162: dispose(); On 2014/05/07 02:10:26, haraken wrote: > > Would you elaborate on why we need to call dispose() here? I expected that this > line is 'm_registrationNode = nullptr'. > > It seems a bit strange to me that you need to call dispose() in > clearWeakMember(), because the fact that m_keepAlive is false implies that > clearTransientRegistrations() is already called via dispose(). > > If the above observation is correct, you won't need clearWeakMembers. > Alternately, you can change trace() like this: > > void trace(Visitor* visitor) { > if (m_keepAlive) > visitor->trace(m_registrationNode); > } The pair of weak&strong fields stays the same (see the latest patch; the use of m_keepAlive wasn't valid). Explicitly calling dispose() vs not, is subtle. If we don't remove a registration, we run the risk of having a MutationObserver accessing a registration without a Node (e.g., when it samples the current set of registrations as part of a V8 GC). Supporting such near-dead access just makes the implementation more complex for no apparent gain. Removing it eagerly prevents that. https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/shadow/... File Source/core/dom/shadow/ElementShadow.cpp (right): https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/shadow/... Source/core/dom/shadow/ElementShadow.cpp:170: // necessary/allowed, just clearing the list of roots. On 2014/05/07 02:10:26, haraken wrote: > > ElementShadow::removeDetachedShadowRoots() is not called in oilpan builds. You > can remove the comment and add !ENABLE(OILPAN) to > ElementShadow::removeDetachedShadowRoots(). Will remove comment. https://codereview.chromium.org/265793017/diff/110001/Source/core/dom/shadow/... Source/core/dom/shadow/ElementShadow.cpp:176: InspectorInstrumentation::willPopShadowRoot(shadowHost, oldRoot.get()); On 2014/05/07 02:10:26, haraken wrote: > > It's not clear if we can remove this line, and a FIXME will be added by a > follow-up CL from Mads. Yes, that's what I was referring to when saying that disposing of ElementShadow is something not yet addressed.
https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:144: dispose(); Shouldn't this be: if (!isAlive(m_registrationNode)) { dispose(); m_registrationNode = nullptr; } ?
https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:144: dispose(); On 2014/05/07 07:10:36, haraken wrote: > > Shouldn't this be: > > if (!isAlive(m_registrationNode)) { > dispose(); > m_registrationNode = nullptr; > } > > ? m_registrationNode is a weak member, so it will be cleared if Node isn't otherwise 'alive', so this extra check and resetting would be redundant.
https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:144: dispose(); On 2014/05/07 07:10:36, haraken wrote: > > Shouldn't this be: > > if (!isAlive(m_registrationNode)) { > dispose(); > m_registrationNode = nullptr; > } > > ? The trace method is both 'tracing' the weak member, which registers a callback and also registering an explicit callback. We don't have any guarantees about the order that weak processing callbacks are done, so you should probably only do one of those. Currently the zeroing-only callbacks caused by the tracing of the weak member are done on the GCing thread, whereas the others are done on the correct thread according to which thread heap the object is on, so probably at the moment things are done in the order you expect.
https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:144: dispose(); On 2014/05/07 07:18:40, Erik Corry wrote: > On 2014/05/07 07:10:36, haraken wrote: > > > > Shouldn't this be: > > > > if (!isAlive(m_registrationNode)) { > > dispose(); > > m_registrationNode = nullptr; > > } > > > > ? > > The trace method is both 'tracing' the weak member, which registers a callback > and also registering an explicit callback. We don't have any guarantees about > the order that weak processing callbacks are done, so you should probably only > do one of those. Currently the zeroing-only callbacks caused by the tracing of > the weak member are done on the GCing thread, whereas the others are done on the > correct thread according to which thread heap the object is on, so probably at > the moment things are done in the order you expect. You're right, but that's a fragile assumption. I'm OK with it in this CL at the moment though. Either way, let's have a comment about it. Erik: Do you have any idea to make this logic saner?
We don't manually register weak handling callbacks very many places, so I don't think it's a big issue.
https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:144: dispose(); On 2014/05/07 07:32:08, haraken wrote: > On 2014/05/07 07:18:40, Erik Corry wrote: > > On 2014/05/07 07:10:36, haraken wrote: > > > > > > Shouldn't this be: > > > > > > if (!isAlive(m_registrationNode)) { > > > dispose(); > > > m_registrationNode = nullptr; > > > } > > > > > > ? > > > > The trace method is both 'tracing' the weak member, which registers a callback > > and also registering an explicit callback. We don't have any guarantees about > > the order that weak processing callbacks are done, so you should probably only > > do one of those. Currently the zeroing-only callbacks caused by the tracing > of > > the weak member are done on the GCing thread, whereas the others are done on > the > > correct thread according to which thread heap the object is on, so probably at > > the moment things are done in the order you expect. > > You're right, but that's a fragile assumption. I'm OK with it in this CL at the > moment though. Either way, let's have a comment about it. > > Erik: Do you have any idea to make this logic saner? ok, i'll leave it as a WeakMember, but not trace it (and consult isAlive() instead in the callback).
https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:144: dispose(); On 2014/05/07 07:37:51, sof wrote: > On 2014/05/07 07:32:08, haraken wrote: > > On 2014/05/07 07:18:40, Erik Corry wrote: > > > On 2014/05/07 07:10:36, haraken wrote: > > > > > > > > Shouldn't this be: > > > > > > > > if (!isAlive(m_registrationNode)) { > > > > dispose(); > > > > m_registrationNode = nullptr; > > > > } > > > > > > > > ? > > > > > > The trace method is both 'tracing' the weak member, which registers a > callback > > > and also registering an explicit callback. We don't have any guarantees > about > > > the order that weak processing callbacks are done, so you should probably > only > > > do one of those. Currently the zeroing-only callbacks caused by the tracing > > of > > > the weak member are done on the GCing thread, whereas the others are done on > > the > > > correct thread according to which thread heap the object is on, so probably > at > > > the moment things are done in the order you expect. > > > > You're right, but that's a fragile assumption. I'm OK with it in this CL at > the > > moment though. Either way, let's have a comment about it. > > > > Erik: Do you have any idea to make this logic saner? > > ok, i'll leave it as a WeakMember, but not trace it (and consult isAlive() > instead in the callback). Running into some rare crashes with that; trying to pin down.
On 2014/05/07 08:13:16, sof wrote: > https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... > File Source/core/dom/MutationObserverRegistration.cpp (right): > > https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... > Source/core/dom/MutationObserverRegistration.cpp:144: dispose(); > On 2014/05/07 07:37:51, sof wrote: > > On 2014/05/07 07:32:08, haraken wrote: > > > On 2014/05/07 07:18:40, Erik Corry wrote: > > > > On 2014/05/07 07:10:36, haraken wrote: > > > > > > > > > > Shouldn't this be: > > > > > > > > > > if (!isAlive(m_registrationNode)) { > > > > > dispose(); > > > > > m_registrationNode = nullptr; > > > > > } > > > > > > > > > > ? > > > > > > > > The trace method is both 'tracing' the weak member, which registers a > > callback > > > > and also registering an explicit callback. We don't have any guarantees > > about > > > > the order that weak processing callbacks are done, so you should probably > > only > > > > do one of those. Currently the zeroing-only callbacks caused by the > tracing > > > of > > > > the weak member are done on the GCing thread, whereas the others are done > on > > > the > > > > correct thread according to which thread heap the object is on, so > probably > > at > > > > the moment things are done in the order you expect. > > > > > > You're right, but that's a fragile assumption. I'm OK with it in this CL at > > the > > > moment though. Either way, let's have a comment about it. > > > > > > Erik: Do you have any idea to make this logic saner? > > > > ok, i'll leave it as a WeakMember, but not trace it (and consult isAlive() > > instead in the callback). > > Running into some rare crashes with that; trying to pin down. Time sink. Uploaded new patch, but seeing a spurious & random test crashing when running the MutationObserver tests in random order. Once every 20 runs or so. Will check if it repros with ToT and/or ASAN enabled (if doable with local HW, haven't built before.)
On 2014/05/07 10:16:26, sof wrote: > On 2014/05/07 08:13:16, sof wrote: > > > https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... > > File Source/core/dom/MutationObserverRegistration.cpp (right): > > > > > https://codereview.chromium.org/265793017/diff/170001/Source/core/dom/Mutatio... > > Source/core/dom/MutationObserverRegistration.cpp:144: dispose(); > > On 2014/05/07 07:37:51, sof wrote: > > > On 2014/05/07 07:32:08, haraken wrote: > > > > On 2014/05/07 07:18:40, Erik Corry wrote: > > > > > On 2014/05/07 07:10:36, haraken wrote: > > > > > > > > > > > > Shouldn't this be: > > > > > > > > > > > > if (!isAlive(m_registrationNode)) { > > > > > > dispose(); > > > > > > m_registrationNode = nullptr; > > > > > > } > > > > > > > > > > > > ? > > > > > > > > > > The trace method is both 'tracing' the weak member, which registers a > > > callback > > > > > and also registering an explicit callback. We don't have any guarantees > > > about > > > > > the order that weak processing callbacks are done, so you should > probably > > > only > > > > > do one of those. Currently the zeroing-only callbacks caused by the > > tracing > > > > of > > > > > the weak member are done on the GCing thread, whereas the others are > done > > on > > > > the > > > > > correct thread according to which thread heap the object is on, so > > probably > > > at > > > > > the moment things are done in the order you expect. > > > > > > > > You're right, but that's a fragile assumption. I'm OK with it in this CL > at > > > the > > > > moment though. Either way, let's have a comment about it. > > > > > > > > Erik: Do you have any idea to make this logic saner? > > > > > > ok, i'll leave it as a WeakMember, but not trace it (and consult isAlive() > > > instead in the callback). > > > > Running into some rare crashes with that; trying to pin down. > > Time sink. Uploaded new patch, but seeing a spurious & random test crashing when > running the MutationObserver tests in random order. Once every 20 runs or so. > > Will check if it repros with ToT and/or ASAN enabled (if doable with local HW, > haven't built before.) This turned out to be the dispose() operation sometimes causing an allocation (down deep in the backing store impl of a hash map), which isn't allowed from within a weak callback. Addressed by dropping the use of dispose() (and a custom weak callback) and made MutationObserver keep weak references to its registrations instead. The alternative would be to somehow delay dispose() calls until the end of a GC somehow, but I didn't see the facility for doing that (or otherwise accomplishing it naturally.) So, haraken's earlier suggestion of using weak references from MutationObserver turned out spot on, if for somewhat obscure reasons in the end. PTAL.
Thanks for being persistent. I double-checked and am convinced that the lifetime relationship of this CL would be OK. LGTM.
forgot to publish comments... https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Element... File Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Element... Source/core/dom/ElementRareData.h:44: static RawPtr<ElementRareData> create(RenderObject* renderer) RawPtr => * ? https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:87: m_registrationNodeKeepAlive = PassRefPtrWillBeRawPtr<Node>(m_registrationNode.get()); // Balanced in clearTransientRegistrations. Add ASSERT(m_registrationNode). https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:110: m_registrationNode->unregisterMutationObserver(this); Add: ASSERT(m_registrationNode); ASSERT(m_registrationNodeKeepAlive); https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:134: nodes.add(m_registrationNode.get()); Add: ASSERT(m_registrationNode); ASSERT(m_registrationNodeKeepAlive); https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.h (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.h:75: OwnPtr<NodeHashSet> m_transientRegistrationNodes; This could be OwnPtrWillBeMember<WillBeHeapHashSet<RefPtrWillBeMember<Node>>>, but you can address it in a follow-up CL. https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/NodeRar... File Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/NodeRar... Source/core/dom/NodeRareData.h:239: static RawPtr<NodeRareData> create(RenderObject* renderer) RawPtr => * ? https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/shadow/... File Source/core/dom/shadow/ElementShadow.h (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/shadow/... Source/core/dom/shadow/ElementShadow.h:94: DoublyLinkedList<ShadowRoot> m_shadowRoots; Add a FIXME and mention that this should be moved to the heap.
https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Element... File Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Element... Source/core/dom/ElementRareData.h:44: static RawPtr<ElementRareData> create(RenderObject* renderer) On 2014/05/08 04:14:00, haraken wrote: > > RawPtr => * ? Done https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... File Source/core/dom/MutationObserverRegistration.cpp (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:87: m_registrationNodeKeepAlive = PassRefPtrWillBeRawPtr<Node>(m_registrationNode.get()); // Balanced in clearTransientRegistrations. On 2014/05/08 04:14:00, haraken wrote: > > Add ASSERT(m_registrationNode). Done. https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:110: m_registrationNode->unregisterMutationObserver(this); On 2014/05/08 04:14:00, haraken wrote: > > Add: > > ASSERT(m_registrationNode); > ASSERT(m_registrationNodeKeepAlive); > First yes, 2nd wouldn't be right. Done. https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/Mutatio... Source/core/dom/MutationObserverRegistration.cpp:134: nodes.add(m_registrationNode.get()); On 2014/05/08 04:14:00, haraken wrote: > > Add: > > ASSERT(m_registrationNode); > ASSERT(m_registrationNodeKeepAlive); > First yes, 2nd wouldn't be right. https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/NodeRar... File Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/NodeRar... Source/core/dom/NodeRareData.h:239: static RawPtr<NodeRareData> create(RenderObject* renderer) On 2014/05/08 04:14:00, haraken wrote: > > RawPtr => * ? Done. https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/shadow/... File Source/core/dom/shadow/ElementShadow.h (right): https://codereview.chromium.org/265793017/diff/210001/Source/core/dom/shadow/... Source/core/dom/shadow/ElementShadow.h:94: DoublyLinkedList<ShadowRoot> m_shadowRoots; On 2014/05/08 04:14:00, haraken wrote: > > Add a FIXME and mention that this should be moved to the heap. Done.
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/265793017/220001
Message was sent while issue was closed.
Change committed as 173615 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
