Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1035803002:
Remove ResourceFetcher's access to a real FetchContext at frame detach time (Closed)
|
Created:
5 years, 9 months ago by Nate Chapin Modified:
5 years, 8 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, dglazkov+blink, eae+blinkwatch, gavinp+loader_chromium.org, oilpan-reviews, rwlbuis, tyoshino+watch_chromium.org, webcomponents-bugzilla_chromium.org, dcheng Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRemove ResourceFetcher's access to a real FetchContext at frame detach time
This allows us to remove a whole bunch of frame() null-checks in FrameFetchContext.
BUG=458222
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193208
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address coments #
Total comments: 6
Patch Set 3 : Fix crash #Patch Set 4 : Moved ASSERT(!m_importsController) to #if !ENABLE(OILPAN) block #
Messages
Total messages: 31 (5 generated)
japhet@chromium.org changed reviewers: + mkwst@chromium.org, sigbjornf@opera.com
This is yet another attempt to land https://codereview.chromium.org/1024263002/ The differences from https://codereview.chromium.org/1024263002/ are in Document::detach and LinkImport.cpp
https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... File Source/core/html/imports/LinkImport.cpp (right): https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... Source/core/html/imports/LinkImport.cpp:131: return m_owner && m_child && m_child->isDone() && !m_child->loader()->hasError(); Why doesn't this ASSERT hold true anymore?
https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... Source/core/dom/Document.cpp:618: if (m_importsController) The lack of this sync removal (and dispose) on the Oilpan side is leading to cases where we may have non-cancelled htmlimports loader objects attempting fetch their master's frames when loading notifications end up calling into its (still attached) FrameFetchContext. Which won't have a frame -- htmlimports/import-async-previous-async.html shows this up (if you've got leak detection enabled.) The invariant introduced by this CL, I think, is that no ResourceFetcher should have a FrameFetchContext attached once the frame of that FrameFetchContext (i.e., FrameFetchContext::frame()) is detached from its document. Correct? In which case I think detaching a frame from a document needs to do a bit more work when detaching the HTMLImportsController, in the Oilpan case. i.e., HTMLImportsController::removeFrom() sync'ly destructs the controller and everything below without Oilpan, it won't with. I need to think it over a bit more; the dependencies here aren't in short supply :-)
On 2015/03/26 08:23:30, sof wrote: > https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cpp > File Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... > Source/core/dom/Document.cpp:618: if (m_importsController) > The lack of this sync removal (and dispose) on the Oilpan side is leading to > cases where we may have non-cancelled htmlimports loader objects attempting > fetch their master's frames when loading notifications end up calling into its > (still attached) FrameFetchContext. Which won't have a frame -- > htmlimports/import-async-previous-async.html shows this up (if you've got leak > detection enabled.) > > The invariant introduced by this CL, I think, is that no ResourceFetcher should > have a FrameFetchContext attached once the frame of that FrameFetchContext > (i.e., FrameFetchContext::frame()) is detached from its document. Correct? > > In which case I think detaching a frame from a document needs to do a bit more > work when detaching the HTMLImportsController, in the Oilpan case. i.e., > HTMLImportsController::removeFrom() sync'ly destructs the controller and > everything below without Oilpan, it won't with. > > I need to think it over a bit more; the dependencies here aren't in short supply > :-) Handling this via https://codereview.chromium.org/1032293002/ -- it can be landed separately. Assuming it is well-formed overall..consulting the bots. Confirmed to fix the crashing condition that this CL runs into.
On 2015/03/26 04:43:14, Mike West wrote: > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > File Source/core/html/imports/LinkImport.cpp (right): > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > Source/core/html/imports/LinkImport.cpp:131: return m_owner && m_child && > m_child->isDone() && !m_child->loader()->hasError(); > Why doesn't this ASSERT hold true anymore? m_owner is nulled when the associated HTMLImportController is destroyed. The LinkImport's lifetime, on the other hand, is tied to that of the HTMLLinkElement. If the HTMLImportController is destroyed at Document detach time, the HTMLLinkElement (and therefore the LinkImport) will outlive it.
On 2015/03/26 09:27:38, sof wrote: > On 2015/03/26 08:23:30, sof wrote: > > https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cpp > > File Source/core/dom/Document.cpp (right): > > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... > > Source/core/dom/Document.cpp:618: if (m_importsController) > > The lack of this sync removal (and dispose) on the Oilpan side is leading to > > cases where we may have non-cancelled htmlimports loader objects attempting > > fetch their master's frames when loading notifications end up calling into its > > (still attached) FrameFetchContext. Which won't have a frame -- > > htmlimports/import-async-previous-async.html shows this up (if you've got leak > > detection enabled.) > > > > The invariant introduced by this CL, I think, is that no ResourceFetcher > should > > have a FrameFetchContext attached once the frame of that FrameFetchContext > > (i.e., FrameFetchContext::frame()) is detached from its document. Correct? > > > > In which case I think detaching a frame from a document needs to do a bit more > > work when detaching the HTMLImportsController, in the Oilpan case. i.e., > > HTMLImportsController::removeFrom() sync'ly destructs the controller and > > everything below without Oilpan, it won't with. > > > > I need to think it over a bit more; the dependencies here aren't in short > supply > > :-) > > Handling this via https://codereview.chromium.org/1032293002/ -- it can be > landed separately. > > Assuming it is well-formed overall..consulting the bots. Confirmed to fix the > crashing condition that this CL runs into. Shall I hold off on this patch until https://codereview.chromium.org/1032293002/ is landed?
On 2015/03/26 22:14:27, Nate Chapin wrote: > On 2015/03/26 09:27:38, sof wrote: > > On 2015/03/26 08:23:30, sof wrote: > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cpp > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... > > > Source/core/dom/Document.cpp:618: if (m_importsController) > > > The lack of this sync removal (and dispose) on the Oilpan side is leading to > > > cases where we may have non-cancelled htmlimports loader objects attempting > > > fetch their master's frames when loading notifications end up calling into > its > > > (still attached) FrameFetchContext. Which won't have a frame -- > > > htmlimports/import-async-previous-async.html shows this up (if you've got > leak > > > detection enabled.) > > > > > > The invariant introduced by this CL, I think, is that no ResourceFetcher > > should > > > have a FrameFetchContext attached once the frame of that FrameFetchContext > > > (i.e., FrameFetchContext::frame()) is detached from its document. Correct? > > > > > > In which case I think detaching a frame from a document needs to do a bit > more > > > work when detaching the HTMLImportsController, in the Oilpan case. i.e., > > > HTMLImportsController::removeFrom() sync'ly destructs the controller and > > > everything below without Oilpan, it won't with. > > > > > > I need to think it over a bit more; the dependencies here aren't in short > > supply > > > :-) > > > > Handling this via https://codereview.chromium.org/1032293002/ -- it can be > > landed separately. > > > > Assuming it is well-formed overall..consulting the bots. Confirmed to fix the > > crashing condition that this CL runs into. > > Shall I hold off on this patch until https://codereview.chromium.org/1032293002/ > is landed? No need to wait for it, we know what it would take to address the added crash.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... Source/core/dom/Document.cpp:2147: else if (m_importsController) Why is this 'else if', not 'if'?
On 2015/03/26 at 22:14:03, japhet wrote: > On 2015/03/26 04:43:14, Mike West wrote: > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > > File Source/core/html/imports/LinkImport.cpp (right): > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > > Source/core/html/imports/LinkImport.cpp:131: return m_owner && m_child && > > m_child->isDone() && !m_child->loader()->hasError(); > > Why doesn't this ASSERT hold true anymore? > > m_owner is nulled when the associated HTMLImportController is destroyed. The LinkImport's lifetime, on the other hand, is tied to that of the HTMLLinkElement. If the HTMLImportController is destroyed at Document detach time, the HTMLLinkElement (and therefore the LinkImport) will outlive it. Got it. LGTM, as long as Sigbjorn is happy (and it sounds like he is).
On 2015/03/27 11:14:44, Mike West wrote: > On 2015/03/26 at 22:14:03, japhet wrote: > > On 2015/03/26 04:43:14, Mike West wrote: > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > > > File Source/core/html/imports/LinkImport.cpp (right): > > > > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > > > Source/core/html/imports/LinkImport.cpp:131: return m_owner && m_child && > > > m_child->isDone() && !m_child->loader()->hasError(); > > > Why doesn't this ASSERT hold true anymore? > > > > m_owner is nulled when the associated HTMLImportController is destroyed. The > LinkImport's lifetime, on the other hand, is tied to that of the > HTMLLinkElement. If the HTMLImportController is destroyed at Document detach > time, the HTMLLinkElement (and therefore the LinkImport) will outlive it. > > Got it. LGTM, as long as Sigbjorn is happy (and it sounds like he is). https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... needs to be addressed/clarified for Oilpan, as Document will not be explicitly dispose()d there (which calls removeFrom() also.) I've no other issues/questions beyond that.
On 2015/03/27 13:21:11, sof wrote: > On 2015/03/27 11:14:44, Mike West wrote: > > On 2015/03/26 at 22:14:03, japhet wrote: > > > On 2015/03/26 04:43:14, Mike West wrote: > > > > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > > > > File Source/core/html/imports/LinkImport.cpp (right): > > > > > > > > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > > > > Source/core/html/imports/LinkImport.cpp:131: return m_owner && m_child && > > > > m_child->isDone() && !m_child->loader()->hasError(); > > > > Why doesn't this ASSERT hold true anymore? > > > > > > m_owner is nulled when the associated HTMLImportController is destroyed. The > > LinkImport's lifetime, on the other hand, is tied to that of the > > HTMLLinkElement. If the HTMLImportController is destroyed at Document detach > > time, the HTMLLinkElement (and therefore the LinkImport) will outlive it. > > > > Got it. LGTM, as long as Sigbjorn is happy (and it sounds like he is). > > https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... > needs to be addressed/clarified for Oilpan, as Document will not be explicitly > dispose()d there (which calls removeFrom() also.) > > I've no other issues/questions beyond that. I don't think I follow this concern. The HTMLImportController is being disposed separately from the Document and all connections are being severed. I would have thought that this will delete the HTMLImportController with and without oilpan, and the Document will then destruct normally. What am I missing?
On 2015/04/01 19:37:21, Nate Chapin wrote: > On 2015/03/27 13:21:11, sof wrote: > > On 2015/03/27 11:14:44, Mike West wrote: > > > On 2015/03/26 at 22:14:03, japhet wrote: > > > > On 2015/03/26 04:43:14, Mike West wrote: > > > > > > > > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > > > > > File Source/core/html/imports/LinkImport.cpp (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/html/imports/Li... > > > > > Source/core/html/imports/LinkImport.cpp:131: return m_owner && m_child > && > > > > > m_child->isDone() && !m_child->loader()->hasError(); > > > > > Why doesn't this ASSERT hold true anymore? > > > > > > > > m_owner is nulled when the associated HTMLImportController is destroyed. > The > > > LinkImport's lifetime, on the other hand, is tied to that of the > > > HTMLLinkElement. If the HTMLImportController is destroyed at Document detach > > > time, the HTMLLinkElement (and therefore the LinkImport) will outlive it. > > > > > > Got it. LGTM, as long as Sigbjorn is happy (and it sounds like he is). > > > > > https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... > > needs to be addressed/clarified for Oilpan, as Document will not be explicitly > > dispose()d there (which calls removeFrom() also.) > > > > I've no other issues/questions beyond that. > > I don't think I follow this concern. The HTMLImportController is being disposed > separately from the Document and all connections are being severed. I would have > thought that this will delete the HTMLImportController with and without oilpan, > and the Document will then destruct normally. What am I missing? The (start of) its disposal happens when HTMLImportsController::removeFrom() is invoked. We do not call Document::dispose() with Oilpan, which is why doing it here matters.
How's this look, Sigbjorn? https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1035803002/diff/1/Source/core/dom/Document.cp... Source/core/dom/Document.cpp:2147: else if (m_importsController) On 2015/03/27 11:01:33, haraken wrote: > > Why is this 'else if', not 'if'? It should be if(). I was experimented and forgot to change it back before uploading. https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... Source/core/dom/Document.cpp:2138: // If this document is the master for an HTMLImportsController, sever that Comment added to explain the rationale for detaching HTMLImportsController here. I believe detach() is guaranteed to be called before dispose() or ~Document(), so I've removed the removeFrom() calls in the #if !ENABLE(OILPAN) blocks in those functions. This is now the only place removeFrom() is called.
https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... Source/core/dom/Document.cpp:532: ASSERT(!m_importsController); Could you move this into the !ENABLE(OILPAN) section? We cannot guarantee that it has been cleared with oilpan. As to just removing the controller in detach(), let's try -- I don't see a code path with non-Oilpan where that won't be sufficient. https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... Source/core/dom/Document.cpp:2138: // If this document is the master for an HTMLImportsController, sever that On 2015/04/02 20:22:06, Nate Chapin wrote: > Comment added to explain the rationale for detaching HTMLImportsController here. > > I believe detach() is guaranteed to be called before dispose() or ~Document(), > so I've removed the removeFrom() calls in the #if !ENABLE(OILPAN) blocks in > those functions. This is now the only place removeFrom() is called. Good comment, thanks. This will hasten the demise of HTMLImportLoaders attached via that controller object, but I notice they don't tidily remove themselves as clients of the document parser when doing so -- leaving dead references. cf. a fast/dom/custom/ test that's failing on the trybots. Could you tidy up that as well here (or otherwise address)?
https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... Source/core/dom/Document.cpp:532: ASSERT(!m_importsController); On 2015/04/03 14:24:01, sof wrote: > Could you move this into the !ENABLE(OILPAN) section? We cannot guarantee that > it has been cleared with oilpan. Doesn't HTMLImportsController::removeFrom() call Document::setImportsController(nullptr)? > > As to just removing the controller in detach(), let's try -- I don't see a code > path with non-Oilpan where that won't be sufficient. https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... Source/core/dom/Document.cpp:2138: // If this document is the master for an HTMLImportsController, sever that On 2015/04/03 14:24:01, sof wrote: > On 2015/04/02 20:22:06, Nate Chapin wrote: > > Comment added to explain the rationale for detaching HTMLImportsController > here. > > > > I believe detach() is guaranteed to be called before dispose() or ~Document(), > > so I've removed the removeFrom() calls in the #if !ENABLE(OILPAN) blocks in > > those functions. This is now the only place removeFrom() is called. > > Good comment, thanks. > > This will hasten the demise of HTMLImportLoaders attached via that controller > object, but I notice they don't tidily remove themselves as clients of the > document parser when doing so -- leaving dead references. cf. a fast/dom/custom/ > test that's failing on the trybots. Could you tidy up that as well here (or > otherwise address)? Added removeClient() in HTMLImportLoader::dispose(), that fixed the test locally.
https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... Source/core/dom/Document.cpp:532: ASSERT(!m_importsController); On 2015/04/03 23:01:09, Nate Chapin wrote: > On 2015/04/03 14:24:01, sof wrote: > > Could you move this into the !ENABLE(OILPAN) section? We cannot guarantee that > > it has been cleared with oilpan. > > Doesn't HTMLImportsController::removeFrom() call > Document::setImportsController(nullptr)? > It does. But as stated previously, we cannot assume that Document::detach() has run with Oilpan.
On 2015/04/04 06:42:15, sof wrote: > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > File Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > Source/core/dom/Document.cpp:532: ASSERT(!m_importsController); > On 2015/04/03 23:01:09, Nate Chapin wrote: > > On 2015/04/03 14:24:01, sof wrote: > > > Could you move this into the !ENABLE(OILPAN) section? We cannot guarantee > that > > > it has been cleared with oilpan. > > > > Doesn't HTMLImportsController::removeFrom() call > > Document::setImportsController(nullptr)? > > > > It does. But as stated previously, we cannot assume that Document::detach() has > run with Oilpan. Aaaah, sorry. I had realized that dispose() wasn't guaranteed to be called with Oilpan, but I had apparently missed that detach() also wasn't guaranteed. Updated with assert in #if !ENABLE(OILPAN) block.
lgtm
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1035803002/#ps60001 (title: "Moved ASSERT(!m_importsController) to #if !ENABLE(OILPAN) block")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1035803002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193208
Message was sent while issue was closed.
On 2015/04/06 at 17:39:19, japhet wrote: > On 2015/04/04 06:42:15, sof wrote: > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > > File Source/core/dom/Document.cpp (right): > > > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > > Source/core/dom/Document.cpp:532: ASSERT(!m_importsController); > > On 2015/04/03 23:01:09, Nate Chapin wrote: > > > On 2015/04/03 14:24:01, sof wrote: > > > > Could you move this into the !ENABLE(OILPAN) section? We cannot guarantee > > that > > > > it has been cleared with oilpan. > > > > > > Doesn't HTMLImportsController::removeFrom() call > > > Document::setImportsController(nullptr)? > > > > > > > It does. But as stated previously, we cannot assume that Document::detach() has > > run with Oilpan. > > Aaaah, sorry. I had realized that dispose() wasn't guaranteed to be called with Oilpan, but I had apparently missed that detach() also wasn't guaranteed. Updated with assert in #if !ENABLE(OILPAN) block. What is the case where Document::detach() won't get called with Oilpan?
Message was sent while issue was closed.
falken@chromium.org changed reviewers: + falken@chromium.org
Message was sent while issue was closed.
I think this might be causing assert failures on imported/web-platform-tests/custom-elements/creating-and-passing-registries/share-registry-import-document.html http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=imp...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1063913002/ by falken@chromium.org. The reason for reverting is: Causes an assert failure on imported/web-platform-tests/custom-elements/creating-and-passing-registries/share-registry-import-document.html ASSERTION FAILED: m_thread == currentThread() ../../third_party/WebKit/Source/platform/Timer.h(160) : bool blink::TimerBase::isActive() const 1 0x7f421ebb9357 2 0x7f422003e6bb 3 0x7f422003f214 4 0x7f4229e9ae37 blink::ThreadTimers::sharedTimerFiredInternal() 5 0x7f4229e9aa65 blink::ThreadTimers::sharedTimerFired() Example failure: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20(dbg)/b... I verified locally that reverting fixes the test..
Message was sent while issue was closed.
On 2015/04/06 22:14:26, dcheng wrote: > On 2015/04/06 at 17:39:19, japhet wrote: > > On 2015/04/04 06:42:15, sof wrote: > > > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > > > File Source/core/dom/Document.cpp (right): > > > > > > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > > > Source/core/dom/Document.cpp:532: ASSERT(!m_importsController); > > > On 2015/04/03 23:01:09, Nate Chapin wrote: > > > > On 2015/04/03 14:24:01, sof wrote: > > > > > Could you move this into the !ENABLE(OILPAN) section? We cannot > guarantee > > > that > > > > > it has been cleared with oilpan. > > > > > > > > Doesn't HTMLImportsController::removeFrom() call > > > > Document::setImportsController(nullptr)? > > > > > > > > > > It does. But as stated previously, we cannot assume that Document::detach() > has > > > run with Oilpan. > > > > Aaaah, sorry. I had realized that dispose() wasn't guaranteed to be called > with Oilpan, but I had apparently missed that detach() also wasn't guaranteed. > Updated with assert in #if !ENABLE(OILPAN) block. > > What is the case where Document::detach() won't get called with Oilpan? That was the assumption when moving frames to the Oilpan heap, that it wouldn't be, but be implicitly cleared on the next GC (as the frame references were weak) or the objects would die at the same time (=> no detach was run.) However, we've moved away from that arrangement since then, making Oilpan vs not lifetimes be equal wrt frame handling/lifecycles, where possible. So, let me recheck status.
Message was sent while issue was closed.
On 2015/04/07 07:11:19, sof wrote: > On 2015/04/06 22:14:26, dcheng wrote: > > On 2015/04/06 at 17:39:19, japhet wrote: > > > On 2015/04/04 06:42:15, sof wrote: > > > > > > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > > > > Source/core/dom/Document.cpp:532: ASSERT(!m_importsController); > > > > On 2015/04/03 23:01:09, Nate Chapin wrote: > > > > > On 2015/04/03 14:24:01, sof wrote: > > > > > > Could you move this into the !ENABLE(OILPAN) section? We cannot > > guarantee > > > > that > > > > > > it has been cleared with oilpan. > > > > > > > > > > Doesn't HTMLImportsController::removeFrom() call > > > > > Document::setImportsController(nullptr)? > > > > > > > > > > > > > It does. But as stated previously, we cannot assume that > Document::detach() > > has > > > > run with Oilpan. > > > > > > Aaaah, sorry. I had realized that dispose() wasn't guaranteed to be called > > with Oilpan, but I had apparently missed that detach() also wasn't guaranteed. > > Updated with assert in #if !ENABLE(OILPAN) block. > > > > What is the case where Document::detach() won't get called with Oilpan? > > That was the assumption when moving frames to the Oilpan heap, that it wouldn't > be, but be implicitly cleared on the next GC (as the frame references were weak) > or the objects would die at the same time (=> no detach was run.) > > However, we've moved away from that arrangement since then, making Oilpan vs not > lifetimes be equal wrt frame handling/lifecycles, where possible. So, let me > recheck status. Frame detach will happen with & without Oilpan -- - ~HTMLFrameOwnerElement assumes that its frame has been cleared. - ~LocalFrame asserts that detach has happened (by checking that the FrameView has been cleared). By clearing the FrameView, Document::prepareForDestruction() will most likely be invoked (if not, there are other code paths achieving same during frame detach.) Hence, Document::detach() will be called with and without Oilpan. So, my assumption that detach() isn't guaranteed with Oilpan doesn't hold, hence the placement of the assert in this CL was correct as it was in ps#3. Sorry about the extra iteration that brought about..but good thing to have this detail clarified. Thanks for the raising the question.
Message was sent while issue was closed.
On 2015/04/07 13:11:42, sof wrote: > On 2015/04/07 07:11:19, sof wrote: > > On 2015/04/06 22:14:26, dcheng wrote: > > > On 2015/04/06 at 17:39:19, japhet wrote: > > > > On 2015/04/04 06:42:15, sof wrote: > > > > > > > > > > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > > > > > File Source/core/dom/Document.cpp (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/1035803002/diff/20001/Source/core/dom/Documen... > > > > > Source/core/dom/Document.cpp:532: ASSERT(!m_importsController); > > > > > On 2015/04/03 23:01:09, Nate Chapin wrote: > > > > > > On 2015/04/03 14:24:01, sof wrote: > > > > > > > Could you move this into the !ENABLE(OILPAN) section? We cannot > > > guarantee > > > > > that > > > > > > > it has been cleared with oilpan. > > > > > > > > > > > > Doesn't HTMLImportsController::removeFrom() call > > > > > > Document::setImportsController(nullptr)? > > > > > > > > > > > > > > > > It does. But as stated previously, we cannot assume that > > Document::detach() > > > has > > > > > run with Oilpan. > > > > > > > > Aaaah, sorry. I had realized that dispose() wasn't guaranteed to be called > > > with Oilpan, but I had apparently missed that detach() also wasn't > guaranteed. > > > Updated with assert in #if !ENABLE(OILPAN) block. > > > > > > What is the case where Document::detach() won't get called with Oilpan? > > > > That was the assumption when moving frames to the Oilpan heap, that it > wouldn't > > be, but be implicitly cleared on the next GC (as the frame references were > weak) > > or the objects would die at the same time (=> no detach was run.) > > > > However, we've moved away from that arrangement since then, making Oilpan vs > not > > lifetimes be equal wrt frame handling/lifecycles, where possible. So, let me > > recheck status. > > Frame detach will happen with & without Oilpan -- > > - ~HTMLFrameOwnerElement assumes that its frame has been cleared. > - ~LocalFrame asserts that detach has happened (by checking that the FrameView > has been cleared). > > By clearing the FrameView, Document::prepareForDestruction() will most likely be > invoked (if not, there are other code paths achieving same during frame detach.) > Hence, Document::detach() will be called with and without Oilpan. > > So, my assumption that detach() isn't guaranteed with Oilpan doesn't hold, hence > the placement of the assert in this CL was correct as it was in ps#3. Sorry > about the extra iteration that brought about..but good thing to have this detail > clarified. Thanks for the raising the question. For those preferring experimental data, https://codereview.chromium.org/1063013002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
