Last Comment Bug 658746 - "Assertion failure: JSID_IS_STRING(id)" with dataset[0]
: "Assertion failure: JSID_IS_STRING(id)" with dataset[0]
Status: RESOLVED FIXED
: 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]
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
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 | Review
Bug fix (8.28 KB, patch)
2011-05-23 12:05 PDT, William Chen [:wchen]
jonas: review+
Details | Diff | Review
exported patch (8.36 KB, patch)
2011-05-23 15:55 PDT, William Chen [:wchen]
no flags Details | Diff | Review

Description Jesse Ruderman 2011-05-20 21:41:47 PDT
Created attachment 534194 [details]
testcase (asserts fatally when loaded)
Comment 1 Jesse Ruderman 2011-05-20 21:43:48 PDT
Created attachment 534195 [details]
stack trace (mac64, debug, 21c304c5f351)
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 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 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-20 21:50:42 PDT
Yup, we need to fix this for FF6. Should be trivial.
Comment 4 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 :Ms2ger 2011-05-22 12:52:48 PDT
Those should be expandos, if I read WebIDL+ES5 right.
Comment 6 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"]
5

js> ({"0": 5})[0]
5
Comment 7 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 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 Jonas Sicking (:sicking) PTO Until July 5th 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 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 :Ms2ger 2011-05-23 10:30:34 PDT
I'm thinking you'd want a helper method...
Comment 11 William Chen [:wchen] 2011-05-23 12:05:13 PDT
Created attachment 534506 [details] [diff] [review]
Bug fix

Now with helper!
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 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 @@
>    return NS_SUCCESS_I_DID_SOMETHING;
>  }
>  
> +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 {
> +    NS_ENSURE_TRUE(JSID_IS_STRING(aId), PR_FALSE);
> +    aResult = nsDependentJSString(aId);
> +  }

This would be somewhat easier to follow as:

} else if (JSID_IS_STRING(aId)) {
  ...
} else {
  return PR_FALSE;
}
Comment 13 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.