Closed
Bug 937083
Opened 11 years ago
Closed 11 years ago
Assertion failure: !cx->isExceptionPending(), at ../jscntxtinlines.h:223 with OOM in js::CallJSNative
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: decoder, Assigned: decoder)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(2 files)
418 bytes,
text/plain
|
Details | |
2.65 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
The following testcase asserts on mozilla-central revision bc8c1eb0f2ba (threadsafe build, run with --fuzzing-safe): oomAfterAllocations(0)('xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx');
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
I investigated this a bit, and the trace for [@ setPendingException] before the assertion looks like this (on m-c rev ): #0 JSContext::setPendingException (this=0x9495980, v=$jsval(-nan(0xfff85f7111de0))) at ../jscntxtinlines.h:362 #1 0x0845248e in js_ReportOutOfMemory (cxArg=0x9495980) at ../jscntxt.cpp:376 #2 0x086289c4 in JSRuntime::onOutOfMemory (this=0x947e628, p=0x0, nbytes=59, cx=0x9495980) at ../vm/Runtime.cpp:806 #3 0x080686ec in js::ThreadSafeContext::onOutOfMemory (this=0x9495980, p=0x0, nbytes=59) at ../../jscntxt.h:266 #4 0x0806c36e in js::MallocProvider<js::ThreadSafeContext>::malloc_ (this=0x9495980, bytes=59) at ../../vm/Runtime.h:608 #5 0x084e9640 in js_strtod (cx=0x9495980, s=0x9543698 u'x' <repeats 58 times>, send=0x954370c u"", ep=0xffffbc10, dp=0xffffbc00) at ../jsnum.cpp:1766 #6 0x084e8cd6 in CharsToNumber (cx=0x9495980, chars=0x9543698 u'x' <repeats 58 times>, length=58, result=0xffffbd58) at ../jsnum.cpp:1519 #7 0x084e8d89 in js::StringToNumber (cx=0x9495980, str='x' <repeats 58 times>, result=0xffffbd58) at ../jsnum.cpp:1531 #8 0x084e8e7d in js::NonObjectToNumberSlow (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbd58) at ../jsnum.cpp:1542 #9 0x084e9046 in js::ToNumberSlow (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbd58) at ../jsnum.cpp:1593 #10 0x084e9181 in js::ToNumberSlow (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbd58) at ../jsnum.cpp:1613 #11 0x0855afae in ToUint32SlowImpl<JSContext, js::ToNumberSlow, JS::Handle<JS::Value> > (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbdd4) at ../jsnum.cpp:1699 #12 0x084e9389 in js::ToUint32Slow (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbdd4) at ../jsnum.cpp:1709 #13 0x080651ee in JS::ToUint32 (cx=0x9495980, v=$jsval(-nan(0xfff85f711d890)), out=0xffffbdd4) at ../../jsapi.h:1186 #14 0x080f5d07 in OOMAfterAllocations (cx=0x9495980, argc=1, vp=0x94fc620) at ../builtin/TestingFunctions.cpp:807 #15 0x085f4fe1 in js::CallJSNative (cx=0x9495980, native=0x80f5c60 <OOMAfterAllocations(JSContext*, unsigned int, jsval*)>, args=...) at ../jscntxtinlines.h:220 Now this frame seems interesting to me: > #6 0x084e8cd6 in CharsToNumber (cx=0x9495980, chars=0x9543698 u'x' <repeats 58 times>, length=58, result=0xffffbd58) at ../jsnum.cpp:1519 The code there looks like this: > if (!js_strtod(cx, bp, end, &ep, &d) || SkipSpace(ep, end) != end) > *result = GenericNaN(); > else > *result = d; The call to [@ js_strtod] is failing due to OOM, but we don't propagate this failure properly it seems (CharsToNumber is void and *result is set to NaN). If I understand the code correctly, then we should somehow propagate the OOM. Needinfo from shu because he touched the code. If you're not the right person, please needinfo someone who is :) I'm also happy to write a patch for this once someone can roughly tell me what the preferred solution is.
Flags: needinfo?(shu)
Comment 3•11 years ago
|
||
Good catch, you're right that CharsToNumber doesn't propagate OOM correctly. Feel free to write the patch and r? me.
Flags: needinfo?(shu)
Assignee | ||
Comment 4•11 years ago
|
||
Patch, passes jit-tests and confirmed to fix the bug.
Comment 5•11 years ago
|
||
Comment on attachment 8343816 [details] [diff] [review] js-CharsToNumber-oom.patch Review of attachment 8343816 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: js/src/jsnum.cpp @@ +1516,5 @@ > */ > const jschar *ep; > double d; > + if (!js_strtod(cx, bp, end, &ep, &d)) { > + *result = GenericNaN(); There shouldn't be a need to set *result to anything if we're returning false. @@ +1517,5 @@ > const jschar *ep; > double d; > + if (!js_strtod(cx, bp, end, &ep, &d)) { > + *result = GenericNaN(); > + return false; Nit: bad indentation?
Attachment #8343816 -
Flags: review?(shu) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Adjusted as discussed on IRC, thanks for the quick review. https://hg.mozilla.org/integration/mozilla-inbound/rev/fa482552d1aa
https://hg.mozilla.org/mozilla-central/rev/fa482552d1aa
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 8•10 years ago
|
||
I get "ReferenceError: oomAfterAllocations is not defined" with both the 11/12 nightly and 02/11 beta shells (Ubuntu 13.04 x86). No assertions or any other issues.
Keywords: verifyme
Comment 9•10 years ago
|
||
If you want to verify this, you'll need a debug build. (The "oomAfterAllocations" testing function is debug-only.)
Flags: needinfo?(ioana.budnar)
Comment 10•10 years ago
|
||
Btw, oomAfterAllocations(N) testcases are notorious for being fragile, so I wouldn't recommend verifying them in general. N==0 here, so you can if you want.
Assignee | ||
Comment 11•10 years ago
|
||
I agree with Jesse here. In general I wouldn't waste resources on verifying non-security OOM errors.
Comment 12•10 years ago
|
||
Thanks guys! I'll leave this kind of bugs alone then :D
Flags: needinfo?(ioana.budnar)
You need to log in
before you can comment on or make changes to this bug.
Description
•