Last Comment Bug 658746 - "Assertion failure: JSID_IS_STRING(id)" with dataset[0]
: "Assertion failure: JSID_IS_STRING(id)" with dataset[0]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86_64 Mac OS X
-- critical (vote)
: mozilla6
Assigned To: William Chen [:wchen]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 326633 dataset
  Show dependency treegraph
Reported: 2011-05-20 21:41 PDT by Jesse Ruderman
Modified: 2013-12-27 14:31 PST (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (asserts fatally when loaded) (77 bytes, text/html)
2011-05-20 21:41 PDT, Jesse Ruderman
no flags Details
stack trace (mac64, debug, 21c304c5f351) (8.54 KB, text/plain)
2011-05-20 21:43 PDT, Jesse Ruderman
no flags Details
Bug fix (7.29 KB, patch)
2011-05-23 10:28 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Bug fix (8.28 KB, patch)
2011-05-23 12:05 PDT, William Chen [:wchen]
jonas: review+
Details | Diff | Splinter Review
exported patch (8.36 KB, patch)
2011-05-23 15:55 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review

Description User image Jesse Ruderman 2011-05-20 21:41:47 PDT
Created attachment 534194 [details]
testcase (asserts fatally when loaded)
Comment 1 User image Jesse Ruderman 2011-05-20 21:43:48 PDT
Created attachment 534195 [details]
stack trace (mac64, debug, 21c304c5f351)
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2011-05-20 21:47:41 PDT
Uh... yes.  Looking at the code from bug 560112 this bit in nsDOMStringMapSH::NewResolve is just bogus:

>+  nsDependentJSString prop(id);

There needs to be a check for it being a string id first.

Similar for DelProperty, GetProperty, SetProperty,
Comment 3 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-20 21:50:42 PDT
Yup, we need to fix this for FF6. Should be trivial.
Comment 4 User image William Chen [:wchen] 2011-05-22 12:40:08 PDT
It's not explicitly stated in the spec what we should do when trying to set a non-string property of the dataset object since it can't correspond to an element attributes. I'm thinking we should throw some kind of exception. Alternatively we could try and treat them as expando properties. Any thoughts?
Comment 5 User image :Ms2ger (⌚ UTC+1/+2) 2011-05-22 12:52:48 PDT
Those should be expandos, if I read WebIDL+ES5 right.
Comment 6 User image Jesse Ruderman 2011-05-22 14:21:23 PDT
Or maybe they should coerce to string? In pure JS, x[0] and x["0"] tend to be the same thing.

js> [5]["0"]

js> ({"0": 5})[0]
Comment 7 User image Cameron McCormack (:heycam) (away 25 Feb–5 Mar) 2011-05-22 14:33:15 PDT
Properties at the level of the DOMStringMap name getter/setter/deleter are all strings.  If they can come in to NewResolve as an int, then they should be converted into a string.
Comment 8 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-22 17:48:15 PDT
Javascript in general never makes a difference between foo[0] and foo["0"]. You can try this by doing:

a = ["i", "ii", "iii"];
alert(a["0"]); // Will alert "i"
b = { "x": "foo", "1": "bar" };
alert(b[1]);   // Will alert "bar"

If you try element.dataset["0"] you'll see that we internally, as an optimization, convert the id argument to an integer value.

So we definitely should convert integers to strings and then treat them as any other property.
Comment 9 User image William Chen [:wchen] 2011-05-23 10:28:22 PDT
Created attachment 534474 [details] [diff] [review]
Bug fix

Here is my patch. I have assumed that valid property accesses will have a jsid that is a string or int. I have read that jsid could also be an object but I have not found a way to access a property with an object jsid.
Comment 10 User image :Ms2ger (⌚ UTC+1/+2) 2011-05-23 10:30:34 PDT
I'm thinking you'd want a helper method...
Comment 11 User image William Chen [:wchen] 2011-05-23 12:05:13 PDT
Created attachment 534506 [details] [diff] [review]
Bug fix

Now with helper!
Comment 12 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-23 15:21:11 PDT
Comment on attachment 534506 [details] [diff] [review]
Bug fix

Review of attachment 534506 [details] [diff] [review]:

Awesome! Thanks for the quick fix.

::: dom/base/nsDOMClassInfo.cpp
@@ +8722,5 @@
>  }
> +PRBool
> +nsDOMStringMapSH::JSIDToProp(const jsid& aId, nsAString& aResult)

For new code like this, you can use bool instead of PRBool. Feel free to go either way though, but IMHO bool is easier on the eyes.

@@ +8729,5 @@
> +    aResult.AppendInt(JSID_TO_INT(aId));
> +  } else {
> +    aResult = nsDependentJSString(aId);
> +  }

This would be somewhat easier to follow as:

} else if (JSID_IS_STRING(aId)) {
} else {
  return PR_FALSE;
Comment 13 User image William Chen [:wchen] 2011-05-23 15:55:33 PDT
Created attachment 534602 [details] [diff] [review]
exported patch

Here is an exported patch with comments addressed.

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