658746 - "Assertion failure: JSID_IS_STRING(id)" with dataset[0]

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack trace (mac64, debug, 21c304c5f351)

Comment 2

[Boris Zbarsky [:bzbarsky]]

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) No longer reading bugmail consistently]

Yup, we need to fix this for FF6. Should be trivial.

Comment 4

[William Chen [:wchen]]

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 (he/him; ⌚ UTC+1/+2)]

Those should be expandos, if I read WebIDL+ES5 right.

Comment 6

[Jesse Ruderman]

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)]

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) No longer reading bugmail consistently]

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]]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

I'm thinking you'd want a helper method...

Comment 11

[William Chen [:wchen]]

Attachment: Bug fix

Now with helper!

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]]

Attachment: exported patch

Here is an exported patch with comments addressed.

Comment 14

[Jesse Ruderman]

http://hg.mozilla.org/mozilla-central/rev/80540db3a269