Bug 103789 – Add FrameLoaderClient::didDisownOpener

• Component: WebKit API
• Status: RESOLVED FIXED

[Charles Reis]

There is a special meaning to assigning window.opener to null: it disowns the opener for the future lifetime of the browsing context (i.e., window).
http://dev.w3.org/html5/spec/single-page.html#disowned-its-opener

In some ports, the client needs to be notified when this occurs for a window.  For example, Chromium needs to know not to make window.opener available in other renderer processes that might be used in the window.

Repro steps:
1) On site A, open a new window.
2) Navigate the new window to site B (cross-process).  window.opener is valid.
3) Set window.opener=null.
4) Go back to the original process, or navigate to site C in a third process.  window.opener should still be null.

To fix this, I'd like to add a FrameLoaderClient::didDisownOpener method to notify the client when the opener gets cleared.

[Charles Reis]

Created attachment 177047 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 177047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177047&action=review

> Source/WebKit/chromium/public/WebFrameClient.h:113
> +    // This frame set its opener to null, disowning it.
> +    virtual void didDisownOpener(WebFrame*) { }

Can you add a reference to the specification in a comment so that it's clear what this function means?

[Alexey Proskuryakov]

Comment on attachment 177047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177047&action=review

> There is a special meaning to assigning window.opener to null

Window.opener is read-only in IDL, so there is no code path that results in FrameLoader::setOpener(0) being called when this happens:

    [Replaceable, DoNotCheckSecurityOnGetter, V8CustomSetter] readonly attribute DOMWindow opener;

Do v8 bindings somehow override this behavior via a custom setter? It seems super misleading to add a cross-platform FrameLoaderClient function that is never invoked in expected circumstances on JSC, but is invoked in unexpected circumstances.

> Source/WebCore/loader/FrameLoader.cpp:944
> +    if (m_opener && !opener)
> +        m_client->didDisownOpener();

Is it OK that this client call will be made from FrameLoader destructor?

Is it OK that it will be called from WKBundleFrameClearOpener WebKit2 API?

[Adam Barth]

In the spec (and I believe in other browsers), you can set the opener to null but not to any other value:

http://html.spec.whatwg.org/#dom-opener

Setting the opener to null is "sticky" and persists across navigations.

[Adam Barth]

From http://html.spec.whatwg.org/#window

interface Window : EventTarget {
[...]
           attribute WindowProxy? opener;

It looks like having readonly in our IDL is a (subtle) mistake.

[Alexey Proskuryakov]

On JSC ports, the lack of a custom setter results in a very different behavior:

void setJSDOMWindowOpener(ExecState* exec, JSObject* thisObject, JSValue value)
{
    UNUSED_PARAM(exec);
    if (!BindingSecurity::shouldAllowAccessToDOMWindow(exec, jsCast<JSDOMWindow*>(thisObject)->impl()))
        return;
    // Shadowing a built-in object
    jsCast<JSDOMWindow*>(thisObject)->putDirect(exec->globalData(), Identifier(exec, "opener"), value);
}

[Adam Barth]

I've filed bug 103913 about the JSC bug.

[Charles Reis]

Comment on attachment 177047 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177047&action=review

>> Source/WebCore/loader/FrameLoader.cpp:944
>> +        m_client->didDisownOpener();
> 
> Is it OK that this client call will be made from FrameLoader destructor?
> 
> Is it OK that it will be called from WKBundleFrameClearOpener WebKit2 API?

Good question about the FrameLoader destructor.  The extra notification there isn't needed but shouldn't be a problem.  In Chrome's case, we've already deleted the WebFrame's client by that time, so the notification doesn't make it out of FrameLoaderClientImpl.

For WKBundleFrameClearOpener, it looks like that's clearing the opener between tests.  I think the notification makes sense there as well.

[Charles Reis]

Created attachment 177319 [details]
Patch

[Charles Reis]

Created attachment 177321 [details]
Patch

[Charles Reis]

(In reply to comment #3)
> (From update of attachment 177047 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177047&action=review
> 
> > Source/WebKit/chromium/public/WebFrameClient.h:113
> > +    // This frame set its opener to null, disowning it.
> > +    virtual void didDisownOpener(WebFrame*) { }
> 
> Can you add a reference to the specification in a comment so that it's clear what this function means?

Added a mention in both FrameLoaderClient.h and WebFrameClient.h.  (Sorry for the extra patch set; I only put it in FrameLoaderClient the first time.)

[Alexey Proskuryakov]

Comment on attachment 177321 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177321&action=review

> Source/WebCore/loader/FrameLoaderClient.h:217
> +        virtual void didDisownOpener() { }

Can you please add a FIXME here, referencing bug 103913?

[Charles Reis]

Created attachment 177337 [details]
Patch

[Charles Reis]

(In reply to comment #13)
> (From update of attachment 177321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177321&action=review
> 
> > Source/WebCore/loader/FrameLoaderClient.h:217
> > +        virtual void didDisownOpener() { }
> 
> Can you please add a FIXME here, referencing bug 103913?

Certainly.  New patch uploaded.

[WebKit Review Bot]

Comment on attachment 177337 [details]
Patch

Rejecting attachment 177337 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   8cbb25f..78cf9b7  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at 78cf9b751c12b828663109bfb0f349f17833261d but expected 8cbb25f3febaaee8d69fbea1e5b98dd0173b71a9
 ! 8cbb25f..78cf9b7  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15117764

[Charles Reis]

Comment on attachment 177337 [details]
Patch

Looks like the failure might have been a git issue on the bot, rather than my patch.  I'll try again.

[WebKit Review Bot]

Comment on attachment 177337 [details]
Patch

Clearing flags on attachment: 177337

Committed r136516: <http://trac.webkit.org/changeset/136516>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.