Closed Bug 693258 Opened 13 years ago Closed 13 years ago

Figure out test failures when turning off new list DOM bindings

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(1 file, 1 obsolete file)

I landed the new DOM bindings from bug 648801 initially turned off, but there were a couple of tests that now fail. I had fixed a number of these in the past, but apparently there's more again. We need to figure out what to do, preferably without changing the behaviour with the pref turned off (so that turning off the pref really brings us back to the state without the new DOM bindings).
Known failures:

content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/test_bug595449.html | fieldset.elements should always return the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/test_bug595449.html | fieldset.elements should always return the same object - got [object HTMLCollection], expected [object HTMLCollection]
dom/tests/mochitest/bugs/test_bug633133.html | empty string should be in the div collection
js/src/xpconnect/tests/chrome/test_nodelists.xul | in operator doesn't see phantom element
layout/reftests/css-ui-invalid/select/select-required-multiple-invalid-changed.html | assertion count 1 is more than expected 0 assertions
layout/reftests/css-ui-valid/select/select-required-multiple-valid-changed.html | assertion count 3 is more than expected 0 assertions
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Comment on attachment 566149 [details] [diff] [review]
v1

>--- a/js/src/xpconnect/src/dombindings.cpp
>+++ b/js/src/xpconnect/src/dombindings.cpp
>     JSObject *proto = getPrototype(cx, scope, triedToWrap);
>+    if (!proto && !*triedToWrap)
>+        aWrapperCache->ClearIsProxy();
>     if (!proto)
>         return NULL;

Personally, I'd write that as

if (!proto) {
  if (!*triedToWrap) {
    aWrapperCache->ClearIsProxy();
  }
  return NULL;
}
I don't, because we want to remove those lines when we remove the pref, but not the others.
Good point
Attached patch v2Splinter Review
Couple of problems.

I added a wrapper cache to all HTMLCollections, so we need to force the right parent using a PreCreate hook for those (otherwise we end up storing the first wrapper in the cache, and that wrapper can be from whatever scope comes first).

If the bindings are disabled we should unmark the IsProxy bit in the wrapper caches, because we won't be storing a proxy binding in it after all.

Make GetArrayIndexFromId do the |ToString(ToUint32(s)) == s| check from the WebIDL spec, but only for string properties. For properties that are not number and not string I kept the current behaviour. I think this fixes most issues (like empty string not ending up as 0) and we can try to remove the support for those other properties at some point in the future. It does mean that we're changing behaviour slightly (rejecting " 0" for example). Let me know if you think that's a problem, but running a bit out of ideas on how to fix this.

The pref check in test_nodelists.xul is unfortunate, but the alternative seems to be to disable the test.

Asking Luke for review on making js::StringIsArrayIndex public, since it does the ToString(ToUint32(s)) == s| check that's in WebIDL and so seems generally useful.
Attachment #566149 - Attachment is obsolete: true
Attachment #566304 - Flags: review?(luke)
Attachment #566304 - Flags: review?(bzbarsky)
I still need to file the "remove the dom.new_bindings pref" bug, but I'll mention that we should remove the ClearIsProxy API and the pref check in test_nodelists.xul at that point too.
> It does mean that we're changing behaviour slightly (rejecting " 0" for example)

Which is what webidl calls for, yes?

But I thought you copied the "get array index from id" code we used to have in classinfo into the new bindings.  Why was there a problem here?
Oh, and the pref check is just working around a bug in the old classinfo-based bindings, right?  I think the pref check is OK there.
Peter pointed out that the named array code in classinfo just went with a string lookup if the id is not JSID_IS_INT.  That answers comment 8.

For the rest, the key part is that jsid's int values have a different range than indices.  In particular, they can only be in the range:

1536 #define JSID_INT_MIN  (-(1 << 30))
1537 #define JSID_INT_MAX  ((1 << 30) - 1)

So foo[(1 << 30)] would end up with a string jsid representing a serialization of 1<<30.

In other words we do still need the call to StringIsArrayIndex.  In fact, we should strongly consider calling js_IdIsIndex, except it doesn't have the "length" special-case....  Maybe that doesn't matter, though?  We could explicitly check for that if we wanted, before the js_IdIsIndex call.  That all requires us limiting this to int/string ids, of course, which is probably a followup.

This didn't bite the old code in practice, since the difference between js_CheckForStringIndex and StringIsArrayIndex only shows up for pretty large indices and nodelists don't really ever have a billion nodes in them....
Comment on attachment 566304 [details] [diff] [review]
v2

r=me but please file a followup on simplifying that id code...
Attachment #566304 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #10)
> In other words we do still need the call to StringIsArrayIndex.  In fact, we
> should strongly consider calling js_IdIsIndex, except it doesn't have the
> "length" special-case....  Maybe that doesn't matter, though?

Both the length check and the checking for first char being a-z helped performance, because they are inlined and StringIsArrayIndex wasn't.

> We could
> explicitly check for that if we wanted, before the js_IdIsIndex call.  That
> all requires us limiting this to int/string ids, of course, which is
> probably a followup.

I'll file a bug.
Comment on attachment 566304 [details] [diff] [review]
v2

Sorry to give the runaround, but Waldo is in the middle of rejiggring the whole property string/element/non-element situation so I'd to defer to him on whether it makes sense to export this.
Attachment #566304 - Flags: review?(luke) → review?(jwalden+bmo)
Comment on attachment 566304 [details] [diff] [review]
v2

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

Temporary exporting makes sense.  Slightly longer-term, I don't think you'll need this when you implement different storage strategies for indexed properties and named properties.  So friend-only for now, and going away soon probably, makes sense.
Attachment #566304 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/da852a7882b5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 830342
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: