Avoid keeping the frame alive when ref'ing a WindowProxy.
Created attachment 340598 [details] WIP patch
<rdar://problem/40004666>
Created attachment 340600 [details] WIP patch
Created attachment 340607 [details] WIP Patch
Created attachment 340612 [details] WIP Patch
Attachment 340612 [details] did not pass style-queue: ERROR: Source/WebCore/page/AbstractFrame.h:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 340619 [details] WIP Patch
Created attachment 340631 [details] Patch
Created attachment 340647 [details] Patch
Comment on attachment 340647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340647&action=review > Source/WebCore/ChangeLog:15 > + It is important to not extend the lifetime of the Frame because we want script > + to stop running when the Page gets destroyed. I don't understand this. What does extending the lifetime of the Frame have to do with stopping script from running when a Page gets destroyed?
(In reply to Sam Weinig from comment #10) > Comment on attachment 340647 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340647&action=review > > > Source/WebCore/ChangeLog:15 > > + It is important to not extend the lifetime of the Frame because we want script > > + to stop running when the Page gets destroyed. > > I don't understand this. What does extending the lifetime of the Frame have > to do with stopping script from running when a Page gets destroyed? To fire events you need a global object. If the frame is gone then you have no way to get a global object.
(In reply to Chris Dumez from comment #11) > (In reply to Sam Weinig from comment #10) > > Comment on attachment 340647 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=340647&action=review > > > > > Source/WebCore/ChangeLog:15 > > > + It is important to not extend the lifetime of the Frame because we want script > > > + to stop running when the Page gets destroyed. > > > > I don't understand this. What does extending the lifetime of the Frame have > > to do with stopping script from running when a Page gets destroyed? > > To fire events you need a global object. If the frame is gone then you have > no way to get a global object. In particular, in JSEventListener::handleEvent(ScriptExecutionContext&, Event&): JSDOMGlobalObject* globalObject = toJSDOMGlobalObject(scriptExecutionContext, m_isolatedWorld); if (!globalObject) return; with toJSDOMGlobalObject() implemented as: JSDOMGlobalObject* toJSDOMGlobalObject(ScriptExecutionContext& context, DOMWrapperWorld& world) { if (is<Document>(context)) return toJSDOMWindow(downcast<Document>(context).frame(), world); if (is<WorkerGlobalScope>(context)) return downcast<WorkerGlobalScope>(context).script()->workerGlobalScopeWrapper(); ASSERT_NOT_REACHED(); return nullptr; } and toJSDOMWindow() returning nullptr when the frame is null: inline JSDOMWindow* toJSDOMWindow(Frame* frame, DOMWrapperWorld& world) { return frame ? toJSDOMWindow(*frame, world) : nullptr; }
(In reply to Chris Dumez from comment #12) > (In reply to Chris Dumez from comment #11) > > (In reply to Sam Weinig from comment #10) > > > Comment on attachment 340647 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=340647&action=review > > > > > > > Source/WebCore/ChangeLog:15 > > > > + It is important to not extend the lifetime of the Frame because we want script > > > > + to stop running when the Page gets destroyed. > > > > > > I don't understand this. What does extending the lifetime of the Frame have > > > to do with stopping script from running when a Page gets destroyed? > > > > To fire events you need a global object. If the frame is gone then you have > > no way to get a global object. > > In particular, in JSEventListener::handleEvent(ScriptExecutionContext&, > Event&): > JSDOMGlobalObject* globalObject = > toJSDOMGlobalObject(scriptExecutionContext, m_isolatedWorld); > if (!globalObject) > return; > > with toJSDOMGlobalObject() implemented as: > JSDOMGlobalObject* toJSDOMGlobalObject(ScriptExecutionContext& context, > DOMWrapperWorld& world) > { > if (is<Document>(context)) > return toJSDOMWindow(downcast<Document>(context).frame(), world); > > if (is<WorkerGlobalScope>(context)) > return > downcast<WorkerGlobalScope>(context).script()->workerGlobalScopeWrapper(); > > ASSERT_NOT_REACHED(); > return nullptr; > } > > and toJSDOMWindow() returning nullptr when the frame is null: > inline JSDOMWindow* toJSDOMWindow(Frame* frame, DOMWrapperWorld& world) { > return frame ? toJSDOMWindow(*frame, world) : nullptr; } Also, if the Frame is gone, then the ScriptController is gone, so no executeScript() / evaluate() / ...
Comment on attachment 340647 [details] Patch Attachment 340647 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7722627 New failing tests: http/tests/preload/onload_event.html
Created attachment 340707 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 340647 [details] Patch Clearing flags on attachment: 340647 Committed r231963: <https://trac.webkit.org/changeset/231963>
All reviewed patches have been landed. Closing bug.