window.location is not nullable (but is null in Gecko when the browsing context is discarded)

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: annevk, Assigned: baku)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 months ago
Test PR: https://github.com/w3c/web-platform-tests/pull/5778.

HTML: https://github.com/whatwg/html/issues/2545.
(Assignee)

Comment 1

6 months ago
Created attachment 8864458 [details] [diff] [review]
location.patch
Assignee: nobody → amarchesini
Attachment #8864458 - Flags: review?(bzbarsky)
Comment on attachment 8864458 [details] [diff] [review]
location.patch

Won't this just crash if the first .location get happens after GetDocShell starts returning null?

What we should probably do is just have the "location" getter pass a null docshell to the Location constructor...

>+    mLocation->SetDocShell(nullptr);

Why is this needed, both places?

Seems to me like we should instead just remove Location::SetDocShell altogether.  It's dead code before this patch.
Attachment #8864458 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 3

6 months ago
Created attachment 8864813 [details] [diff] [review]
location.patch
Attachment #8864458 - Attachment is obsolete: true
Attachment #8864813 - Flags: review?(bzbarsky)
Comment on attachment 8864813 [details] [diff] [review]
location.patch

> Location::GetURI(nsIURI** aURI, bool aGetInnermostURI)

So the behavior change here is to start returning NS_OK instead of an error result if Location has no docshell.  Why is that a desirable behavior change?  Might be good to explain that in the commit message.  Yes, the commit message says the change is being made, but not why.

My main problem with it is that it creates a thing that looks like an XPCOM getter but can in fact return null and NS_OK.  This needs to at least be clearly documented in the header, for both GetURI and GetWritableURI.  I did check that all callers are OK with it...
Attachment #8864813 - Flags: review?(bzbarsky) → review+

Comment 5

5 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79890abc6ff5
window.location is not nullable, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/48282ae47cf7
Fixing tests in order to support window.location not nullable, r=me

Comment 6

5 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4b235f7946
Fixing tests in order to support window.location not nullable: fix eslint complaint. r=eslint-fix

Comment 7

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/79890abc6ff5
https://hg.mozilla.org/mozilla-central/rev/48282ae47cf7
https://hg.mozilla.org/mozilla-central/rev/dc4b235f7946
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.