Bug 185737 - Avoid keeping the frame alive when ref'ing a WindowProxy
Summary: Avoid keeping the frame alive when ref'ing a WindowProxy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-17 10:47 PDT by Chris Dumez
Modified: 2018-05-18 10:56 PDT (History)
10 users (show)

See Also:


Attachments
WIP patch (15.21 KB, patch)
2018-05-17 10:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (15.25 KB, patch)
2018-05-17 10:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (15.78 KB, patch)
2018-05-17 11:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (15.92 KB, patch)
2018-05-17 11:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (17.40 KB, patch)
2018-05-17 12:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.35 KB, patch)
2018-05-17 13:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.42 KB, patch)
2018-05-17 14:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.84 MB, application/zip)
2018-05-18 10:15 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-05-17 10:47:27 PDT
Avoid keeping the frame alive when ref'ing a WindowProxy.
Comment 1 Chris Dumez 2018-05-17 10:49:33 PDT
Created attachment 340598 [details]
WIP patch
Comment 2 Chris Dumez 2018-05-17 10:50:27 PDT
<rdar://problem/40004666>
Comment 3 Chris Dumez 2018-05-17 10:55:13 PDT
Created attachment 340600 [details]
WIP patch
Comment 4 Chris Dumez 2018-05-17 11:08:47 PDT
Created attachment 340607 [details]
WIP Patch
Comment 5 Chris Dumez 2018-05-17 11:44:23 PDT
Created attachment 340612 [details]
WIP Patch
Comment 6 EWS Watchlist 2018-05-17 11:46:46 PDT
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.
Comment 7 Chris Dumez 2018-05-17 12:00:11 PDT
Created attachment 340619 [details]
WIP Patch
Comment 8 Chris Dumez 2018-05-17 13:33:09 PDT
Created attachment 340631 [details]
Patch
Comment 9 Chris Dumez 2018-05-17 14:44:34 PDT
Created attachment 340647 [details]
Patch
Comment 10 Sam Weinig 2018-05-17 21:57:58 PDT
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?
Comment 11 Chris Dumez 2018-05-18 07:13:29 PDT
(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.
Comment 12 Chris Dumez 2018-05-18 08:51:01 PDT
(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; }
Comment 13 Chris Dumez 2018-05-18 08:53:12 PDT
(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 14 EWS Watchlist 2018-05-18 10:15:38 PDT
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
Comment 15 EWS Watchlist 2018-05-18 10:15:50 PDT
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 16 WebKit Commit Bot 2018-05-18 10:34:25 PDT
Comment on attachment 340647 [details]
Patch

Clearing flags on attachment: 340647

Committed r231963: <https://trac.webkit.org/changeset/231963>
Comment 17 WebKit Commit Bot 2018-05-18 10:34:27 PDT
All reviewed patches have been landed.  Closing bug.