If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

BaselineCompiler: Crash [@ Mark<js::types::TypeObject>] with OOM

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

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

Other Branch
mozilla23
x86_64
Linux
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsbugmon:], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
The following testcase crashes on baseline compiler branch revision 5fd27c1b3943 (run with --ion-eager):


function MyObject( value ) {}
gcparam("maxBytes", gcparam("gcBytes") + 4*(1));
gczeal(4);
function test() {}
var obj = new test();
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:]
(Reporter)

Comment 1

5 years ago
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
I was able to reproduce this on decoder's fuzzing machine. Baseline may have exposed the problem in this case, but I think it's a pre-existing issue.

We call js::InvokeConstructorKernel:

    args.setThis(MagicValue(JS_IS_CONSTRUCTING));

Then in StackFrame::prologue:

    if (isConstructing()) {
        RootedObject callee(cx, &this->callee());
        JSObject *obj = CreateThisForFunction(cx, callee, useNewType());
        if (!obj)
            return false;
        functionThis() = ObjectValue(*obj);
    }

If the CreateThisForFunction OOM's this will leave |this| == MagicValue(JS_IS_CONSTRUCTING).

Then in StackFrame::epilogue:

    if (isConstructing() && returnValue().isPrimitive())
        setReturnValue(ObjectValue(constructorThis()));

This sets the frame's return value slot to ObjectValue(0xa). (JS_IS_CONSTRUCTING == 0xa). With a debug build this would assert isObject() I think, but this is an opt build.
Created attachment 733928 [details] [diff] [review]
Patch

See the analysis in comment 2. Let me know if you can think of a better fix.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #733928 - Flags: review?(luke)
Comment on attachment 733928 [details] [diff] [review]
Patch

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

Stealing, as Luke's out and this is clearly fine.

::: js/src/vm/Stack.cpp
@@ +411,1 @@
>          setReturnValue(ObjectValue(constructorThis()));

Looking at this, we might also consider making constructorThis() just return a Value, and then there's no need for this extra check.  The return value of a function *should be* considered meaningless in cases like these, but I could imagine mistakes being made.
Attachment #733928 - Flags: review?(luke) → review+
...that is, using that return value as though it were real, not expecting this rare case where a MagicValue manifested itself.
Thanks for stealing the r? Waldo.

https://hg.mozilla.org/integration/mozilla-inbound/rev/25323c442d1a
https://hg.mozilla.org/mozilla-central/rev/25323c442d1a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.