As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 948647 - Assertion failure: JSVAL_IS_DOUBLE_IMPL(data), at dist/include/js/Value.h:1170 or Crash [@ exn_finalize] or Crash [@ ucol_close_50] with OOM
: Assertion failure: JSVAL_IS_DOUBLE_IMPL(data), at dist/include/js/Value.h:117...
Status: VERIFIED FIXED
[jsbugmon:update]
: assertion, crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla29
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz 912928
  Show dependency treegraph
 
Reported: 2013-12-10 13:54 PST by Christian Holler (:decoder)
Modified: 2014-03-26 02:39 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Make exn_finalize safe when obj's reserved slot was never initialized (due to OOM right after it was allocated). v1 (891 bytes, patch)
2013-12-10 15:39 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 1 - Make exn_finalize safe when obj's reserved slot was never initialized (due to OOM right after it was allocated). v2 (897 bytes, patch)
2013-12-10 15:40 PST, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 2 - Tidying. v2 (8.02 KB, patch)
2013-12-10 15:42 PST, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 3 - Change js_ErrorToException to return true iff cx->throwing was set, and document the convention. (5.18 KB, patch)
2013-12-10 15:44 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 3 - Change js_ErrorToException to return true iff cx->throwing was set, and document the convention, v3 (5.28 KB, patch)
2013-12-11 10:43 PST, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2013-12-10 13:54:43 PST
The following testcase asserts on mozilla-central revision df82be9d89a5 (run with --fuzzing-safe):


oomAfterAllocations(26); 
try { eval('syntax error'); } catch(exn) {}
Comment 1 User image Christian Holler (:decoder) 2013-12-10 13:55:32 PST
Jason is working on this one already \o/
Comment 2 User image Jason Orendorff [:jorendorff] 2013-12-10 15:39:24 PST
Created attachment 8345577 [details] [diff] [review]
Part 1 - Make exn_finalize safe when obj's reserved slot was never initialized (due to OOM right after it was allocated). v1
Comment 3 User image Jason Orendorff [:jorendorff] 2013-12-10 15:40:41 PST
Created attachment 8345578 [details] [diff] [review]
Part 1 - Make exn_finalize safe when obj's reserved slot was never initialized (due to OOM right after it was allocated). v2

D'oh, qref fail.
Comment 4 User image Jason Orendorff [:jorendorff] 2013-12-10 15:42:06 PST
Created attachment 8345580 [details] [diff] [review]
Part 2 - Tidying. v2

Rename js_ReportErrorAgain -> js::CallErrorReporter. Replace its check that message is not null with an assertion (and move the check to the one call site that needs it). Make frontend::CompileError::throwError call CallErrorReporter instead of including a copy of its body.

Also, delete unused API function JS_ThrowReportedError.
Comment 5 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-10 15:43:27 PST
Comment on attachment 8345578 [details] [diff] [review]
Part 1 - Make exn_finalize safe when obj's reserved slot was never initialized (due to OOM right after it was allocated). v2

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

Oops, I thought I handled this sufficiently with the init at the top of ErrorObject::init.  Maybe not quite.

::: js/src/vm/ErrorObject.h
@@ +76,5 @@
>          return JSExnType(getReservedSlot(EXNTYPE_SLOT).toInt32());
>      }
>  
>      JSErrorReport * getErrorReport() const {
> +        const Value &slot = getReservedSlot(ERROR_REPORT_SLOT);

HeapSlot& maybe?

@@ +78,5 @@
>  
>      JSErrorReport * getErrorReport() const {
> +        const Value &slot = getReservedSlot(ERROR_REPORT_SLOT);
> +        if (slot.isUndefined())
> +            return NULL;

nullptr

@@ +84,1 @@
>          return static_cast<JSErrorReport*>(priv);

I'd just one-liner this.
Comment 6 User image Jason Orendorff [:jorendorff] 2013-12-10 15:44:15 PST
Created attachment 8345581 [details] [diff] [review]
Part 3 - Change js_ErrorToException to return true iff cx->throwing was set, and document the convention.
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-10 15:58:55 PST
Comment on attachment 8345580 [details] [diff] [review]
Part 2 - Tidying. v2

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

30 lines down, eleventy billion exception-handling lines to go.
Comment 8 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-12-10 16:27:12 PST
Comment on attachment 8345581 [details] [diff] [review]
Part 3 - Change js_ErrorToException to return true iff cx->throwing was set, and document the convention.

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

::: js/src/jsexn.cpp
@@ +671,4 @@
>      RootedString messageStr(cx, reportp->ucmessage ? JS_NewUCStringCopyZ(cx, reportp->ucmessage)
>                                                     : JS_NewStringCopyZ(cx, message));
>      if (!messageStr)
> +        return true;

Shouldn't this, and all the |return true;| in here, be |return cx->isExceptionPending();|?  Why can't this fail via too-much-recursion, which could return false without setting an exception?

::: js/src/jsexn.h
@@ +28,5 @@
> + * Return true if cx->throwing and cx->exception were set.
> + *
> + * This means that:
> + *
> + *   - If the error is successfuly converted to an exception and stored in

successfully
Comment 9 User image Jason Orendorff [:jorendorff] 2013-12-10 16:53:38 PST
Sigh. Yeah, back to the drawing board.
Comment 10 User image Jason Orendorff [:jorendorff] 2013-12-11 10:43:25 PST
Created attachment 8346045 [details] [diff] [review]
Part 3 - Change js_ErrorToException to return true iff cx->throwing was set, and document the convention, v3

On second thought, I think taking Jeff's suggestion is best.
Comment 11 User image Christian Holler (:decoder) 2013-12-16 04:11:17 PST
Shall we land this now? I can do it if nobody else has time :)
Comment 14 User image Ioana (away) 2014-03-26 02:39:42 PDT
Reproduced with the 12/10 mozilla-central ASAN JS shell. Verified as fixed using the 03/25 mozilla-beta ASAN JS shell - I only get "out of memory" messages now.

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