Last Comment Bug 693258 - Figure out test failures when turning off new list DOM bindings
: Figure out test failures when turning off new list DOM bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Peter Van der Beken [:peterv]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 830342
Blocks: 648801
  Show dependency treegraph
 
Reported: 2011-10-10 03:06 PDT by Peter Van der Beken [:peterv]
Modified: 2013-01-15 17:05 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.53 KB, patch)
2011-10-11 02:09 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v2 (8.93 KB, patch)
2011-10-11 12:43 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2011-10-10 03:06:44 PDT
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).
Comment 1 Peter Van der Beken [:peterv] 2011-10-10 04:32:29 PDT
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
Comment 2 Peter Van der Beken [:peterv] 2011-10-11 02:09:04 PDT
Created attachment 566149 [details] [diff] [review]
v1
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-10-11 02:58:19 PDT
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;
}
Comment 4 Peter Van der Beken [:peterv] 2011-10-11 03:43:27 PDT
I don't, because we want to remove those lines when we remove the pref, but not the others.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-10-11 04:18:15 PDT
Good point
Comment 6 Peter Van der Beken [:peterv] 2011-10-11 12:43:10 PDT
Created attachment 566304 [details] [diff] [review]
v2

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.
Comment 7 Peter Van der Beken [:peterv] 2011-10-11 12:45:04 PDT
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.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-10-11 13:05:41 PDT
> 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?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-10-11 13:08:22 PDT
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.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-10-11 13:44:59 PDT
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 11 Boris Zbarsky [:bz] (still a bit busy) 2011-10-11 13:46:04 PDT
Comment on attachment 566304 [details] [diff] [review]
v2

r=me but please file a followup on simplifying that id code...
Comment 12 Peter Van der Beken [:peterv] 2011-10-12 01:28:02 PDT
(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 13 Luke Wagner [:luke] 2011-10-12 10:37:19 PDT
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.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-19 14:50:05 PDT
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.
Comment 15 Peter Van der Beken [:peterv] 2011-10-20 05:01:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/da852a7882b5
Comment 16 Marco Bonardo [::mak] 2011-10-21 02:10:06 PDT
https://hg.mozilla.org/mozilla-central/rev/da852a7882b5

Note You need to log in before you can comment on or make changes to this bug.