The default bug view has changed. See this FAQ.

Assertion failure: !cx->isExceptionPending(), at ../jscntxtinlines.h:223 with OOM in js::CallJSNative

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla28
x86
Linux
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
The following testcase asserts on mozilla-central revision bc8c1eb0f2ba (threadsafe build, run with --fuzzing-safe):


oomAfterAllocations(0)('xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx');
(Assignee)

Comment 1

3 years ago
Created attachment 830134 [details]
[crash-signature] Machine-readable crash signature
(Assignee)

Comment 2

3 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

3 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

3 years ago
Created attachment 8343816 [details] [diff] [review]
js-CharsToNumber-oom.patch

Patch, passes jit-tests and confirmed to fix the bug.
Assignee: general → choller
Status: NEW → ASSIGNED
Attachment #8343816 - Flags: review?(shu)

Comment 5

3 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

3 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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Assignee)

Updated

3 years ago
Blocks: 912928

Updated

3 years ago
Keywords: verifyme

Comment 8

3 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

3 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

3 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

3 years ago
I agree with Jesse here. In general I wouldn't waste resources on verifying non-security OOM errors.

Comment 12

3 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.