Bug 181505 – Setting Window.opener to null should disown its opener

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Setting Window.opener to null to disown the window's opener:
- https://html.spec.whatwg.org/#dom-opener

This seems to be impacting Gmail.

[Chris Dumez]

Created attachment 330990 [details]
WIP Patch

[Chris Dumez]

Will work on a test tomorrow.

[EWS Watchlist]

Comment on attachment 330990 [details]
WIP Patch

Attachment 330990 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6029767

New failing tests:
webgl/1.0.2/conformance/uniforms/uniform-default-values.html

[EWS Watchlist]

Created attachment 331026 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 331026 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

Not related.

[Radar WebKit Bug Importer]

<rdar://problem/36443151>

[Chris Dumez]

Created attachment 331074 [details]
Patch

[John Wilander]

Looks good to me. This was the thing I think Mike West was talking about — disowning the opener.

[John Wilander]

Is there a way to test that disowning the opener makes WebKit not keep the pages in the same web process?

[Chris Dumez]

(In reply to John Wilander from comment #9)
> Is there a way to test that disowning the opener makes WebKit not keep the
> pages in the same web process?

Well, but it does currently :P

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> (In reply to John Wilander from comment #9)
> > Is there a way to test that disowning the opener makes WebKit not keep the
> > pages in the same web process?
> 
> Well, but it does currently :P

That's another issue we have to fix.

[EWS Watchlist]

Comment on attachment 331074 [details]
Patch

Attachment 331074 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6037592

New failing tests:
fast/dom/Window/window-opener-set-to-null.html

[EWS Watchlist]

Created attachment 331096 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 331098 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 331098 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:478
> +    replaceStaticPropertySlot(state.vm(), this, Identifier::fromString(&state.vm(), reinterpret_cast<const LChar*>("opener"), strlen("opener")), value);

Why not just Identifier::fromString(&state.vm(), "opener")?

[Chris Dumez]

Comment on attachment 331098 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:478
>> +    replaceStaticPropertySlot(state.vm(), this, Identifier::fromString(&state.vm(), reinterpret_cast<const LChar*>("opener"), strlen("opener")), value);
> 
> Why not just Identifier::fromString(&state.vm(), "opener")?

Hmm, I copy/pasted the code from the bindings generator. I'll look into it so see if there is a good reason.

[Chris Dumez]

Created attachment 331161 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 331161 [details]
Patch

Clearing flags on attachment: 331161

Committed r226842: <https://trac.webkit.org/changeset/226842>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 331161 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:478
> +    if (value.isNull()) {
> +        wrapped().disownOpener();
> +        return;
> +    }
> +    replaceStaticPropertySlot(state.vm(), this, Identifier::fromString(&state.vm(), "opener"), value);

I don’t see test coverage for the case where we first have opener reflecting the real opening, then set it to "foo" and then set it to null. Will this do the right thing in that case, or will "foo" still be left behind in the static property slot?

[Chris Dumez]

(In reply to Darin Adler from comment #20)
> Comment on attachment 331161 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331161&action=review
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:478
> > +    if (value.isNull()) {
> > +        wrapped().disownOpener();
> > +        return;
> > +    }
> > +    replaceStaticPropertySlot(state.vm(), this, Identifier::fromString(&state.vm(), "opener"), value);
> 
> I don’t see test coverage for the case where we first have opener reflecting
> the real opening, then set it to "foo" and then set it to null. Will this do
> the right thing in that case, or will "foo" still be left behind in the
> static property slot?

I will add testing and check other browsers. My current implementation would disown the opener and keep "foo" expando property. This seems correct to me based on my interpretation of the specification:
"""
On setting, if the new value is null then the current browsing context must disown its opener; if the new value is anything else then the user agent must call the [[DefineOwnProperty]] internal method of the Window object, passing the property name "opener" as the property key, and the Property Descriptor { [[Value]]: value, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true } as the property descriptor, where value is the new value.
"""

[Chris Dumez]

(In reply to Chris Dumez from comment #21)
> (In reply to Darin Adler from comment #20)
> > Comment on attachment 331161 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=331161&action=review
> > 
> > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:478
> > > +    if (value.isNull()) {
> > > +        wrapped().disownOpener();
> > > +        return;
> > > +    }
> > > +    replaceStaticPropertySlot(state.vm(), this, Identifier::fromString(&state.vm(), "opener"), value);
> > 
> > I don’t see test coverage for the case where we first have opener reflecting
> > the real opening, then set it to "foo" and then set it to null. Will this do
> > the right thing in that case, or will "foo" still be left behind in the
> > static property slot?
> 
> I will add testing and check other browsers. My current implementation would
> disown the opener and keep "foo" expando property. This seems correct to me
> based on my interpretation of the specification:
> """
> On setting, if the new value is null then the current browsing context must
> disown its opener; if the new value is anything else then the user agent
> must call the [[DefineOwnProperty]] internal method of the Window object,
> passing the property name "opener" as the property key, and the Property
> Descriptor { [[Value]]: value, [[Writable]]: true, [[Enumerable]]: true,
> [[Configurable]]: true } as the property descriptor, where value is the new
> value.
> """

Firefox and chrome seem to return null and not "foo" which means we are not interoperable unfortunately. I filed a bug against the spec (https://github.com/whatwg/html/issues/3351) and will align WebKit with other browser engines.

[Chris Dumez]

(In reply to Chris Dumez from comment #22)
> (In reply to Chris Dumez from comment #21)
> > (In reply to Darin Adler from comment #20)
> > > Comment on attachment 331161 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=331161&action=review
> > > 
> > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:478
> > > > +    if (value.isNull()) {
> > > > +        wrapped().disownOpener();
> > > > +        return;
> > > > +    }
> > > > +    replaceStaticPropertySlot(state.vm(), this, Identifier::fromString(&state.vm(), "opener"), value);
> > > 
> > > I don’t see test coverage for the case where we first have opener reflecting
> > > the real opening, then set it to "foo" and then set it to null. Will this do
> > > the right thing in that case, or will "foo" still be left behind in the
> > > static property slot?
> > 
> > I will add testing and check other browsers. My current implementation would
> > disown the opener and keep "foo" expando property. This seems correct to me
> > based on my interpretation of the specification:
> > """
> > On setting, if the new value is null then the current browsing context must
> > disown its opener; if the new value is anything else then the user agent
> > must call the [[DefineOwnProperty]] internal method of the Window object,
> > passing the property name "opener" as the property key, and the Property
> > Descriptor { [[Value]]: value, [[Writable]]: true, [[Enumerable]]: true,
> > [[Configurable]]: true } as the property descriptor, where value is the new
> > value.
> > """
> 
> Firefox and chrome seem to return null and not "foo" which means we are not
> interoperable unfortunately. I filed a bug against the spec
> (https://github.com/whatwg/html/issues/3351) and will align WebKit with
> other browser engines.

As it turns out, the spec and our implementation is correct. I am extending testing via Bug 181634. When you set it to "foo", it calls [DefineOwnProperty] which overrides our "native" property. When you then set to null, it merely updates the descriptor's value to null, it does not call our native setter.