724993 - "ASSERTION: Item should only return null for out-of-bounds access" after explicitly adding null

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Item should only return null for out-of-bounds access: 'PRUint32(aIndex) >= length', file dom/base/nsDOMClassInfo.cpp, line 10924

nsStringArraySH::GetProperty [dom/base/nsDOMClassInfo.cpp:10031]
XPC_WN_Helper_GetProperty [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:963]
js::CallJSPropertyOp [js/src/jscntxtinlines.h:361]
js_NativeGetInline [js/src/jsscopeinlines.h:301]
js_GetPropertyHelperInline [js/src/jsobj.cpp:5396]
js::Interpret [js/src/jsobjinlines.h:209]

Suspicious: 7b0c5dcf5bcb (bug 722071), 2cd5af3aca9a (bug 529328)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch with tests

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 595412 [details] [diff] [review]
patch with tests

>+function run_tests()
>+{
>+  document.getElementById('svg').pauseAnimations();

Doesn't look like this ^ is necessary. (there are no animations in the test AFAICT)

>+  ok(initializeThrowsFor(strings, null),
>+     "SVGStringList.initialize() should throw when passed null");
>+  ok(initializeThrowsFor(strings, ""),
>+     "SVGStringList.initialize() should throw when passed the empty string");

As a sanity-check for the test, it'd probably be worth adding a call to each "xxxThrowsFor" function with a *nonempty* string, like so:

  ok(!initializeThrowsFor(strings, "ThisIsARealString"),
     "SVGStringList.initialize() shouldn't throw when passed a nonempty string");

to be sure that (a) the underlying methods don't just always throw (likely already covered elsewhere, but worth checking while you're here), and (b) your xxxThrowsFor test-methods don't just always return true due to a bug in the test.

Maybe these would belong at the end, so they don't muck up the rest of the test by modifying the string list?

Otherwise, r=me.

Also, should this bug be marked as security-sensitive?  (Comment 0 sounds scary, but the patch makes it look less scary.  Not sure what "Suspicious: [pointervalue]" meant in comment 0, but those are definitely non-null values, and I'd hope we aren't somehow pulling those out of IsEmpty strings & operating on them...)

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It's "Suspicious: [hg revision]", "Suspicious: [pointervalue]". There's only a correctness issue here.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Er, s$ion]", "Sus$ion]", not "Sus$

Comment 5

[Daniel Holbert [:dholbert]]

Ahhh, of course, sorry for my confusion.

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/16952c8a6b43