605274 - JM: Check for OOM in data structure usage

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

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.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Part 1

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.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 3

[David Mandelin [:dmandelin]]

Part 1: http://hg.mozilla.org/tracemonkey/rev/d365a5745be8

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Delayed OOM check patch

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.

Comment 5

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/da076e4b0ad8

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/da076e4b0ad8