The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla29

Status

()

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

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

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

Trunk
mozilla29
x86_64
Linux
assertion, crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
The following testcase asserts on mozilla-central revision df82be9d89a5 (run with --fuzzing-safe):


oomAfterAllocations(26); 
try { eval('syntax error'); } catch(exn) {}
(Reporter)

Comment 1

3 years ago
Jason is working on this one already \o/
Assignee: general → jorendorff
Blocks: 912928
Keywords: crash
QA Contact: general
Whiteboard: [jsbugmon:update]
(Assignee)

Comment 2

3 years ago
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
Attachment #8345577 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

3 years ago
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.
Attachment #8345577 - Attachment is obsolete: true
Attachment #8345577 - Flags: review?(jwalden+bmo)
Attachment #8345578 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

3 years ago
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.
Attachment #8345580 - Flags: review?(jwalden+bmo)
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.
Attachment #8345578 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8345581 [details] [diff] [review]
Part 3 - Change js_ErrorToException to return true iff cx->throwing was set, and document the convention.
Attachment #8345581 - Flags: review?(jwalden+bmo)
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.
Attachment #8345580 - Flags: review?(jwalden+bmo) → review+
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
(Assignee)

Comment 9

3 years ago
Sigh. Yeah, back to the drawing board.
(Assignee)

Comment 10

3 years ago
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.
Attachment #8345581 - Attachment is obsolete: true
Attachment #8345581 - Flags: review?(jwalden+bmo)
Attachment #8346045 - Flags: review?(jwalden+bmo)
Attachment #8346045 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Comment 11

3 years ago
Shall we land this now? I can do it if nobody else has time :)
Crash Signature: [@ exn_finalize] [@ ucol_close_50]
Summary: Assertion failure: JSVAL_IS_DOUBLE_IMPL(data), at dist/include/js/Value.h:1170 or Crash [@ exn_finalize] with OOM → 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
(Assignee)

Comment 12

3 years ago
Sorry, it got delayed behind other stuff that kept flunking the try server :-P

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d152bc73ece
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e93efa64af
https://hg.mozilla.org/integration/mozilla-inbound/rev/327a6af06942
https://hg.mozilla.org/mozilla-central/rev/9d152bc73ece
https://hg.mozilla.org/mozilla-central/rev/e8e93efa64af
https://hg.mozilla.org/mozilla-central/rev/327a6af06942
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

3 years ago
Keywords: verifyme

Comment 14

3 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.