Object.prototype.toString doesn't override classList (DOMTokenList)

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

Trunk
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

7 years ago
uneval(Object.prototype.toString.call(window.getSelection()))
"[object Selection]"

But:

uneval(Object.prototype.toString.call(document.documentElement.classList))
""

The "catalogNew" function in my DOM fuzzer relies on the "normal" Object.prototype.toString behavior of calling getClass.  It does this to know when it has found a new "kind" of object, and it's usually more reliable than checking obj.__proto__.
So this is an interesting question.  What _should_ the behavior be here?

js::obj_toStringHelper checks for isProxy() and if true calls Proxy::obj_toString.  That's what we use to hand back the string in the normal document.documentElement.classList.toString() case.

Peter, can we just put the toString on the proto for stringifiers like http://dev.w3.org/2006/webapi/WebIDL/#es-stringifier says and have our proxy's obj_toString return the classname thing?  Seems like that would be more correct...
(Tested in dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json)
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86_64 → All
Yup, that seems fine to me. We could even give non-proxies stringifier support at the same time?
> (Tested in dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json)

It is?  I don't see where...
In fact, I see nothing in test_interfaces.html that tests this.

What that test _does_ have is:

632         assert_equals(String(window[this.name].prototype), "[object " + this.name + "Prototype]",
633                       "String(" + this.name + ")");

which should throw for an interface with a stringifier.  And does with the changes I'm making in this bug.  I guess I'll fix the harness to:

1)  Actually test this bug.
2)  Not bogusly test the above thing.
Oh, I see.  #1 is the |"Stringification of document.body.classList": true| bit.
So general comment on this test harness:  The object model is obtuse and undocumented.  Add to that the fact that the test harness coalesces stuff so its hard to figure out where inside a long test function it actually did something dumb and failed, and actually working with it a huge pain.  :(
And in particular, I have now spent several hours trying to figure out how to teach the harness that DOMSettableTokenList has a (inherited) stringifier, such that the above String() test is bogus for it, with no success.
Attachment #668679 - Flags: review?(Ms2ger)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Comment on attachment 668679 [details] [diff] [review]
Support stringifier operations (but not yet attributes!) on non-proxy bindings, and fix Object.prototype.toString for proxy bindings.

Review of attachment 668679 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the test bits (and completely agree on the object model :/).

::: dom/bindings/Configuration.py
@@ +222,5 @@
>                          else:
>                              assert m.isNamed()
>                              operation = 'Named' + operation
>                          addOperation(operation, m)
>                          

Nit: fix the trailing whitespace while you're here

::: dom/imptests/idlharness.js
@@ +1054,5 @@
> +                haveStringifier = true;
> +                break;
> +            }
> +            if (iface.inheritance.length > 0) {
> +                 iface = interfaces[iface.inheritance[0]];

Nit: indentation is off a bit here
Attachment #668679 - Flags: review?(Ms2ger) → review+
A question on the test harness.   Do I just land the change on our end and then you port it to the W3C side?  Or do you land it there and port it here?
Just land, I can deal.
(In reply to Boris Zbarsky (:bz) from comment #7)
> So general comment on this test harness:  The object model is obtuse and
> undocumented.  Add to that the fact that the test harness coalesces stuff so
> its hard to figure out where inside a long test function it actually did
> something dumb and failed, and actually working with it a huge pain.  :(

The object model is mostly determined by the parser -- it's taken from the syntax used in compiling the parser.  Which, unfortunately, makes it both obtuse and undocumented, but when writing this I only found one available WebIDL parser, so I dealt with what I had.  What attributes different types of things have can be inferred somewhat from this:

https://github.com/darobin/webidl.js/blob/master/lib/grammar.peg

So for instance

  // interface definition
  interface
      =   extAttrs:extendedAttributeList? S? "interface" S name:identifier w herit:ifInheritance? w "{" w mem:ifMember* w "}" w ";" w
          { return { type: "interface", name: name, inheritance: herit, members: mem, extAttrs: extAttrs }; }

means that an interface object will have members .type (equal to the string "interface"), .name (equal to the parsed interface name), .inheritance (equal to an object derived from ifInheritance elsewhere in the file, or I guess maybe null because of the "?"), .members, and .extAttrs (like .inheritance).

I can add some more comments explaining the object structure when I have time, including a breakdown of the properties of the more common types of objects.  Right now Ehsan has put some security bugs and crashers on my plate, though, so this isn't my first priority.
Comment on attachment 668679 [details] [diff] [review]
Support stringifier operations (but not yet attributes!) on non-proxy bindings, and fix Object.prototype.toString for proxy bindings.

Review of attachment 668679 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/imptests/idlharness.js
@@ +1063,5 @@
> +        if (!haveStringifier)
> +        {
> +            assert_equals(String(window[this.name].prototype), "[object " + this.name + "Prototype]",
> +                          "String(" + this.name + ".prototype)");
> +        }

This doesn't deal with the two other places in the file where we deal with stringifiers.  I'll write a patch to idlharness.js upstream to do the same thing more generally, and maybe test stringifiers a bit more overall.
Stringifier check fixes:

http://dvcs.w3.org/hg/resources/rev/69e9d2c53980

Documentation:

http://dvcs.w3.org/hg/resources/rev/2d81e8368bb1

I realized after the first commit that exceptions can't have stringifiers, so I removed the code related to that in the second commit, and some more over here:

http://dvcs.w3.org/hg/resources/rev/a985f4dfdc99
> The object model is mostly determined by the parser

Sort of.  The specific issue I was having is that the .inheritance of an interface object is not the parent interface object (as I would have expected), nor is it an array of interface objects (as one might expect based on quick code inspection).  It seems to be an array of interface names, which is a combination of "exactly what I got from the parser" (the name of the parent interface) with some post-processing (getting the rest of the ancestors from said parent interface).  As long as we're doing post-processing, we should, imo make things sane and make .inheritance be the parent interface object, null if there isn't one.  I agree it's not a high priority to do that.

But it does mean that the documentation you checked in for .inheritance is wrong.  It's never null, for a start.  :(

But thank you for documenting things!
(In reply to Boris Zbarsky (:bz) from comment #16)
> Sort of.  The specific issue I was having is that the .inheritance of an
> interface object is not the parent interface object (as I would have
> expected), nor is it an array of interface objects (as one might expect
> based on quick code inspection).  It seems to be an array of interface
> names, which is a combination of "exactly what I got from the parser" (the
> name of the parent interface) with some post-processing (getting the rest of
> the ancestors from said parent interface).  As long as we're doing
> post-processing, we should, imo make things sane and make .inheritance be
> the parent interface object, null if there isn't one.  I agree it's not a
> high priority to do that.

Actually, it is "exactly what I got from the parser", with no preprocessing:

  this.inheritance = obj.inheritance;
  http://dvcs.w3.org/hg/resources/file/6b1884d0eddb/idlharness.js#l653

Where obj is an object produced directly by the WebIDL parser, possibly with some extra idlharness-specific properties added.  (Nitpick: it was "this.inheritance = obj.inheritance ? obj.inheritance : [];" until yesterday, when I updated the parser to make it always a list instead of sometimes "", and that's arguably preprocessing.)

The reason it's an array is because the parser supports multiple inheritance for interfaces, which I believe follows an old WebIDL draft.  So it's a list of parent names, not a list of ancestors.  In practice it will have either zero or one entry, for a currently-valid IDL.  Now that I realize WebIDL no longer supports multiple inheritance, I'll make it string-or-null.

The reason it's a string is because the IDL items are processed sequentially, and it's legal for an earlier interface to inherit from a later one.  So to make it a reference to the actual parent interface would require a separate post-processing stage.  It would be possible to do that -- I already have to do it for partial interfaces and "implements".  But it would mean the meaning of the property would be different at different stages of code execution, which I'm not enthusiastic about.

In practice, the file isn't huge and I didn't expect a lot of people to hack on it, so I paid attention to the design of the external APIs but not the internal ones.  The newly-added docs should help, but it's still probably most efficient to check with Ms2ger or (preferably) me before making changes, to avoid wasting time.

> But it does mean that the documentation you checked in for .inheritance is
> wrong.  It's never null, for a start.  :(

The documentation didn't say it was ever null -- it was always an array, which might have had zero elements.  Now it can be either a string or null: http://dvcs.w3.org/hg/resources/rev/38a451f9ecfc
(In reply to :Aryeh Gregor from comment #17)
> The reason it's an array is because the parser supports multiple inheritance
> for interfaces, which I believe follows an old WebIDL draft.  So it's a list
> of parent names, not a list of ancestors.

Ugh, I did miss that :/
> The reason it's an array is because the parser supports multiple inheritance

Ah, OK.  I thought it was a list of ancestors because there was code in the harness that was treating it that way, for what it's worth.  Might want to fix that too.  :(

> The documentation didn't say it was ever null

http://dvcs.w3.org/hg/resources/rev/2d81e8368bb1 begged to differ, in the "Notes for peopl" part.  But that's not relevant now...
(In reply to Boris Zbarsky (:bz) from comment #19)
> Ah, OK.  I thought it was a list of ancestors because there was code in the
> harness that was treating it that way, for what it's worth.  Might want to
> fix that too.  :(

Hmm, that sounds like a bug.  I don't suppose you remember where.

> http://dvcs.w3.org/hg/resources/rev/2d81e8368bb1 begged to differ, in the
> "Notes for peopl" part.  But that's not relevant now...

That was talking about the property of the object generated by the WebIDL parser, not the IdlInterface object.  (It should actually say empty string, not null -- I misremembered what the parser did.)
> Hmm, that sounds like a bug.  I don't suppose you remember where.

I think I was thinking of the inherit_interface function, but I may have just misunderstood its goal...  It seems like the code around that will just do completely wacky things if an interface has a parent that's [NoInterfaceObject].

> That was talking about the property of the object generated by the WebIDL parser

Ah, ok.
(In reply to Boris Zbarsky (:bz) from comment #21)
> I think I was thinking of the inherit_interface function, but I may have
> just misunderstood its goal...  It seems like the code around that will just
> do completely wacky things if an interface has a parent that's
> [NoInterfaceObject].

Do you by any chance mean the code I gave r- for in bug 742191 comment 30?  :)
Yes, that code.  ;)

Why are we blocking commits to Gecko on that stuff?  :(
We probably shouldn't.  I'll change to r+ and request a followup.
Comment on attachment 668679 [details] [diff] [review]
Support stringifier operations (but not yet attributes!) on non-proxy bindings, and fix Object.prototype.toString for proxy bindings.

Review of attachment 668679 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +1047,5 @@
> +                                 "length": 0,
> +                                 "flags": "JSPROP_ENUMERATE",
> +                                 "pref": PropertyDefiner.getControllingPref(stringifier) }
> +                self.regular.append(toStringDesc)
> +                self.chrome.append(toStringDesc)

You should only append to either regular or chrome (if we expect we'll have ChromeOnly stringifiers).
Attachment #668679 - Flags: review?(peterv) → review+
Good catch; this patch predates self.chrome not including self.regular.  ;)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa2d8f5b8be
Whiteboard: [need review]
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/ffa2d8f5b8be
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.