Closed Bug 724993 Opened 10 years ago Closed 10 years ago

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


(Core :: SVG, defect)

Not set





(Reporter: jruderman, Assigned: jwatt)



(Keywords: assertion, testcase)


(2 files)

Attached file 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)
Assignee: nobody → jwatt
OS: Mac OS X → All
Hardware: x86_64 → All
Attached patch patch with testsSplinter Review
Attachment #595412 - Flags: review?(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...)
Attachment #595412 - Flags: review?(dholbert) → review+
It's "Suspicious: [hg revision]", "Suspicious: [pointervalue]". There's only a correctness issue here.
Er, s$ion]", "Sus$ion]", not "Sus$
Ahhh, of course, sorry for my confusion.
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.