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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


oomAfterAllocations(0)('xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx');

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

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.

Comment 3

[Shu-yu Guo [:shu]]

Good catch, you're right that CharsToNumber doesn't propagate OOM correctly.

Feel free to write the patch and r? me.

Comment 4

[Christian Holler (:decoder)]

Attachment: js-CharsToNumber-oom.patch

Patch, passes jit-tests and confirmed to fix the bug.

Comment 5

[Shu-yu Guo [:shu]]

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?

Comment 6

[Christian Holler (:decoder)]

Adjusted as discussed on IRC, thanks for the quick review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa482552d1aa

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fa482552d1aa

Comment 8

[Ioana (away)]

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.

Comment 9

[Jesse Ruderman]

If you want to verify this, you'll need a debug build. (The "oomAfterAllocations" testing function is debug-only.)

Comment 10

[Jesse Ruderman]

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.

Comment 11

[Christian Holler (:decoder)]

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

Comment 12

[Ioana (away)]

Thanks guys! I'll leave this kind of bugs alone then :D