Closed
Bug 605274
Opened 14 years ago
Closed 14 years ago
JM: Check for OOM in data structure usage
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dmandelin, Assigned: dmandelin)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files)
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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 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+
Assignee | ||
Comment 3•14 years ago
|
||
Part 1: http://hg.mozilla.org/tracemonkey/rev/d365a5745be8
Assignee | ||
Comment 4•14 years ago
|
||
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)
![]() |
||
Updated•14 years ago
|
Attachment #487427 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 5•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/da076e4b0ad8
Whiteboard: fixed-in-tracemonkey
Comment 6•14 years ago
|
||
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.
Description
•