Closed Bug 1333045 Opened 3 years ago Closed 3 years ago

Location object changed spec violations

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: anba, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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.".
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]].
(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
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
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
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.
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
Attachment #8837176 - Flags: review?(kyle) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5061e0dedf2f
Update Location object properties to current spec.  r=qdot
https://hg.mozilla.org/mozilla-central/rev/5061e0dedf2f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1365840
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.