Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
292503006:
Oilpan: add [WillBeGarbageCollected] for Node. (Closed)
|
Created:
6 years, 7 months ago by sof Modified:
6 years, 7 months ago CC:
blink-reviews, arv+blink, eae+blinkwatch, abarth-chromium, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews-bindings_chromium.org, Inactive, watchdog-blink-watchlist_google.com, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionOilpan: add [WillBeGarbageCollected] for Node.
Also make Document hold onto its DOMWindow via a Member when
Oilpan is enabled.
R=haraken@chromium.org,ager@chromium.org,tkent@chromium.org
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174804
Patch Set 1 #Patch Set 2 : Pull in ImageBitmapTest fix for easier verification of CL #Patch Set 3 : Rebased + use transition types in XSLTProcessor #
Total comments: 2
Patch Set 4 : DOMWindow dtor fixes #
Total comments: 4
Patch Set 5 : Have Document keep a weak reference to its DOMWindow #Patch Set 6 : Rebased #
Total comments: 5
Patch Set 7 : Clear document touch event handlers upon losing DOMWindow. #
Total comments: 3
Patch Set 8 : Tidy up removeAllEventListeners() usage from DOMWindow dtor #
Total comments: 10
Patch Set 9 : Add FIXME + tighten up asserts #Patch Set 10 : Fix unit test detaching document from window #
Total comments: 1
Patch Set 11 : Rebased #
Total comments: 2
Patch Set 12 : Make Document strongly refer to its DOMWindow #Patch Set 13 : Add now-required tracing of m_domWindow (oops) #
Messages
Total messages: 48 (0 generated)
LGTM
+oilpan-reviews
https://codereview.chromium.org/292503006/diff/40001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/40001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:490: clearDocument(); Why do we have this call to clearDocument in the oilpan build? We should not be able to do that. That will access the document which could already have been destructed. If the DOMWindow and the Document die together hopefully we will not have to do anything here?
https://codereview.chromium.org/292503006/diff/40001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/40001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:490: clearDocument(); On 2014/05/23 06:27:29, Mads Ager (chromium) wrote: > Why do we have this call to clearDocument in the oilpan build? We should not be > able to do that. That will access the document which could already have been > destructed. If the DOMWindow and the Document die together hopefully we will not > have to do anything here? Oh, clearDocument() not a simple clearing setter. Yes, that's not right. Stubbing out |m_document| is needed to avoid removeAllEventListeners() from unregistering touch handlers on the window's document. The document needs to handle that.
Updated and ready for a closer look. (Will wait for https://codereview.chromium.org/288343017/ to land before taking any next steps here.)
The destruction issue of DOMWindow looks hard. https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:368: RefPtrWillBeRawPtr<Document> document; = nullptr https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:498: clearEventQueue(); I guess you need to call Document::clearDOMWindow, because the Document can outlive the DOMWindow. I also wonder why we don't need to call Document::detach() in a case where the Document outlives the DOMWindow.
https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:498: clearEventQueue(); On 2014/05/23 08:32:14, haraken wrote: > > I guess you need to call Document::clearDOMWindow, because the Document can > outlive the DOMWindow. > > I also wonder why we don't need to call Document::detach() in a case where the > Document outlives the DOMWindow. Hmm, Document needs to clear out its weak back reference to DOMWindow. Will investigate the FIXME in clearDocument() wrt detach().
Rebased on top of http://crrev.com/288343017 https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWin... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/60001/Source/core/frame/DOMWin... Source/core/frame/DOMWindow.cpp:498: clearEventQueue(); On 2014/05/23 08:32:14, haraken wrote: > > I guess you need to call Document::clearDOMWindow, because the Document can > outlive the DOMWindow. > Now arranged for in the other direction by having Document clear out its weak reference. > I also wonder why we don't need to call Document::detach() in a case where the > Document outlives the DOMWindow. I've found no evidence of clearDocument()'s detach() (via ~DOMWindow) activating. The regular path is for detaching to be handled by Document::prepareForDestruction(), as called by the frame. When might a window on destruction need to detach the document from its frame?
> > I also wonder why we don't need to call Document::detach() in a case where the > > Document outlives the DOMWindow. > > I've found no evidence of clearDocument()'s detach() (via ~DOMWindow) > activating. > > The regular path is for detaching to be handled by > Document::prepareForDestruction(), as called by the frame. When might a window > on destruction need to detach the document from its frame? hmm, a FIXME at line 342 of DOMWindow.cpp (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) confused me a lot, and I don't either understand how the Document::detach() is activated from ~DOMWindow. Shall we ask the author of the FIXME? https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Documen... Source/core/dom/Document.cpp:5724: m_domWindow.clear(); I think we can just trace m_domWindow and ask oilpan's GC to do the weak processing. https://codereview.chromium.org/292503006/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:494: m_document = nullptr; I'm not sure if it's OK to clear m_document here. If the Document and the DOMWindow die together, it's OK to clear m_document here, so that removeAllEventListeners doesn't try to document->didClearTouchEventHandlers(document). However, if the Document outlives the DOMWindow, we need to call document->didClearTouchEventHandlers(document).
haraken, thanks - awesome comments as always :) https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Documen... Source/core/dom/Document.cpp:5724: m_domWindow.clear(); On 2014/05/23 12:23:27, haraken wrote: > > I think we can just trace m_domWindow and ask oilpan's GC to do the weak > processing. I'm fine with doing that, but we already have clearWeakMembers() and avoid an extra weak callback this way, right? https://codereview.chromium.org/292503006/diff/100001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/100001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:494: m_document = nullptr; On 2014/05/23 12:23:27, haraken wrote: > > I'm not sure if it's OK to clear m_document here. > > If the Document and the DOMWindow die together, it's OK to clear m_document > here, so that removeAllEventListeners doesn't try to > document->didClearTouchEventHandlers(document). However, if the Document > outlives the DOMWindow, we need to call > document->didClearTouchEventHandlers(document). I think we can address that via DOMWindow::clearWeakMembers() when clearing m_domWindow when it goes out of scope. What do you think?
On 2014/05/23 12:23:26, haraken wrote: > > > I also wonder why we don't need to call Document::detach() in a case where > the > > > Document outlives the DOMWindow. > > > > I've found no evidence of clearDocument()'s detach() (via ~DOMWindow) > > activating. > > > > The regular path is for detaching to be handled by > > Document::prepareForDestruction(), as called by the frame. When might a window > > on destruction need to detach the document from its frame? > > hmm, a FIXME at line 342 of DOMWindow.cpp > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) > confused me a lot, and I don't either understand how the Document::detach() is > activated from ~DOMWindow. Shall we ask the author of the FIXME? > The comment is there due to DOMWindow::installNewDocument(), which also calls clearDocument(), and it being called from XSLTProcessor::createDocumentFromSource().
https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/292503006/diff/100001/Source/core/dom/Documen... Source/core/dom/Document.cpp:5724: m_domWindow.clear(); On 2014/05/23 14:29:07, sof wrote: > On 2014/05/23 12:23:27, haraken wrote: > > > > I think we can just trace m_domWindow and ask oilpan's GC to do the weak > > processing. > > I'm fine with doing that, but we already have clearWeakMembers() and avoid an > extra weak callback this way, right? You're right. https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:497: removeAllEventListeners(); Instead of moving only didClearTouchEventHandlers() into the weak processing of Document::m_domWindow, how about just moving the entire removeAllEventListeners() into the weak processing? Then you can completely comment out line 497 in oilpan builds and remove line 492 - 496. https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:500: // FIXME: This should be part of ActiveDOM Object shutdown Nit: ActiveDOMObject
https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:497: removeAllEventListeners(); On 2014/05/23 15:02:45, haraken wrote: > > Instead of moving only didClearTouchEventHandlers() into the weak processing of > Document::m_domWindow, how about just moving the entire > removeAllEventListeners() into the weak processing? > > Then you can completely comment out line 497 in oilpan builds and remove line > 492 - 496. Hmm, don't understand what you are suggesting :) The DOMWindow specific parts of removeAllEventListeners() needs to remain under DOMWindow's control, not Document and its weak processing. But perhaps you mean something else?
On 2014/05/23 15:30:30, sof wrote: > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... > File Source/core/frame/DOMWindow.cpp (right): > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... > Source/core/frame/DOMWindow.cpp:497: removeAllEventListeners(); > On 2014/05/23 15:02:45, haraken wrote: > > > > Instead of moving only didClearTouchEventHandlers() into the weak processing > of > > Document::m_domWindow, how about just moving the entire > > removeAllEventListeners() into the weak processing? > > > > Then you can completely comment out line 497 in oilpan builds and remove line > > 492 - 496. > > Hmm, don't understand what you are suggesting :) The DOMWindow specific parts of > removeAllEventListeners() needs to remain under DOMWindow's control, not > Document and its weak processing. But perhaps you mean something else? I'm sorry I was confused. You're right. However, it looks tricky that ~DOMWindow() calls removeAllEventListeners after setting m_document to 0. I think we should completely remove removeAllEventListeners from ~DOMWindow() at some point, so shall we just write like this? ~DOMWindow() { #if ENABLE(OILPAN) // FIXME: Oilpan: Should be removed once EventTarget is moved on the heap. EventTarget::removeAllEventListeners(); lifecycleNotifier().notifyRemoveAllEventListeners(this); if (m_frame && m_frame->host()) m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this); // FIXME: Oilpan: Should be removed once we make the event map weak. removeAllUnloadEventListeners(this); removeAllBeforeUnloadEventListeners(this); #else removeAllEventListeners(); #endif }
> ~DOMWindow() {
> #if ENABLE(OILPAN)
> // FIXME: Oilpan: Should be removed once EventTarget is moved on the heap.
> EventTarget::removeAllEventListeners();
> lifecycleNotifier().notifyRemoveAllEventListeners(this);
> if (m_frame && m_frame->host())
>
> m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this);
> // FIXME: Oilpan: Should be removed once we make the event map weak.
> removeAllUnloadEventListeners(this);
> removeAllBeforeUnloadEventListeners(this);
> #else
> removeAllEventListeners();
> #endif
> }
Probably we should also remove:
m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this);
as well. The EventHandlerRegistry is already on the heap and doing weak
processing for Window objects.
So I think the latest patch set touches an on-heap object (EventHandlerRegistry)
from a destructor of another on-heap object (DOMWindow). I guess it will break
ASan builds.
On 2014/05/23 15:44:13, haraken wrote: > On 2014/05/23 15:30:30, sof wrote: > > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... > > File Source/core/frame/DOMWindow.cpp (right): > > > > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... > > Source/core/frame/DOMWindow.cpp:497: removeAllEventListeners(); > > On 2014/05/23 15:02:45, haraken wrote: > > > > > > Instead of moving only didClearTouchEventHandlers() into the weak processing > > of > > > Document::m_domWindow, how about just moving the entire > > > removeAllEventListeners() into the weak processing? > > > > > > Then you can completely comment out line 497 in oilpan builds and remove > line > > > 492 - 496. > > > > Hmm, don't understand what you are suggesting :) The DOMWindow specific parts > of > > removeAllEventListeners() needs to remain under DOMWindow's control, not > > Document and its weak processing. But perhaps you mean something else? > > I'm sorry I was confused. You're right. > > However, it looks tricky that ~DOMWindow() calls removeAllEventListeners after > setting m_document to 0. I think we should completely remove > removeAllEventListeners from ~DOMWindow() at some point, so shall we just write > like this? > > ~DOMWindow() { > #if ENABLE(OILPAN) > // FIXME: Oilpan: Should be removed once EventTarget is moved on the heap. > EventTarget::removeAllEventListeners(); > lifecycleNotifier().notifyRemoveAllEventListeners(this); > if (m_frame && m_frame->host()) > > m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this); > // FIXME: Oilpan: Should be removed once we make the event map weak. > removeAllUnloadEventListeners(this); > removeAllBeforeUnloadEventListeners(this); > #else > removeAllEventListeners(); > #endif > } It's too much duplication of delicate code for my liking; we could add an "internal" version of removeAllEventListeners that takes a flag, but it would add a method slot for one single use.
On 2014/05/23 15:52:56, sof wrote: > On 2014/05/23 15:44:13, haraken wrote: > > On 2014/05/23 15:30:30, sof wrote: > > > > > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... > > > File Source/core/frame/DOMWindow.cpp (right): > > > > > > > > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... > > > Source/core/frame/DOMWindow.cpp:497: removeAllEventListeners(); > > > On 2014/05/23 15:02:45, haraken wrote: > > > > > > > > Instead of moving only didClearTouchEventHandlers() into the weak > processing > > > of > > > > Document::m_domWindow, how about just moving the entire > > > > removeAllEventListeners() into the weak processing? > > > > > > > > Then you can completely comment out line 497 in oilpan builds and remove > > line > > > > 492 - 496. > > > > > > Hmm, don't understand what you are suggesting :) The DOMWindow specific > parts > > of > > > removeAllEventListeners() needs to remain under DOMWindow's control, not > > > Document and its weak processing. But perhaps you mean something else? > > > > I'm sorry I was confused. You're right. > > > > However, it looks tricky that ~DOMWindow() calls removeAllEventListeners after > > setting m_document to 0. I think we should completely remove > > removeAllEventListeners from ~DOMWindow() at some point, so shall we just > write > > like this? > > > > ~DOMWindow() { > > #if ENABLE(OILPAN) > > // FIXME: Oilpan: Should be removed once EventTarget is moved on the heap. > > EventTarget::removeAllEventListeners(); > > lifecycleNotifier().notifyRemoveAllEventListeners(this); > > if (m_frame && m_frame->host()) > > > > m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this); > > // FIXME: Oilpan: Should be removed once we make the event map weak. > > removeAllUnloadEventListeners(this); > > removeAllBeforeUnloadEventListeners(this); > > #else > > removeAllEventListeners(); > > #endif > > } > > It's too much duplication of delicate code for my liking; we could add an > "internal" version of removeAllEventListeners that takes a flag, but it would > add a method slot for one single use. Either is fine with me, but I think we should remove m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers() in this CL, and it's easy to remove removeAllUnloadEventListeners(this) and removeAllBeforeUnloadEventListeners(this) in a follow-up.
On 2014/05/23 15:56:59, haraken wrote: > On 2014/05/23 15:52:56, sof wrote: > > On 2014/05/23 15:44:13, haraken wrote: > > > On 2014/05/23 15:30:30, sof wrote: > > > > > > > > > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... > > > > File Source/core/frame/DOMWindow.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/120001/Source/core/frame/DOMWi... > > > > Source/core/frame/DOMWindow.cpp:497: removeAllEventListeners(); > > > > On 2014/05/23 15:02:45, haraken wrote: > > > > > > > > > > Instead of moving only didClearTouchEventHandlers() into the weak > > processing > > > > of > > > > > Document::m_domWindow, how about just moving the entire > > > > > removeAllEventListeners() into the weak processing? > > > > > > > > > > Then you can completely comment out line 497 in oilpan builds and remove > > > line > > > > > 492 - 496. > > > > > > > > Hmm, don't understand what you are suggesting :) The DOMWindow specific > > parts > > > of > > > > removeAllEventListeners() needs to remain under DOMWindow's control, not > > > > Document and its weak processing. But perhaps you mean something else? > > > > > > I'm sorry I was confused. You're right. > > > > > > However, it looks tricky that ~DOMWindow() calls removeAllEventListeners > after > > > setting m_document to 0. I think we should completely remove > > > removeAllEventListeners from ~DOMWindow() at some point, so shall we just > > write > > > like this? > > > > > > ~DOMWindow() { > > > #if ENABLE(OILPAN) > > > // FIXME: Oilpan: Should be removed once EventTarget is moved on the > heap. > > > EventTarget::removeAllEventListeners(); > > > lifecycleNotifier().notifyRemoveAllEventListeners(this); > > > if (m_frame && m_frame->host()) > > > > > > m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers(*this); > > > // FIXME: Oilpan: Should be removed once we make the event map weak. > > > removeAllUnloadEventListeners(this); > > > removeAllBeforeUnloadEventListeners(this); > > > #else > > > removeAllEventListeners(); > > > #endif > > > } > > > > It's too much duplication of delicate code for my liking; we could add an > > "internal" version of removeAllEventListeners that takes a flag, but it would > > add a method slot for one single use. > > Either is fine with me, but I think we should remove > m_frame->host()->eventHandlerRegistry().didRemoveAllEventHandlers() in this CL, > and it's easy to remove removeAllUnloadEventListeners(this) and > removeAllBeforeUnloadEventListeners(this) in a follow-up. Tried to address; does that capture it? Well observed that host() and its event registry are also GCed objects; I wonder if we've just been skating on thin ice here previously.
Thanks for the update! The last remaining question to me is m_document->detach(). https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (left): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:495: ASSERT(m_document->isDisposed()); Just help me understand: Why do we need to remove this ASSERT? https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:348: m_document->detach(); I'm not yet fully convinced about this one. Would you mind again elaborating on why we don't need to call detach() when the DOMWindow dies but the Document is still alive? If we really wants to keep the current behavior as is, I think we need to move the detach() call into the weak processing for Document::m_domWindow. https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.h (right): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.h:346: enum BroadcastListenerRemoval { Shall we add a FIXME to remove this? Once we move EventTarget to the heap and make DOMWindowSet a hash set of weak members, the contents of removeAllEventListeners(DoNotBroadcastListenerRemoval) should become just one line: lifecycleNotifier().notifyRemoveAllEventListeners(this); and thus we can just write the line in ~DOMWindow(). https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.h:351: void removeAllEventListeners(BroadcastListenerRemoval); It's a bit confusing to have the overridden version of removeAllEventListeners() and the not-overridden version of removeAllEventListeners(). Shall we rename this to removeAllEventListenersInternal()?
https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:348: m_document->detach(); On 2014/05/23 20:53:58, haraken wrote: > > I'm not yet fully convinced about this one. Would you mind again elaborating on > why we don't need to call detach() when the DOMWindow dies but the Document is > still alive? > > If we really wants to keep the current behavior as is, I think we need to move > the detach() call into the weak processing for Document::m_domWindow. I don't have a better answer than the one given earlier today. Will have to sleep on it.
> Well observed that host() and its event registry are also GCed objects; I wonder > if we've just been skating on thin ice here previously. Yes, I guess so. The reason we're not hitting an ASan error is just that the DOMWindow and the FrameHost don't die in the same GC cycle by accident.
https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (left): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:495: ASSERT(m_document->isDisposed()); On 2014/05/23 20:53:58, haraken wrote: > > Just help me understand: Why do we need to remove this ASSERT? It was safe with m_document being a RefPtr<>, no longer so with it being a Member. Add an assert to the DOMWindow weak processing on Document instead? https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:348: m_document->detach(); On 2014/05/23 20:53:58, haraken wrote: > > I'm not yet fully convinced about this one. Would you mind again elaborating on > why we don't need to call detach() when the DOMWindow dies but the Document is > still alive? Consider the following steps: - A DOMWindow can at the earliest be destructed once a (Local)Frame lets go of the window. (- A subsequent v8 GC could then finalize and destruct the DOMWindow, assuming nothing else keeping the window alive.) - A LocalFrame will only let go of the window in its destructor. - The LocalFrame destructor will prior to that clear out the 'view', invoking setView(nullptr). - Clearing that view will invoke Document::prepareForDestruction(). - prepareForDestruction() detach()es the document. => By the time the DOMWindow is a candidate for destruction, the document will already have been detached. - When the DOMWindow's dtor does get to run, the window's detached document (should it be accessible somehow with Oilpan enabled) will be in a Stopped state. Hence, the conditional detach() in DOMWindow::clearDocument() can never apply. - As a result of that being a no-op, the entire call to clearDocument() is not needed for Oilpan. - Indeed, as Document::detach() already calls clearEventQueue() on its Window, the extra call we added to clearEventQueue() in ~DOMWindow for the Oilpan case is also redundant. Will remove. And if that detailed set of steps doesn't convince, the detached document condition is already asserted for in ~DOMWindow (cf. isStopped() / isDisposed().) > > If we really wants to keep the current behavior as is, I think we need to move > the detach() call into the weak processing for Document::m_domWindow. As detailed above, not needed.
https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (left): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:495: ASSERT(m_document->isDisposed()); On 2014/05/24 05:51:00, sof wrote: > On 2014/05/23 20:53:58, haraken wrote: > > > > Just help me understand: Why do we need to remove this ASSERT? > > It was safe with m_document being a RefPtr<>, no longer so with it being a > Member. > > Add an assert to the DOMWindow weak processing on Document instead? Done. https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.h (right): https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.h:346: enum BroadcastListenerRemoval { On 2014/05/23 20:53:58, haraken wrote: > > Shall we add a FIXME to remove this? > > Once we move EventTarget to the heap and make DOMWindowSet a hash set of weak > members, the contents of removeAllEventListeners(DoNotBroadcastListenerRemoval) > should become just one line: > > lifecycleNotifier().notifyRemoveAllEventListeners(this); > > and thus we can just write the line in ~DOMWindow(). Done. https://codereview.chromium.org/292503006/diff/140001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.h:351: void removeAllEventListeners(BroadcastListenerRemoval); On 2014/05/23 20:53:58, haraken wrote: > > It's a bit confusing to have the overridden version of removeAllEventListeners() > and the not-overridden version of removeAllEventListeners(). Shall we rename > this to removeAllEventListenersInternal()? Done.
https://codereview.chromium.org/292503006/diff/170001/Source/core/frame/DOMWi... File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/292503006/diff/170001/Source/core/frame/DOMWi... Source/core/frame/DOMWindow.cpp:499: ASSERT(!m_eventQueue); This assert showed up a unit test not simulating document detachment properly. Fixed/adjusted the Source/web/ test.
Thanks for the detailed explanation. Now I'm convinced of your reasoning about DOMWindow shutdown. LGTM, but I'd like to have one more reviewer about the lifetime of DOMWindow and Document.
On 2014/05/26 00:58:19, haraken wrote: > Thanks for the detailed explanation. Now I'm convinced of your reasoning about > DOMWindow shutdown. > > LGTM, but I'd like to have one more reviewer about the lifetime of DOMWindow and > Document. Thanks, there's a one line change in Source/web/ that needs approval also.
https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); This seems fishy to me. If the Document is dead and the DOMWindow is still alive they must have been explicitly disassociated. The Document must have been detached. When a document is detached it is no longer displayed and it seems surprising that we would need touch event handlers for something that is not even displayed? A Document cannot be reattached. Can't we clear the touch event handlers at that point instead of doing it via weak processing? Do we ever need the touch event handlers after a document has been explicitly detached? I would expect DOMWindow and Document to have strong pointers to each other as long as they are associated. When the document is detached from a DOMWindow we should perform cleanup. If it is never detached, the DOMWindow and the Document should die together and no cleanup is needed? https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> m_domWindow; I'm surprised that this isn't a strong pointer instead. A document corresponds to a DOMWindow until it is explicitly detached from that DOMWindow. When the explicit detach happens we should perform cleanup and disassociate the two. If the Document is never detached from the DOMWindow I would expect them to live and die together and no cleanup would have to happen.
On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > File Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > This seems fishy to me. If the Document is dead and the DOMWindow is still alive > they must have been explicitly disassociated. The Document must have been > detached. When a document is detached it is no longer displayed and it seems > surprising that we would need touch event handlers for something that is not > even displayed? A Document cannot be reattached. Can't we clear the touch event > handlers at that point instead of doing it via weak processing? Do we ever need > the touch event handlers after a document has been explicitly detached? > > I would expect DOMWindow and Document to have strong pointers to each other as > long as they are associated. When the document is detached from a DOMWindow we > should perform cleanup. If it is never detached, the DOMWindow and the Document > should die together and no cleanup is needed? > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > File Source/core/dom/Document.h (right): > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> m_domWindow; > I'm surprised that this isn't a strong pointer instead. A document corresponds > to a DOMWindow until it is explicitly detached from that DOMWindow. When the > explicit detach happens we should perform cleanup and disassociate the two. If > the Document is never detached from the DOMWindow I would expect them to live > and die together and no cleanup would have to happen. I'm not sure I understand your point, but I think Document::m_domWindow should be weak because the Document can outlive the DOMWindow. // main.html <html> <body> <iframe src="iframe.html"></iframe> <script> var iframe = document.querySelector("iframe"); iframe.onload = function () { var doc = iframe.contentDocument; console.log(doc.defaultView); // This should be a Window. document.body.innerHTML = ""; console.log(doc.defaultView); // This should be null. } </script> </body> </html>
On 2014/05/26 07:33:49, haraken wrote: > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > File Source/core/dom/Document.cpp (right): > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > This seems fishy to me. If the Document is dead and the DOMWindow is still > alive > > they must have been explicitly disassociated. The Document must have been > > detached. When a document is detached it is no longer displayed and it seems > > surprising that we would need touch event handlers for something that is not > > even displayed? A Document cannot be reattached. Can't we clear the touch > event > > handlers at that point instead of doing it via weak processing? Do we ever > need > > the touch event handlers after a document has been explicitly detached? > > > > I would expect DOMWindow and Document to have strong pointers to each other as > > long as they are associated. When the document is detached from a DOMWindow we > > should perform cleanup. If it is never detached, the DOMWindow and the > Document > > should die together and no cleanup is needed? > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > File Source/core/dom/Document.h (right): > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > m_domWindow; > > I'm surprised that this isn't a strong pointer instead. A document corresponds > > to a DOMWindow until it is explicitly detached from that DOMWindow. When the > > explicit detach happens we should perform cleanup and disassociate the two. If > > the Document is never detached from the DOMWindow I would expect them to live > > and die together and no cleanup would have to happen. > > I'm not sure I understand your point, but I think Document::m_domWindow should > be weak because the Document can outlive the DOMWindow. > > // main.html > <html> > <body> > <iframe src="iframe.html"></iframe> > <script> > var iframe = document.querySelector("iframe"); > iframe.onload = function () { > var doc = iframe.contentDocument; > console.log(doc.defaultView); // This should be a Window. > document.body.innerHTML = ""; > console.log(doc.defaultView); // This should be null. > } > </script> > </body> > </html> Yes, of course it can but that doesn't mean that we should use a weak pointer. When you do this the Document is explicitly detached from the DOMWindow. At that point you perform cleanup and clear out the pointers in both directions. There is no need for weak processing to allow things to die independently when they have been explicitly disconnected. *While* the DOMWindow and Document are connected they should keep eachother alive. When the Document is detached the strong pointers are cleared out and therefore the objects no longer keep eachother alive. No need for weakness for that.
On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > On 2014/05/26 07:33:49, haraken wrote: > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > This seems fishy to me. If the Document is dead and the DOMWindow is still > > alive > > > they must have been explicitly disassociated. The Document must have been > > > detached. When a document is detached it is no longer displayed and it seems > > > surprising that we would need touch event handlers for something that is not > > > even displayed? A Document cannot be reattached. Can't we clear the touch > > event > > > handlers at that point instead of doing it via weak processing? Do we ever > > need > > > the touch event handlers after a document has been explicitly detached? > > > > > > I would expect DOMWindow and Document to have strong pointers to each other > as > > > long as they are associated. When the document is detached from a DOMWindow > we > > > should perform cleanup. If it is never detached, the DOMWindow and the > > Document > > > should die together and no cleanup is needed? > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > File Source/core/dom/Document.h (right): > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > m_domWindow; > > > I'm surprised that this isn't a strong pointer instead. A document > corresponds > > > to a DOMWindow until it is explicitly detached from that DOMWindow. When the > > > explicit detach happens we should perform cleanup and disassociate the two. > If > > > the Document is never detached from the DOMWindow I would expect them to > live > > > and die together and no cleanup would have to happen. > > > > I'm not sure I understand your point, but I think Document::m_domWindow should > > be weak because the Document can outlive the DOMWindow. > > > > // main.html > > <html> > > <body> > > <iframe src="iframe.html"></iframe> > > <script> > > var iframe = document.querySelector("iframe"); > > iframe.onload = function () { > > var doc = iframe.contentDocument; > > console.log(doc.defaultView); // This should be a Window. > > document.body.innerHTML = ""; > > console.log(doc.defaultView); // This should be null. > > } > > </script> > > </body> > > </html> > > Yes, of course it can but that doesn't mean that we should use a weak pointer. > When you do this the Document is explicitly detached from the DOMWindow. At that > point you perform cleanup and clear out the pointers in both directions. There > is no need for weak processing to allow things to die independently when they > have been explicitly disconnected. *While* the DOMWindow and Document are > connected they should keep eachother alive. When the Document is detached the > strong pointers are cleared out and therefore the objects no longer keep > eachother alive. No need for weakness for that. There is need for weakness to observe when the other (DOMWindow) object falls away, so as to be able to instead finalize parts of Document that was previously done in ~DOMWindow() (via removeAllEventListeners().) This is the reason for introducing it here, I think.
On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > On 2014/05/26 07:33:49, haraken wrote: > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > This seems fishy to me. If the Document is dead and the DOMWindow is still > > alive > > > they must have been explicitly disassociated. The Document must have been > > > detached. When a document is detached it is no longer displayed and it seems > > > surprising that we would need touch event handlers for something that is not > > > even displayed? A Document cannot be reattached. Can't we clear the touch > > event > > > handlers at that point instead of doing it via weak processing? Do we ever > > need > > > the touch event handlers after a document has been explicitly detached? > > > > > > I would expect DOMWindow and Document to have strong pointers to each other > as > > > long as they are associated. When the document is detached from a DOMWindow > we > > > should perform cleanup. If it is never detached, the DOMWindow and the > > Document > > > should die together and no cleanup is needed? > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > File Source/core/dom/Document.h (right): > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > m_domWindow; > > > I'm surprised that this isn't a strong pointer instead. A document > corresponds > > > to a DOMWindow until it is explicitly detached from that DOMWindow. When the > > > explicit detach happens we should perform cleanup and disassociate the two. > If > > > the Document is never detached from the DOMWindow I would expect them to > live > > > and die together and no cleanup would have to happen. > > > > I'm not sure I understand your point, but I think Document::m_domWindow should > > be weak because the Document can outlive the DOMWindow. > > > > // main.html > > <html> > > <body> > > <iframe src="iframe.html"></iframe> > > <script> > > var iframe = document.querySelector("iframe"); > > iframe.onload = function () { > > var doc = iframe.contentDocument; > > console.log(doc.defaultView); // This should be a Window. > > document.body.innerHTML = ""; > > console.log(doc.defaultView); // This should be null. > > } > > </script> > > </body> > > </html> > > Yes, of course it can but that doesn't mean that we should use a weak pointer. > When you do this the Document is explicitly detached from the DOMWindow. At that > point you perform cleanup and clear out the pointers in both directions. There > is no need for weak processing to allow things to die independently when they > have been explicitly disconnected. *While* the DOMWindow and Document are > connected they should keep eachother alive. When the Document is detached the > strong pointers are cleared out and therefore the objects no longer keep > eachother alive. No need for weakness for that. Then, in the above example, how can the DOMWindow know the timing when the DOMWindow should detach the Document? The DOMWindow needs to detach the Document when the iframe is destructed (and thus the DOMWindow associated with the iframe is destructed). If the Document has a strong member to the DOMWindow, ~DOMWindow is not called when the iframe is destructed and thus the DOMWindow wouldn't be able to understand the timing when it should detach the Document.
On 2014/05/26 07:48:59, haraken wrote: > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > On 2014/05/26 07:33:49, haraken wrote: > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > This seems fishy to me. If the Document is dead and the DOMWindow is still > > > alive > > > > they must have been explicitly disassociated. The Document must have been > > > > detached. When a document is detached it is no longer displayed and it > seems > > > > surprising that we would need touch event handlers for something that is > not > > > > even displayed? A Document cannot be reattached. Can't we clear the touch > > > event > > > > handlers at that point instead of doing it via weak processing? Do we ever > > > need > > > > the touch event handlers after a document has been explicitly detached? > > > > > > > > I would expect DOMWindow and Document to have strong pointers to each > other > > as > > > > long as they are associated. When the document is detached from a > DOMWindow > > we > > > > should perform cleanup. If it is never detached, the DOMWindow and the > > > Document > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > m_domWindow; > > > > I'm surprised that this isn't a strong pointer instead. A document > > corresponds > > > > to a DOMWindow until it is explicitly detached from that DOMWindow. When > the > > > > explicit detach happens we should perform cleanup and disassociate the > two. > > If > > > > the Document is never detached from the DOMWindow I would expect them to > > live > > > > and die together and no cleanup would have to happen. > > > > > > I'm not sure I understand your point, but I think Document::m_domWindow > should > > > be weak because the Document can outlive the DOMWindow. > > > > > > // main.html > > > <html> > > > <body> > > > <iframe src="iframe.html"></iframe> > > > <script> > > > var iframe = document.querySelector("iframe"); > > > iframe.onload = function () { > > > var doc = iframe.contentDocument; > > > console.log(doc.defaultView); // This should be a Window. > > > document.body.innerHTML = ""; > > > console.log(doc.defaultView); // This should be null. > > > } > > > </script> > > > </body> > > > </html> > > > > Yes, of course it can but that doesn't mean that we should use a weak pointer. > > When you do this the Document is explicitly detached from the DOMWindow. At > that > > point you perform cleanup and clear out the pointers in both directions. There > > is no need for weak processing to allow things to die independently when they > > have been explicitly disconnected. *While* the DOMWindow and Document are > > connected they should keep eachother alive. When the Document is detached the > > strong pointers are cleared out and therefore the objects no longer keep > > eachother alive. No need for weakness for that. > > Then, in the above example, how can the DOMWindow know the timing when the > DOMWindow should detach the Document? The DOMWindow needs to detach the Document > when the iframe is destructed (and thus the DOMWindow associated with the iframe > is destructed). If the Document has a strong member to the DOMWindow, ~DOMWindow > is not called when the iframe is destructed and thus the DOMWindow wouldn't be > able to understand the timing when it should detach the Document. In that case, does it need to? The iframe, DOMWindow and for all I can see Document are all being torn down, so why do we need to do anything more except let the GC collect them once they are truly unreachable?
On 2014/05/26 07:48:59, haraken wrote: > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > On 2014/05/26 07:33:49, haraken wrote: > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > This seems fishy to me. If the Document is dead and the DOMWindow is still > > > alive > > > > they must have been explicitly disassociated. The Document must have been > > > > detached. When a document is detached it is no longer displayed and it > seems > > > > surprising that we would need touch event handlers for something that is > not > > > > even displayed? A Document cannot be reattached. Can't we clear the touch > > > event > > > > handlers at that point instead of doing it via weak processing? Do we ever > > > need > > > > the touch event handlers after a document has been explicitly detached? > > > > > > > > I would expect DOMWindow and Document to have strong pointers to each > other > > as > > > > long as they are associated. When the document is detached from a > DOMWindow > > we > > > > should perform cleanup. If it is never detached, the DOMWindow and the > > > Document > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > m_domWindow; > > > > I'm surprised that this isn't a strong pointer instead. A document > > corresponds > > > > to a DOMWindow until it is explicitly detached from that DOMWindow. When > the > > > > explicit detach happens we should perform cleanup and disassociate the > two. > > If > > > > the Document is never detached from the DOMWindow I would expect them to > > live > > > > and die together and no cleanup would have to happen. > > > > > > I'm not sure I understand your point, but I think Document::m_domWindow > should > > > be weak because the Document can outlive the DOMWindow. > > > > > > // main.html > > > <html> > > > <body> > > > <iframe src="iframe.html"></iframe> > > > <script> > > > var iframe = document.querySelector("iframe"); > > > iframe.onload = function () { > > > var doc = iframe.contentDocument; > > > console.log(doc.defaultView); // This should be a Window. > > > document.body.innerHTML = ""; > > > console.log(doc.defaultView); // This should be null. > > > } > > > </script> > > > </body> > > > </html> > > > > Yes, of course it can but that doesn't mean that we should use a weak pointer. > > When you do this the Document is explicitly detached from the DOMWindow. At > that > > point you perform cleanup and clear out the pointers in both directions. There > > is no need for weak processing to allow things to die independently when they > > have been explicitly disconnected. *While* the DOMWindow and Document are > > connected they should keep eachother alive. When the Document is detached the > > strong pointers are cleared out and therefore the objects no longer keep > > eachother alive. No need for weakness for that. > > Then, in the above example, how can the DOMWindow know the timing when the > DOMWindow should detach the Document? The DOMWindow needs to detach the Document > when the iframe is destructed (and thus the DOMWindow associated with the iframe > is destructed). If the Document has a strong member to the DOMWindow, ~DOMWindow > is not called when the iframe is destructed and thus the DOMWindow wouldn't be > able to understand the timing when it should detach the Document. How would weak processing help? If you make this rely on weak processing then sometimes it will return the DOMWindow (before a GC happens) and sometimes it will return null. I don't think the above example should rely on whether or not you get a GC. Therefore, we should attempt to get notifications propagated through the system when things are being detached. In this case you are setting innerHTML and that apparently leads to defaultView becoming null. That should happen independently of GC with and without Oilpan if that is the desired behavior. Using weak processing for it means that we need to have a GC for that pointer to become null which does not seem right.
On 2014/05/26 07:44:39, sof wrote: > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > On 2014/05/26 07:33:49, haraken wrote: > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > This seems fishy to me. If the Document is dead and the DOMWindow is still > > > alive > > > > they must have been explicitly disassociated. The Document must have been > > > > detached. When a document is detached it is no longer displayed and it > seems > > > > surprising that we would need touch event handlers for something that is > not > > > > even displayed? A Document cannot be reattached. Can't we clear the touch > > > event > > > > handlers at that point instead of doing it via weak processing? Do we ever > > > need > > > > the touch event handlers after a document has been explicitly detached? > > > > > > > > I would expect DOMWindow and Document to have strong pointers to each > other > > as > > > > long as they are associated. When the document is detached from a > DOMWindow > > we > > > > should perform cleanup. If it is never detached, the DOMWindow and the > > > Document > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > m_domWindow; > > > > I'm surprised that this isn't a strong pointer instead. A document > > corresponds > > > > to a DOMWindow until it is explicitly detached from that DOMWindow. When > the > > > > explicit detach happens we should perform cleanup and disassociate the > two. > > If > > > > the Document is never detached from the DOMWindow I would expect them to > > live > > > > and die together and no cleanup would have to happen. > > > > > > I'm not sure I understand your point, but I think Document::m_domWindow > should > > > be weak because the Document can outlive the DOMWindow. > > > > > > // main.html > > > <html> > > > <body> > > > <iframe src="iframe.html"></iframe> > > > <script> > > > var iframe = document.querySelector("iframe"); > > > iframe.onload = function () { > > > var doc = iframe.contentDocument; > > > console.log(doc.defaultView); // This should be a Window. > > > document.body.innerHTML = ""; > > > console.log(doc.defaultView); // This should be null. > > > } > > > </script> > > > </body> > > > </html> > > > > Yes, of course it can but that doesn't mean that we should use a weak pointer. > > When you do this the Document is explicitly detached from the DOMWindow. At > that > > point you perform cleanup and clear out the pointers in both directions. There > > is no need for weak processing to allow things to die independently when they > > have been explicitly disconnected. *While* the DOMWindow and Document are > > connected they should keep eachother alive. When the Document is detached the > > strong pointers are cleared out and therefore the objects no longer keep > > eachother alive. No need for weakness for that. > > There is need for weakness to observe when the other (DOMWindow) object falls > away, so as to be able to instead finalize parts of Document that was previously > done in ~DOMWindow() (via removeAllEventListeners().) This is the reason for > introducing it here, I think. My question is if we have to do that at all? Either the DOMWindow and the Document die together or the Document has been detached from the DOMWindow (and you even have that assert in the Document weak processing). Can we perform the cleanup of touch event listeners at detach instead of in weak processing? If the Document is not detached from the DOMWindow it seems to me that they should just die together and we shouldn't have to perform cleanup related to DOMWindow in the Document?
On 2014/05/26 08:00:25, Mads Ager (chromium) wrote: > On 2014/05/26 07:44:39, sof wrote: > > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > > On 2014/05/26 07:33:49, haraken wrote: > > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > > This seems fishy to me. If the Document is dead and the DOMWindow is > still > > > > alive > > > > > they must have been explicitly disassociated. The Document must have > been > > > > > detached. When a document is detached it is no longer displayed and it > > seems > > > > > surprising that we would need touch event handlers for something that is > > not > > > > > even displayed? A Document cannot be reattached. Can't we clear the > touch > > > > event > > > > > handlers at that point instead of doing it via weak processing? Do we > ever > > > > need > > > > > the touch event handlers after a document has been explicitly detached? > > > > > > > > > > I would expect DOMWindow and Document to have strong pointers to each > > other > > > as > > > > > long as they are associated. When the document is detached from a > > DOMWindow > > > we > > > > > should perform cleanup. If it is never detached, the DOMWindow and the > > > > Document > > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > > m_domWindow; > > > > > I'm surprised that this isn't a strong pointer instead. A document > > > corresponds > > > > > to a DOMWindow until it is explicitly detached from that DOMWindow. When > > the > > > > > explicit detach happens we should perform cleanup and disassociate the > > two. > > > If > > > > > the Document is never detached from the DOMWindow I would expect them to > > > live > > > > > and die together and no cleanup would have to happen. > > > > > > > > I'm not sure I understand your point, but I think Document::m_domWindow > > should > > > > be weak because the Document can outlive the DOMWindow. > > > > > > > > // main.html > > > > <html> > > > > <body> > > > > <iframe src="iframe.html"></iframe> > > > > <script> > > > > var iframe = document.querySelector("iframe"); > > > > iframe.onload = function () { > > > > var doc = iframe.contentDocument; > > > > console.log(doc.defaultView); // This should be a Window. > > > > document.body.innerHTML = ""; > > > > console.log(doc.defaultView); // This should be null. > > > > } > > > > </script> > > > > </body> > > > > </html> > > > > > > Yes, of course it can but that doesn't mean that we should use a weak > pointer. > > > When you do this the Document is explicitly detached from the DOMWindow. At > > that > > > point you perform cleanup and clear out the pointers in both directions. > There > > > is no need for weak processing to allow things to die independently when > they > > > have been explicitly disconnected. *While* the DOMWindow and Document are > > > connected they should keep eachother alive. When the Document is detached > the > > > strong pointers are cleared out and therefore the objects no longer keep > > > eachother alive. No need for weakness for that. > > > > There is need for weakness to observe when the other (DOMWindow) object falls > > away, so as to be able to instead finalize parts of Document that was > previously > > done in ~DOMWindow() (via removeAllEventListeners().) This is the reason for > > introducing it here, I think. > > My question is if we have to do that at all? Either the DOMWindow and the > Document die together or the Document has been detached from the DOMWindow (and > you even have that assert in the Document weak processing). Can we perform the > cleanup of touch event listeners at detach instead of in weak processing? If the > Document is not detached from the DOMWindow it seems to me that they should just > die together and we shouldn't have to perform cleanup related to DOMWindow in > the Document? I'm loath to change the timing of when this happens, but will give it a try to move "up" touch unregistration to detach.
On 2014/05/26 08:02:58, sof wrote: > On 2014/05/26 08:00:25, Mads Ager (chromium) wrote: > > On 2014/05/26 07:44:39, sof wrote: > > > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > > > On 2014/05/26 07:33:49, haraken wrote: > > > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > > > This seems fishy to me. If the Document is dead and the DOMWindow is > > still > > > > > alive > > > > > > they must have been explicitly disassociated. The Document must have > > been > > > > > > detached. When a document is detached it is no longer displayed and it > > > seems > > > > > > surprising that we would need touch event handlers for something that > is > > > not > > > > > > even displayed? A Document cannot be reattached. Can't we clear the > > touch > > > > > event > > > > > > handlers at that point instead of doing it via weak processing? Do we > > ever > > > > > need > > > > > > the touch event handlers after a document has been explicitly > detached? > > > > > > > > > > > > I would expect DOMWindow and Document to have strong pointers to each > > > other > > > > as > > > > > > long as they are associated. When the document is detached from a > > > DOMWindow > > > > we > > > > > > should perform cleanup. If it is never detached, the DOMWindow and the > > > > > Document > > > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > > > m_domWindow; > > > > > > I'm surprised that this isn't a strong pointer instead. A document > > > > corresponds > > > > > > to a DOMWindow until it is explicitly detached from that DOMWindow. > When > > > the > > > > > > explicit detach happens we should perform cleanup and disassociate the > > > two. > > > > If > > > > > > the Document is never detached from the DOMWindow I would expect them > to > > > > live > > > > > > and die together and no cleanup would have to happen. > > > > > > > > > > I'm not sure I understand your point, but I think Document::m_domWindow > > > should > > > > > be weak because the Document can outlive the DOMWindow. > > > > > > > > > > // main.html > > > > > <html> > > > > > <body> > > > > > <iframe src="iframe.html"></iframe> > > > > > <script> > > > > > var iframe = document.querySelector("iframe"); > > > > > iframe.onload = function () { > > > > > var doc = iframe.contentDocument; > > > > > console.log(doc.defaultView); // This should be a Window. > > > > > document.body.innerHTML = ""; > > > > > console.log(doc.defaultView); // This should be null. > > > > > } > > > > > </script> > > > > > </body> > > > > > </html> > > > > > > > > Yes, of course it can but that doesn't mean that we should use a weak > > pointer. > > > > When you do this the Document is explicitly detached from the DOMWindow. > At > > > that > > > > point you perform cleanup and clear out the pointers in both directions. > > There > > > > is no need for weak processing to allow things to die independently when > > they > > > > have been explicitly disconnected. *While* the DOMWindow and Document are > > > > connected they should keep eachother alive. When the Document is detached > > the > > > > strong pointers are cleared out and therefore the objects no longer keep > > > > eachother alive. No need for weakness for that. > > > > > > There is need for weakness to observe when the other (DOMWindow) object > falls > > > away, so as to be able to instead finalize parts of Document that was > > previously > > > done in ~DOMWindow() (via removeAllEventListeners().) This is the reason for > > > introducing it here, I think. > > > > My question is if we have to do that at all? Either the DOMWindow and the > > Document die together or the Document has been detached from the DOMWindow > (and > > you even have that assert in the Document weak processing). Can we perform the > > cleanup of touch event listeners at detach instead of in weak processing? If > the > > Document is not detached from the DOMWindow it seems to me that they should > just > > die together and we shouldn't have to perform cleanup related to DOMWindow in > > the Document? > > I'm loath to change the timing of when this happens, but will give it a try to > move "up" touch unregistration to detach. It does change the timing. However, using weak processing changes the timing as well. The timing with weak processing is not predictable at all because it happens when we happen to get a GC. If we can make the timing predictable I think that would be a win. The question is if there is any reason why touch event listeners should stay on a Document after detach? I can't come up with one, but there might be something hiding there that we don't know about.
On 2014/05/26 07:57:48, Mads Ager (chromium) wrote: > On 2014/05/26 07:48:59, haraken wrote: > > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > > On 2014/05/26 07:33:49, haraken wrote: > > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > > This seems fishy to me. If the Document is dead and the DOMWindow is > still > > > > alive > > > > > they must have been explicitly disassociated. The Document must have > been > > > > > detached. When a document is detached it is no longer displayed and it > > seems > > > > > surprising that we would need touch event handlers for something that is > > not > > > > > even displayed? A Document cannot be reattached. Can't we clear the > touch > > > > event > > > > > handlers at that point instead of doing it via weak processing? Do we > ever > > > > need > > > > > the touch event handlers after a document has been explicitly detached? > > > > > > > > > > I would expect DOMWindow and Document to have strong pointers to each > > other > > > as > > > > > long as they are associated. When the document is detached from a > > DOMWindow > > > we > > > > > should perform cleanup. If it is never detached, the DOMWindow and the > > > > Document > > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > > m_domWindow; > > > > > I'm surprised that this isn't a strong pointer instead. A document > > > corresponds > > > > > to a DOMWindow until it is explicitly detached from that DOMWindow. When > > the > > > > > explicit detach happens we should perform cleanup and disassociate the > > two. > > > If > > > > > the Document is never detached from the DOMWindow I would expect them to > > > live > > > > > and die together and no cleanup would have to happen. > > > > > > > > I'm not sure I understand your point, but I think Document::m_domWindow > > should > > > > be weak because the Document can outlive the DOMWindow. > > > > > > > > // main.html > > > > <html> > > > > <body> > > > > <iframe src="iframe.html"></iframe> > > > > <script> > > > > var iframe = document.querySelector("iframe"); > > > > iframe.onload = function () { > > > > var doc = iframe.contentDocument; > > > > console.log(doc.defaultView); // This should be a Window. > > > > document.body.innerHTML = ""; > > > > console.log(doc.defaultView); // This should be null. > > > > } > > > > </script> > > > > </body> > > > > </html> > > > > > > Yes, of course it can but that doesn't mean that we should use a weak > pointer. > > > When you do this the Document is explicitly detached from the DOMWindow. At > > that > > > point you perform cleanup and clear out the pointers in both directions. > There > > > is no need for weak processing to allow things to die independently when > they > > > have been explicitly disconnected. *While* the DOMWindow and Document are > > > connected they should keep eachother alive. When the Document is detached > the > > > strong pointers are cleared out and therefore the objects no longer keep > > > eachother alive. No need for weakness for that. > > > > Then, in the above example, how can the DOMWindow know the timing when the > > DOMWindow should detach the Document? The DOMWindow needs to detach the > Document > > when the iframe is destructed (and thus the DOMWindow associated with the > iframe > > is destructed). If the Document has a strong member to the DOMWindow, > ~DOMWindow > > is not called when the iframe is destructed and thus the DOMWindow wouldn't be > > able to understand the timing when it should detach the Document. > > How would weak processing help? If you make this rely on weak processing then > sometimes it will return the DOMWindow (before a GC happens) and sometimes it > will return null. I don't think the above example should rely on whether or not > you get a GC. Therefore, we should attempt to get notifications propagated > through the system when things are being detached. In this case you are setting > innerHTML and that apparently leads to defaultView becoming null. That should > happen independently of GC with and without Oilpan if that is the desired > behavior. Using weak processing for it means that we need to have a GC for that > pointer to become null which does not seem right. I just patched in Sigbjorns change. Removed the weak processing and made the pointer to DOMWindow strong. Haraken, your example still return a DOMWindow first and then 'undefined'. That is the same behavior as pre-oilpan and it does not rely on weak processing as expected.
On 2014/05/26 08:23:06, Mads Ager (chromium) wrote: > On 2014/05/26 07:57:48, Mads Ager (chromium) wrote: > > On 2014/05/26 07:48:59, haraken wrote: > > > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > > > On 2014/05/26 07:33:49, haraken wrote: > > > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > > > This seems fishy to me. If the Document is dead and the DOMWindow is > > still > > > > > alive > > > > > > they must have been explicitly disassociated. The Document must have > > been > > > > > > detached. When a document is detached it is no longer displayed and it > > > seems > > > > > > surprising that we would need touch event handlers for something that > is > > > not > > > > > > even displayed? A Document cannot be reattached. Can't we clear the > > touch > > > > > event > > > > > > handlers at that point instead of doing it via weak processing? Do we > > ever > > > > > need > > > > > > the touch event handlers after a document has been explicitly > detached? > > > > > > > > > > > > I would expect DOMWindow and Document to have strong pointers to each > > > other > > > > as > > > > > > long as they are associated. When the document is detached from a > > > DOMWindow > > > > we > > > > > > should perform cleanup. If it is never detached, the DOMWindow and the > > > > > Document > > > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > > > m_domWindow; > > > > > > I'm surprised that this isn't a strong pointer instead. A document > > > > corresponds > > > > > > to a DOMWindow until it is explicitly detached from that DOMWindow. > When > > > the > > > > > > explicit detach happens we should perform cleanup and disassociate the > > > two. > > > > If > > > > > > the Document is never detached from the DOMWindow I would expect them > to > > > > live > > > > > > and die together and no cleanup would have to happen. > > > > > > > > > > I'm not sure I understand your point, but I think Document::m_domWindow > > > should > > > > > be weak because the Document can outlive the DOMWindow. > > > > > > > > > > // main.html > > > > > <html> > > > > > <body> > > > > > <iframe src="iframe.html"></iframe> > > > > > <script> > > > > > var iframe = document.querySelector("iframe"); > > > > > iframe.onload = function () { > > > > > var doc = iframe.contentDocument; > > > > > console.log(doc.defaultView); // This should be a Window. > > > > > document.body.innerHTML = ""; > > > > > console.log(doc.defaultView); // This should be null. > > > > > } > > > > > </script> > > > > > </body> > > > > > </html> > > > > > > > > Yes, of course it can but that doesn't mean that we should use a weak > > pointer. > > > > When you do this the Document is explicitly detached from the DOMWindow. > At > > > that > > > > point you perform cleanup and clear out the pointers in both directions. > > There > > > > is no need for weak processing to allow things to die independently when > > they > > > > have been explicitly disconnected. *While* the DOMWindow and Document are > > > > connected they should keep eachother alive. When the Document is detached > > the > > > > strong pointers are cleared out and therefore the objects no longer keep > > > > eachother alive. No need for weakness for that. > > > > > > Then, in the above example, how can the DOMWindow know the timing when the > > > DOMWindow should detach the Document? The DOMWindow needs to detach the > > Document > > > when the iframe is destructed (and thus the DOMWindow associated with the > > iframe > > > is destructed). If the Document has a strong member to the DOMWindow, > > ~DOMWindow > > > is not called when the iframe is destructed and thus the DOMWindow wouldn't > be > > > able to understand the timing when it should detach the Document. > > > > How would weak processing help? If you make this rely on weak processing then > > sometimes it will return the DOMWindow (before a GC happens) and sometimes it > > will return null. I don't think the above example should rely on whether or > not > > you get a GC. Therefore, we should attempt to get notifications propagated > > through the system when things are being detached. In this case you are > setting > > innerHTML and that apparently leads to defaultView becoming null. That should > > happen independently of GC with and without Oilpan if that is the desired > > behavior. Using weak processing for it means that we need to have a GC for > that > > pointer to become null which does not seem right. > > I just patched in Sigbjorns change. Removed the weak processing and made the > pointer to DOMWindow strong. Haraken, your example still return a DOMWindow > first and then 'undefined'. That is the same behavior as pre-oilpan and it does > not rely on weak processing as expected. Thanks, I agree with it. - The fact that Oilpan doesn't need to call Document::detach() in DOMWindow's destructor (as sof@ explained in comment #22) implies that Document::detach() is already called explicitly. - Then Document::m_domWindow doesn't need to be a weak member. That sounds reasonable to me :)
On 2014/05/26 08:09:57, Mads Ager (chromium) wrote: > On 2014/05/26 08:02:58, sof wrote: ... > > > > I'm loath to change the timing of when this happens, but will give it a try to > > move "up" touch unregistration to detach. > > It does change the timing. However, using weak processing changes the timing as > well. The timing with weak processing is not predictable at all because it > happens when we happen to get a GC. If we can make the timing predictable I > think that would be a win. The question is if there is any reason why touch > event listeners should stay on a Document after detach? I can't come up with > one, but there might be something hiding there that we don't know about. Not a full testrun, but no regressions seen when unregistering at detach.
On 2014/05/26 08:33:45, haraken wrote: > On 2014/05/26 08:23:06, Mads Ager (chromium) wrote: > > On 2014/05/26 07:57:48, Mads Ager (chromium) wrote: > > > On 2014/05/26 07:48:59, haraken wrote: > > > > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > > > > On 2014/05/26 07:33:49, haraken wrote: > > > > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > > > > This seems fishy to me. If the Document is dead and the DOMWindow is > > > still > > > > > > alive > > > > > > > they must have been explicitly disassociated. The Document must have > > > been > > > > > > > detached. When a document is detached it is no longer displayed and > it > > > > seems > > > > > > > surprising that we would need touch event handlers for something > that > > is > > > > not > > > > > > > even displayed? A Document cannot be reattached. Can't we clear the > > > touch > > > > > > event > > > > > > > handlers at that point instead of doing it via weak processing? Do > we > > > ever > > > > > > need > > > > > > > the touch event handlers after a document has been explicitly > > detached? > > > > > > > > > > > > > > I would expect DOMWindow and Document to have strong pointers to > each > > > > other > > > > > as > > > > > > > long as they are associated. When the document is detached from a > > > > DOMWindow > > > > > we > > > > > > > should perform cleanup. If it is never detached, the DOMWindow and > the > > > > > > Document > > > > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > > > > m_domWindow; > > > > > > > I'm surprised that this isn't a strong pointer instead. A document > > > > > corresponds > > > > > > > to a DOMWindow until it is explicitly detached from that DOMWindow. > > When > > > > the > > > > > > > explicit detach happens we should perform cleanup and disassociate > the > > > > two. > > > > > If > > > > > > > the Document is never detached from the DOMWindow I would expect > them > > to > > > > > live > > > > > > > and die together and no cleanup would have to happen. > > > > > > > > > > > > I'm not sure I understand your point, but I think > Document::m_domWindow > > > > should > > > > > > be weak because the Document can outlive the DOMWindow. > > > > > > > > > > > > // main.html > > > > > > <html> > > > > > > <body> > > > > > > <iframe src="iframe.html"></iframe> > > > > > > <script> > > > > > > var iframe = document.querySelector("iframe"); > > > > > > iframe.onload = function () { > > > > > > var doc = iframe.contentDocument; > > > > > > console.log(doc.defaultView); // This should be a Window. > > > > > > document.body.innerHTML = ""; > > > > > > console.log(doc.defaultView); // This should be null. > > > > > > } > > > > > > </script> > > > > > > </body> > > > > > > </html> > > > > > > > > > > Yes, of course it can but that doesn't mean that we should use a weak > > > pointer. > > > > > When you do this the Document is explicitly detached from the DOMWindow. > > At > > > > that > > > > > point you perform cleanup and clear out the pointers in both directions. > > > There > > > > > is no need for weak processing to allow things to die independently when > > > they > > > > > have been explicitly disconnected. *While* the DOMWindow and Document > are > > > > > connected they should keep eachother alive. When the Document is > detached > > > the > > > > > strong pointers are cleared out and therefore the objects no longer keep > > > > > eachother alive. No need for weakness for that. > > > > > > > > Then, in the above example, how can the DOMWindow know the timing when the > > > > DOMWindow should detach the Document? The DOMWindow needs to detach the > > > Document > > > > when the iframe is destructed (and thus the DOMWindow associated with the > > > iframe > > > > is destructed). If the Document has a strong member to the DOMWindow, > > > ~DOMWindow > > > > is not called when the iframe is destructed and thus the DOMWindow > wouldn't > > be > > > > able to understand the timing when it should detach the Document. > > > > > > How would weak processing help? If you make this rely on weak processing > then > > > sometimes it will return the DOMWindow (before a GC happens) and sometimes > it > > > will return null. I don't think the above example should rely on whether or > > not > > > you get a GC. Therefore, we should attempt to get notifications propagated > > > through the system when things are being detached. In this case you are > > setting > > > innerHTML and that apparently leads to defaultView becoming null. That > should > > > happen independently of GC with and without Oilpan if that is the desired > > > behavior. Using weak processing for it means that we need to have a GC for > > that > > > pointer to become null which does not seem right. > > > > I just patched in Sigbjorns change. Removed the weak processing and made the > > pointer to DOMWindow strong. Haraken, your example still return a DOMWindow > > first and then 'undefined'. That is the same behavior as pre-oilpan and it > does > > not rely on weak processing as expected. > > Thanks, I agree with it. > > - The fact that Oilpan doesn't need to call Document::detach() in DOMWindow's > destructor (as sof@ explained in comment #22) implies that Document::detach() is > already called explicitly. > > - Then Document::m_domWindow doesn't need to be a weak member. > > That sounds reasonable to me :) Updated to use a strong reference instead -- we (still) do want to clear out the window reference on detach..right?
On 2014/05/26 09:05:17, sof wrote: > On 2014/05/26 08:33:45, haraken wrote: > > On 2014/05/26 08:23:06, Mads Ager (chromium) wrote: > > > On 2014/05/26 07:57:48, Mads Ager (chromium) wrote: > > > > On 2014/05/26 07:48:59, haraken wrote: > > > > > On 2014/05/26 07:38:02, Mads Ager (chromium) wrote: > > > > > > On 2014/05/26 07:33:49, haraken wrote: > > > > > > > On 2014/05/26 06:15:19, Mads Ager (chromium) wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > > > Source/core/dom/Document.cpp:5749: ASSERT(isDisposed()); > > > > > > > > This seems fishy to me. If the Document is dead and the DOMWindow > is > > > > still > > > > > > > alive > > > > > > > > they must have been explicitly disassociated. The Document must > have > > > > been > > > > > > > > detached. When a document is detached it is no longer displayed > and > > it > > > > > seems > > > > > > > > surprising that we would need touch event handlers for something > > that > > > is > > > > > not > > > > > > > > even displayed? A Document cannot be reattached. Can't we clear > the > > > > touch > > > > > > > event > > > > > > > > handlers at that point instead of doing it via weak processing? Do > > we > > > > ever > > > > > > > need > > > > > > > > the touch event handlers after a document has been explicitly > > > detached? > > > > > > > > > > > > > > > > I would expect DOMWindow and Document to have strong pointers to > > each > > > > > other > > > > > > as > > > > > > > > long as they are associated. When the document is detached from a > > > > > DOMWindow > > > > > > we > > > > > > > > should perform cleanup. If it is never detached, the DOMWindow and > > the > > > > > > > Document > > > > > > > > should die together and no cleanup is needed? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Document.h > > > > > > > > File Source/core/dom/Document.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/292503006/diff/190001/Source/core/dom/Documen... > > > > > > > > Source/core/dom/Document.h:1195: RawPtrWillBeWeakMember<DOMWindow> > > > > > > > m_domWindow; > > > > > > > > I'm surprised that this isn't a strong pointer instead. A document > > > > > > corresponds > > > > > > > > to a DOMWindow until it is explicitly detached from that > DOMWindow. > > > When > > > > > the > > > > > > > > explicit detach happens we should perform cleanup and disassociate > > the > > > > > two. > > > > > > If > > > > > > > > the Document is never detached from the DOMWindow I would expect > > them > > > to > > > > > > live > > > > > > > > and die together and no cleanup would have to happen. > > > > > > > > > > > > > > I'm not sure I understand your point, but I think > > Document::m_domWindow > > > > > should > > > > > > > be weak because the Document can outlive the DOMWindow. > > > > > > > > > > > > > > // main.html > > > > > > > <html> > > > > > > > <body> > > > > > > > <iframe src="iframe.html"></iframe> > > > > > > > <script> > > > > > > > var iframe = document.querySelector("iframe"); > > > > > > > iframe.onload = function () { > > > > > > > var doc = iframe.contentDocument; > > > > > > > console.log(doc.defaultView); // This should be a Window. > > > > > > > document.body.innerHTML = ""; > > > > > > > console.log(doc.defaultView); // This should be null. > > > > > > > } > > > > > > > </script> > > > > > > > </body> > > > > > > > </html> > > > > > > > > > > > > Yes, of course it can but that doesn't mean that we should use a weak > > > > pointer. > > > > > > When you do this the Document is explicitly detached from the > DOMWindow. > > > At > > > > > that > > > > > > point you perform cleanup and clear out the pointers in both > directions. > > > > There > > > > > > is no need for weak processing to allow things to die independently > when > > > > they > > > > > > have been explicitly disconnected. *While* the DOMWindow and Document > > are > > > > > > connected they should keep eachother alive. When the Document is > > detached > > > > the > > > > > > strong pointers are cleared out and therefore the objects no longer > keep > > > > > > eachother alive. No need for weakness for that. > > > > > > > > > > Then, in the above example, how can the DOMWindow know the timing when > the > > > > > DOMWindow should detach the Document? The DOMWindow needs to detach the > > > > Document > > > > > when the iframe is destructed (and thus the DOMWindow associated with > the > > > > iframe > > > > > is destructed). If the Document has a strong member to the DOMWindow, > > > > ~DOMWindow > > > > > is not called when the iframe is destructed and thus the DOMWindow > > wouldn't > > > be > > > > > able to understand the timing when it should detach the Document. > > > > > > > > How would weak processing help? If you make this rely on weak processing > > then > > > > sometimes it will return the DOMWindow (before a GC happens) and sometimes > > it > > > > will return null. I don't think the above example should rely on whether > or > > > not > > > > you get a GC. Therefore, we should attempt to get notifications propagated > > > > through the system when things are being detached. In this case you are > > > setting > > > > innerHTML and that apparently leads to defaultView becoming null. That > > should > > > > happen independently of GC with and without Oilpan if that is the desired > > > > behavior. Using weak processing for it means that we need to have a GC for > > > that > > > > pointer to become null which does not seem right. > > > > > > I just patched in Sigbjorns change. Removed the weak processing and made the > > > pointer to DOMWindow strong. Haraken, your example still return a DOMWindow > > > first and then 'undefined'. That is the same behavior as pre-oilpan and it > > does > > > not rely on weak processing as expected. > > > > Thanks, I agree with it. > > > > - The fact that Oilpan doesn't need to call Document::detach() in DOMWindow's > > destructor (as sof@ explained in comment #22) implies that Document::detach() > is > > already called explicitly. > > > > - Then Document::m_domWindow doesn't need to be a weak member. > > > > That sounds reasonable to me :) > > Updated to use a strong reference instead -- we (still) do want to clear out the > window reference on detach..right? Yes, I think we should. A couple of methods in Document are relying on the fact that Document::m_domWindow becomes 0 promptly when the Document is detached from the DOMWindow. LGTM.
On 2014/05/26 09:08:55, haraken wrote: > On 2014/05/26 09:05:17, sof wrote: ...... > > > > Updated to use a strong reference instead -- we (still) do want to clear out > the > > window reference on detach..right? > > Yes, I think we should. A couple of methods in Document are relying on the fact > that Document::m_domWindow becomes 0 promptly when the Document is detached from > the DOMWindow. > > LGTM. tkent: could you review the Source/web/ test change?
LGTM Thanks Sigbjørn!
Source/web lgtm
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/292503006/230001
Message was sent while issue was closed.
Change committed as 174804
Message was sent while issue was closed.
On 2014/05/26 12:03:16, I haz the power (commit-bot) wrote: > Change committed as 174804 FYI (no action necessary): This change broke run-bindings-tests, due to changing a real IDL file that is used by test files. This is ok (we're not going to run r-b-t as PRESUBMIT for changes in core/), and yhirano@ will rebaseline test results, but something to be aware of generally. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
