Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 6

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: wchen)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla6
x86_64
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6+ fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 534194 [details]
testcase (asserts fatally when loaded)
(Reporter)

Comment 1

6 years ago
Created attachment 534195 [details]
stack trace (mac64, debug, 21c304c5f351)

Comment 2

6 years ago
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,
tracking-firefox6: --- → ?
Yup, we need to fix this for FF6. Should be trivial.
Assignee: nobody → wchen
tracking-firefox6: ? → +
(Assignee)

Comment 4

6 years ago
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?
Those should be expandos, if I read WebIDL+ES5 right.
(Reporter)

Comment 6

6 years ago
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
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.
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.
(Assignee)

Comment 9

6 years ago
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.
Attachment #534474 - Flags: review?(jonas)
I'm thinking you'd want a helper method...
(Assignee)

Comment 11

6 years ago
Created attachment 534506 [details] [diff] [review]
Bug fix

Now with helper!
Attachment #534474 - Attachment is obsolete: true
Attachment #534474 - Flags: review?(jonas)
Attachment #534506 - Flags: review?(jonas)
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;
}
Attachment #534506 - Flags: review?(jonas) → review+
(Assignee)

Comment 13

6 years ago
Created attachment 534602 [details] [diff] [review]
exported patch

Here is an exported patch with comments addressed.
(Reporter)

Comment 14

6 years ago
http://hg.mozilla.org/mozilla-central/rev/80540db3a269
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Target Milestone: --- → mozilla6

Updated

6 years ago
status-firefox6: --- → fixed
You need to log in before you can comment on or make changes to this bug.