Bug 185737 – Avoid keeping the frame alive when ref'ing a WindowProxy

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Avoid keeping the frame alive when ref'ing a WindowProxy.

[Chris Dumez]

Created attachment 340598 [details]
WIP patch

[Chris Dumez]

<rdar://problem/40004666>

[Chris Dumez]

Created attachment 340600 [details]
WIP patch

[Chris Dumez]

Created attachment 340607 [details]
WIP Patch

[Chris Dumez]

Created attachment 340612 [details]
WIP Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 340619 [details]
WIP Patch

[Chris Dumez]

Created attachment 340631 [details]
Patch

[Chris Dumez]

Created attachment 340647 [details]
Patch

[Sam Weinig]

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?

[Chris Dumez]

(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.

[Chris Dumez]

(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; }

[Chris Dumez]

(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() / ...

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 340647 [details]
Patch

Clearing flags on attachment: 340647

Committed r231963: <https://trac.webkit.org/changeset/231963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.