Last Comment Bug 724993 - "ASSERTION: Item should only return null for out-of-bounds access" after explicitly adding null
: "ASSERTION: Item should only return null for out-of-bounds access" after expl...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jonathan Watt [:jwatt]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 326633 722071
  Show dependency treegraph
 
Reported: 2012-02-07 10:11 PST by Jesse Ruderman
Modified: 2012-02-09 10:28 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (135 bytes, text/html)
2012-02-07 10:11 PST, Jesse Ruderman
no flags Details
patch with tests (5.55 KB, patch)
2012-02-08 07:59 PST, Jonathan Watt [:jwatt]
dholbert: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-02-07 10:11:38 PST
Created attachment 595075 [details]
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] 2012-02-08 07:59:43 PST
Created attachment 595412 [details] [diff] [review]
patch with tests
Comment 2 Daniel Holbert [:dholbert] 2012-02-08 09:59:59 PST
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 (⌚ UTC+1/+2) 2012-02-08 10:08:36 PST
It's "Suspicious: [hg revision]", "Suspicious: [pointervalue]". There's only a correctness issue here.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-02-08 10:10:10 PST
Er, s$ion]", "Sus$ion]", not "Sus$
Comment 5 Daniel Holbert [:dholbert] 2012-02-08 10:11:43 PST
Ahhh, of course, sorry for my confusion.
Comment 6 Ed Morley [:emorley] 2012-02-09 10:28:36 PST
https://hg.mozilla.org/mozilla-central/rev/16952c8a6b43

Note You need to log in before you can comment on or make changes to this bug.