Setting Window.opener to null to disown the window's opener: - https://html.spec.whatwg.org/#dom-opener This seems to be impacting Gmail.
Created attachment 330990 [details] WIP Patch
Will work on a test tomorrow.
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
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
Comment on attachment 331026 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 Not related.
<rdar://problem/36443151>
Created attachment 331074 [details] Patch
Looks good to me. This was the thing I think Mike West was talking about — disowning the opener.
Is there a way to test that disowning the opener makes WebKit not keep the pages in the same web process?
(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
(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.
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
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
Created attachment 331098 [details] Patch
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")?
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.
Created attachment 331161 [details] Patch
Comment on attachment 331161 [details] Patch Clearing flags on attachment: 331161 Committed r226842: <https://trac.webkit.org/changeset/226842>
All reviewed patches have been landed. Closing bug.
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?
(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. """
(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.
(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.