1122361 - "Assertion failure: reportp" (js_ReportUncaughtException) with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: stack

oomAfterAllocations(0);
({});

Assertion failure: reportp, at /Users/jruderman/trees/mozilla-central/js/src/jscntxt.cpp:868

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/a2f5fa870c8a
user:        Boris Zbarsky
date:        Fri Jul 04 01:24:54 2014 -0400
summary:     Bug 966452 part 1.  Refactor the js_ReportUncaughtException to produce a (message, JSErrorReport*) pair before reporting.  r=waldo and including the fix for bug 1034616 to fix JS tests to deal with this, r=jorendorff.  r=terrence on the AutoStableStringChars bits

Comment 1

[Boris Zbarsky [:bzbarsky]]

I bet we landed in populateUncaughtExceptionReportVA, js_ExpandErrorArguments returned false, so we never set reportp but also didn't return false from ErrorReport::init.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Deal with populateUncaughtExceptionReport failing

Comment 3

[Christian Holler (:decoder)]

This blocks all further OOM testing right now, as it's a very frequently triggering bug.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8550099 [details] [diff] [review]
Deal with populateUncaughtExceptionReport failing

Review of attachment 8550099 [details] [diff] [review]:
-----------------------------------------------------------------

I assume a patch here will need backporting eventually, right?  Feel free to consider this an r+ on this patch for backporting purposes, but on trunk we have to do better.

::: js/src/jsexn.cpp
@@ +857,5 @@
>          populateUncaughtExceptionReport(cx, message_);
> +        if (!reportp) {
> +            // Just give up.  We're out of memory or something, not
> +            // much we can do here.
> +            return false;

So this seems fine enough.  But.  This is totally unreadable with reportp not actually being the local variable I expected it was on initial read of the code.

Please convert populateUncaughtExceptionReport to return a bool indicating success/failure the normal way.  That makes clear that there's a failure that's possibly been rectified, justifying the double-check of what is (at this level of code-reading) apparently a local variable.  Also remove it and the VA form from jsfriendapi.h (noticed just now via MXR) while you're in the area.  Those must be detritus from something, because they appear unused now.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Also remove it and the VA form from jsfriendapi.h

These are private member methods of the ErrorReport class, and while these methods are not used from outside the JS engine (being private and all), the class itself is used.

Did you want me to make them static methods or something?  But they assign to the class-member reportp....

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> These are private member methods of the ErrorReport class, and while these
> methods are not used from outside the JS engine (being private and all), the
> class itself is used.

Oh, sigh.  Never mind, MXR search results weren't showing the leading whitespace on the line that would have reminded me this was a class method, and that the class itself would of course still be used externally.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Deal with populateUncaughtExceptionReport failing

Updated to review comments.  I don't think we need to worry about backporting this, but if we do this should backport fine, I expect.

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0594d3bb9892

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0594d3bb9892