Location object changed spec violations

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: anba, Assigned: bz)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
From https://html.spec.whatwg.org/multipage/browsers.html#the-location-interface:

1. "toString", "valueOf", and "toJSON" are specified as non-enumerable, but are enumerable in Firefox.
2. window.location.valueOf isn't the intrinsic object %ObjProto_valueOf% (`window.location.valueOf === Object.prototype.valueOf` is false, but should be true).
3. window.location.toString isn't the intrinsic object %ObjProto_toString% (`window.location.toString === Object.prototype.toString` is false, but should be true). The spec seems to contradict itself, because later in the spec toString() is described as "Returns the Location object's URL.".

Comment 1

a year ago
Could you file a bug on the toString() behavior? We should remove that declaration as it appears to be broken. It was meant to mimic https://github.com/heycam/webidl/commit/20a1c3d310f84b103ba0bf429aa4d2a2658f6fa9, but the fact that the Location IDL already contained a [Unforgeable] stringifier overrode that default behavior.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=27361 has the rationale for changing [[Enumerable]].
(Reporter)

Comment 2

a year ago
(In reply to Anne (:annevk) from comment #1)
> Could you file a bug on the toString() behavior? We should remove that
> declaration as it appears to be broken. It was meant to mimic
> https://github.com/heycam/webidl/commit/
> 20a1c3d310f84b103ba0bf429aa4d2a2658f6fa9, but the fact that the Location IDL
> already contained a [Unforgeable] stringifier overrode that default behavior.

Filed https://github.com/whatwg/html/issues/2284.
Yeah, so basically the spec changed on us after we'd been shipping what the previous spec said for a good long while.

How confidend are we that the spec is stable this time?  I'm loath to invest any time in this, or ask anyone else to, without some sort of guarantees on this front.  In any case, the bits that are needed to fix this are:

1)  Ignore what the spec is talking about with toString, because it's bonkers, per comment 1.
2)  Remove the JSPROP_ENUMERATE from http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/bindings/Codegen.py#3548-3550
3)  Remove the JSPROP_ENUMERATE from http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/bindings/Codegen.py#2487-2495

That will fix all the bits except window.location.valueOf === Object.prototype.valueOf testing false.  For that one... first note that currently the two valueOf methods have different behavior.  Here's a simple example; these expressions both test true per the old Location spec:

  location.valueOf.call(5) === 5
  Object.prototype.valueOf.call(5) !== 5

Again, it's not clear to me whether this behavior change is desirable.  I should note that other browsers (e.g. Chrome) don't seem to be in a hurry to implement the new thing: location.valueOf !== Object.prototype.valueOf there.

In any case, if one did want to implement this, it would be by far the hardest part of this bug; possibly worth doing in a separate bug.  Object.prototype.valueOf is not self-hosted, so the usual way we common up methods by listing the selfHostedName wouldn't work.  SpiderMonkey doesn't seem to have a declarative way of saying you want a particular JSNative-backed built-in.  So whoever wants to fix this part would need to either add such a way or self-host Object.prototype.valueOf.

In either case, once all that's is done you'd want to remove http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/bindings/BindingUtils.cpp#1214-1220
Summary: Location object spec violations → Location object changed spec violations

Comment 4

a year ago
window.location.valueOf === Object.prototype.valueOf and location.valueOf.call(5) !== 5 are true in Safari TP. They implemented the standard recently.

The standard has been discussed with you, Bobby, and Adam (Google), and hopefully also reviewed to some extent.

valueOf has been discussed at https://github.com/whatwg/html/pull/638#issuecomment-185890302 and you were involved in that too.
Yes, I'm aware of all that.  The real question is what Blink and WebKit plan to do here.
Depends on: 1333143

Comment 6

a year ago
Steps I've taken thus far:

* Provide a fix for the toString issue in HTML: https://github.com/whatwg/html/pull/2294
* Wrote (additional) tests for toString, toJSON, valueOf, and Symbol.toPrimitive: https://github.com/w3c/web-platform-tests/pull/4623

Once those are reviewed I will file issues against the other browsers. We can then either await what they do or try to fix this first.
Priority: -- → P2

Comment 7

a year ago
The issue with toString() in the HTML Standard is now fixed. It also resulted in us removing toJSON() from Location as only Gecko had that implemented. The tests have been updated accordingly and landed on WPT.
Created attachment 8837176 [details] [diff] [review]
Update Location object properties to current spec

Specifically, three changes:

1) valueOf should be non-enumerable.
2) valueOf should be === to Object.prototype.valueOf.
3) There should be no toJSON.

The tests come directly from https://github.com/w3c/web-platform-tests/pull/4623
so not much need to review them.
Attachment #8837176 - Flags: review?(kyle)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED

Comment 9

a year ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5061e0dedf2f
Update Location object properties to current spec.  r=qdot

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5061e0dedf2f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

11 months ago
Duplicate of this bug: 1365840
You need to log in before you can comment on or make changes to this bug.