Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make window.name deal with lack of browsing context #4366

Merged
merged 2 commits into from
Feb 19, 2019
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented Feb 14, 2019

annevk added a commit to web-platform-tests/wpt that referenced this pull request Feb 14, 2019
@domenic
Copy link
Member

domenic commented Feb 14, 2019

As discussed in IRC, this reveals some fractal brokenness.

The current spec seems to assume that there is always a strong reference from Window -> BC. Discarding only removes the strong reference from UA -> BC; if some script holds a JS -> Window reference, then BC stays alive.

The tests in web-platform-tests/wpt#15391 indicate, however, that window.name returns the empty string (and thus, setting is a no-op) for any Window attached to an iframe that was .remove()ed. So this PR is indeed doing something right.

I see two ways forward:

  1. Discarding a BC (mostly visible by .remove()ing iframes? also window.close() and manual window closes) should null out the Window -> BC reference. Then this PR is good as-is, although there are other parts of the spec that are probably broken and inconsistent (e.g. window.closed checks whether the BC was discarded).
  2. This PR should switch from branching on "Window doesn't have a BC" to "Window's BC was discarded". This would be consistent with the existing spec, but the existing spec is on shaky ground. There is no "discarded" boolean yet; that's a bit hand-wavey. And the notion of there being a Window -> BC link that exists but we always (usually?) guard with an "is discarded" check is annoying.

I lean toward (1), with follow-up work being to guard all Window -> BC references with a null check. But, I am a bit scared that we have this long-standing assumption in the spec of an always-existing Window -> BC link. I'd love to hear from folks like @bzbarsky and @jdm.

Somewhat-related: #1279 #4076 #1073

@domenic
Copy link
Member

domenic commented Feb 14, 2019

Oh, well, it turns out the spec doesn't seem to have any definition of a Window -> BC pointer, only a document -> BC pointer. It assumes and uses a Window -> BC pointer a lot. But in #4363 (comment) @annevk suggests defining "Window's BC" to be "Window's associated document's BC", or null if Window has no associated document.

I'm unsure what the implications would be there, as I can't immediately see if "discard" severs the Document -> BC relationship. But, it's something to consider.

@jdm
Copy link
Member

jdm commented Feb 14, 2019

https://html.spec.whatwg.org/multipage/window-object.html#a-browsing-context-is-discarded seems to say that the document's BC relationship would be severed but other references to the BC remain valid.

@annevk
Copy link
Member Author

annevk commented Feb 15, 2019

FWIW, I think this is a variation on #3846 and I think as @bzbarsky says there we need null checks. Or we need a flag and then check for that flag, but null seems much cleaner and easier to spot mistakes with.

@annevk
Copy link
Member Author

annevk commented Feb 15, 2019

Note also that the specification already has this:

And the Document originally created for an iframe element, which has since been removed from the document, has no associated browsing context, since that browsing context was discarded.

@domenic
Copy link
Member

domenic commented Feb 15, 2019

With #4368 as a foundation I'm happy with (1). It looks like that makes it properly clear that after discarding, a Document's BC and thus the Window's BC is null.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Feb 18, 2019
annevk added a commit that referenced this pull request Feb 19, 2019
Previously, a Document could sometimes "have no browsing context". Now,
"a Document's browsing context" is either a browsing context or null.

Previously, "a Window's browsing context" was not defined, despite being
referenced extensively. Now, it is defined to be the Window's associated
Document's browsing context.

Related to #3846, #4363, and #4366.
@annevk annevk force-pushed the annevk/window.name branch from 496364c to f9cc3c2 Compare February 19, 2019 10:42
@annevk annevk merged commit b8c084e into master Feb 19, 2019
@annevk annevk deleted the annevk/window.name branch February 19, 2019 10:50
@annevk annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Feb 19, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 15, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Mar 16, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 16, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Mar 17, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 23, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391

UltraBlame original commit: 2eb522ceb6e1c842c8c5b2950d4a4ab2ef88268c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 4, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391

UltraBlame original commit: 9acf395a6242f447db603b86c78f32f722081cf9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391

UltraBlame original commit: 2eb522ceb6e1c842c8c5b2950d4a4ab2ef88268c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 4, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391

UltraBlame original commit: 9acf395a6242f447db603b86c78f32f722081cf9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391

UltraBlame original commit: 2eb522ceb6e1c842c8c5b2950d4a4ab2ef88268c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 4, 2019
Automatic update from web-platform-tests
HTML: window.name

For whatwg/html#4366.
--

wpt-commits: 8e76fc76d3af0a56d0ccdd627fe844fd9c5a1554
wpt-pr: 15391

UltraBlame original commit: 9acf395a6242f447db603b86c78f32f722081cf9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants