Closed Bug 605274 Opened 14 years ago Closed 14 years ago

JM: Check for OOM in data structure usage

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dmandelin, Assigned: dmandelin)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

We use |.append()| a bunch without checking for OOM. This could be causing topcrashes. We need to fix this.

- Some functions usage js::Vector just to make a short list of up to, say, 5 elements. These don't need to check but might want a static assert on the inline storage length.

- Functions that create vectors of unbounded size need to check for OOM. I think most of our vectors have a short lifetime (e.g., while compiling), so we can use ContextAllocPolicy to get the OOMs reported. We just need to abort compilation/PIC generation/whatever and propagate the return value out to the top.
Attached patch Part 1Splinter Review
Here's part 1. It does just one call site and propagates the values all the way through. I figured I'd get a review on part 1 before going ahead and doing the same thing all the way through.
Assignee: general → dmandelin
Status: NEW → ASSIGNED
Attachment #487047 - Flags: review?(dvander)
Comment on attachment 487047 [details] [diff] [review]
Part 1

>-    else
>-        branchPatches.append(BranchPatch(j, pc));
>+        return true;
>+    } else {
>+        return branchPatches.append(BranchPatch(j, pc));
>+    }

No return after else, just remove the else and unindent the return.

>         if (test->isTypeKnown()) {
>-            emitStubCmpOp(stub, target, fused);
>-            return;
>+            return emitStubCmpOp(stub, target, fused);
>         }

Can get rid of the curly braces here.

>-            jsop_equality(op, stub, target, fused);
>+            return jsop_equality(op, stub, target, fused);
>         else
>-            emitStubCmpOp(stub, target, fused);
>-        return;
>+            return emitStubCmpOp(stub, target, fused);

return after else

>     if (op == JSOP_EQ || op == JSOP_NE) {
>         if ((lhs->isNotType(JSVAL_TYPE_INT32) && lhs->isNotType(JSVAL_TYPE_STRING)) ||
>             (rhs->isNotType(JSVAL_TYPE_INT32) && rhs->isNotType(JSVAL_TYPE_STRING))) {
>-            emitStubCmpOp(stub, target, fused);
>+            return emitStubCmpOp(stub, target, fused);
>         } else if (!target && (lhs->isType(JSVAL_TYPE_STRING) || rhs->isType(JSVAL_TYPE_STRING))) {
>-            emitStubCmpOp(stub, target, fused);
>+            return emitStubCmpOp(stub, target, fused);
>         } else if (frame.haveSameBacking(lhs, rhs)) {
>-            emitStubCmpOp(stub, target, fused);
>+            return emitStubCmpOp(stub, target, fused);
>         } else {
>-            jsop_equality_int_string(op, stub, target, fused);
>+            return jsop_equality_int_string(op, stub, target, fused);
>         }

I don't even know what we're supposed to do here so r=me
Attachment #487047 - Flags: review?(dvander) → review+
Propagating OOM flags through everything was going to touch a lot of code. Worse:

  - Callers would need to know which functions return void and which bool, so
    that they check everything. This would probably be hard to keep clean going
    forward.

  - Functions that return a C struct value need something annoying in order to
    propagate out an OOM.

So, I went with a new strategy of flagging the OOM in the compiler, and checking it before we actually try to use a data structure that may have OOM'd. Fortunately, it turns out aside from one place with a traceIC where I had to use propagation, we only read the vectors in finishThisUp (including fixCrossJumps called by the latter), so only one check is needed.
Attachment #487427 - Flags: review?(dvander)
Attachment #487427 - Flags: review?(dvander) → review+
http://hg.mozilla.org/tracemonkey/rev/da076e4b0ad8
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/da076e4b0ad8
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: