Closed Bug 1466626 Opened 2 years ago Closed 2 years ago

Add missing ReportOutOfMemory calls when using system- or zone-allocations

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

No description provided.
js/src/frontend/BinSource.cpp
BinASTParser<Tok>::buildFunctionBox(...)
- LifoAlloc doesn't report OOM errors to the context, so we need to call raiseOOM() manually.


js/src/frontend/BinSource.yaml
- Handle the case when BinASTParserBase::new_(...) returns nullptr.
- |parseContext_->positionalFormalParameterNames()| returns an AtomVector which uses SystemAllocPolicy [1] and SystemAllocPolicy doesn't report OOM errors to the context, so we need to call raiseOOM() here, too.

[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/js/src/frontend/NameAnalysisTypes.h#370


js/src/frontend/BinToken.cpp
- BinaryASTSupport::binKindMap_ and BinaryASTSupport::binVariantMap_ both use SystemAllocPolicy, so we need to handle OOM errors again manually.


js/src/frontend/BinTokenReaderMultipart.cpp
- BinTokenReaderMultipart::variantsTable_ also uses SystemAllocPolicy.


js/src/frontend/binsource/src/main.rs:
- CheckRecursionLimit(...) returns false when the recursion limit was reached, so we need BINJS_TRY here to actually detect stack overflows.
Attachment #8983144 - Flags: review?(dteller)
Forgot to mention:

I've used the following script to detect the OOM errors in part 1:
---
var list = [ /* bin-js files from jsapi-tests/binast */ ];
for (var name of list) {
    var ta = read("./js/src/jsapi-tests/binast/" + name, "binary");
    var format = name.includes("multipart") ? "multipart" : "simple";
    // format = "simple";
    var options = {format};

    oomTest(function(){
        for (var i = 0; i < 10; ++i)  {
            parseBin(ta.buffer, options);
        }
    });
}
---
Attached patch bug1466626-part2-js-oom.patch (obsolete) — Splinter Review
js/src/builtin/TypedObject.cpp
- ObjectWeakMap::init() calls WeakMap::init() which uses ZoneAllocPolicy, so we need to report OOM manually.
- InlineTransparentTypedObject::getOrCreateBuffer(...) is currently not called at all, so we can't create a regression test.


js/src/frontend/Parser.cpp
- possibleAnnexBFunctionBoxes_ is a FunctionBoxVector which uses SystemAllocPolicy, so we need to report OOM error manually, too.
- js/src/jit-test/tests/auto-regress/bug1466626-4.js covers this case.


js/src/jsapi.cpp
- JSErrorNotes::addNote{ASCII,Latin1,UTF8}(...) appends to JSErrorNotes::notes_ which uses SystemAllocPolicy.
- We're currently only adding a single error note into the notes_ vector and the notes_ vector has inline storage for a single note, so we can't create a regression test case here.


js/src/shell/js.cpp
- BufferStreamJob::bytes and BufferStreamState::jobs both uses SystemAllocPolicy.
- js::Thread::init(...) can fail theoretically, so we should handle this case by... err... crashing! Just like we already do here https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/js/src/shell/js.cpp#3705.


js/src/vm/JSCompartment.cpp
- JSRope::copyCharsInternal(...) uses a Vector with SystemAllocPolicy, so the callers need to report OOM errors manually.
- Callers to copyCharsInternal() are JSRope::copy{Latin1,TwoByte}Chars(), but those two are never called anywhere.
- And JSRope::copy{Latin1,TwoByte}CharsZ() which are only called from CopyStringPure().
- Normally I'd say copyCharsInternal() should call ReportOutOfMemory(), but it seems like its JSContext* parameter is allowed to be nullptr, so the callers of copyCharsInternal() need to handle OOM themselves.
- js/src/jit-test/tests/auto-regress/bug1466626-3.js is the regression test for this change.


js/src/vm/JSCompartment.cpp
- getOrCreateNonSyntacticLexicalEnvironment() calls ObjectWeakMap::init(), so we need to report OOM errors manually.
- js/src/jit-test/tests/auto-regress/bug1466626-2.js covers this change.


js/src/vm/JSContext.cpp
- js::JobQueue uses SystemAllocPolicy, so again we need to handle OOM errors manually.
- js/src/jit-test/tests/auto-regress/bug1466626-1.js is the regression test for this one.


js/src/vm/JSScript.cpp
- JSScript::ensureHasDebugScript() calls zone()->pod_calloc<...>(...), which requires manually OOM reporting.
- js::DebugScriptMap uses SystemAllocPolicy, so more manual OOM reporting needed.
- I wasn't able to create regression test cases for these changes, because there are more Debugger OOM exception issues present: The debugger clears pending OOM errors, but then continues execution with ResumeMode::Terminate, which means the interpreter goes into the error case, but then there's no longer a pending exception to throw, so oomTest() asserts again. For example this test case should still assert in oomTest():
---
var g = newGlobal();

var dbg = Debugger(g);
var handler = {
    hit: function (frame) {}
};

dbg.onDebuggerStatement = function (frame) {
    frame.script.setBreakpoint(0, handler);
};

oomTest(function() {
    g.eval("debugger;");
});
---
Attachment #8983155 - Flags: review?(jcoppeard)
Blocks: 912928
Comment on attachment 8983155 [details] [diff] [review]
bug1466626-part2-js-oom.patch

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

Thanks for doing this!

> The debugger clears
> pending OOM errors, but then continues execution with ResumeMode::Terminate,
> which means the interpreter goes into the error case, but then there's no
> longer a pending exception to throw, so oomTest() asserts again.

You can pass false as a second argument to oomTest() to suppress this check, which was added for this reason.

::: js/src/vm/JSCompartment.cpp
@@ +282,5 @@
>      }
>  
>      if (str->hasLatin1Chars()) {
>          ScopedJSFreePtr<Latin1Char> copiedChars;
> +        if (!str->asRope().copyLatin1CharsZ(cx, copiedChars)) {

The convention is that if a function/method takes a JSContext* argument then it should report OOM itself (this doesn't seem to apply to make_unique though).

I think we should make the copy*Chars methods do this.
Attachment #8983155 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #4)
> this doesn't seem to apply to make_unique though

I looked further into this and it seems that cx->make_unique() does already report out of memory, so you patch will report it twice in some cases.  I think we should change this so we only report once.
(In reply to Jon Coppeard (:jonco) from comment #5)
> (In reply to Jon Coppeard (:jonco) from comment #4)
> > this doesn't seem to apply to make_unique though
> 
> I looked further into this and it seems that cx->make_unique() does already
> report out of memory, so you patch will report it twice in some cases.  I
> think we should change this so we only report once.

Do you mean to split them into two OOM-checks? For example instead of:
---
    auto map = cx->make_unique<ObjectWeakMap>(cx);
    if (!map || !map->init()) {
        ReportOutOfMemory(cx);
        return nullptr;
    }
---

it should rather be:
---
    auto map = cx->make_unique<ObjectWeakMap>(cx);
    if (!map)
        return nullptr;
    if (!map->init()) {
        ReportOutOfMemory(cx);
        return nullptr;
    }
---
(In reply to Jon Coppeard (:jonco) from comment #4)
> ::: js/src/vm/JSCompartment.cpp
> @@ +282,5 @@
> >      }
> >  
> >      if (str->hasLatin1Chars()) {
> >          ScopedJSFreePtr<Latin1Char> copiedChars;
> > +        if (!str->asRope().copyLatin1CharsZ(cx, copiedChars)) {
> 
> The convention is that if a function/method takes a JSContext* argument then
> it should report OOM itself (this doesn't seem to apply to make_unique
> though).
> 
> I think we should make the copy*Chars methods do this.

Hmm okay, in that case I'll probably also simply rip out the unused feature that JSRope::copyCharsInternal(...)'s JSContext* parameter can be nullptr [1], if that's okay with you. (That feature was added in bug 893222, but it doesn't seem to be used anymore.)

[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/js/src/vm/StringType.cpp#316,318-319
(In reply to André Bargull [:anba] from comment #6)

> Do you mean to split them into two OOM-checks? For example instead of:

Yes, unfortunately.

> Hmm okay, in that case I'll probably also simply rip out the unused feature that 
> JSRope::copyCharsInternal(...)'s JSContext* parameter can be nullptr [1], if that's okay with you. 

Sounds good to me.
Attached patch bug1466626-part2-js-oom.patch (obsolete) — Splinter Review
Update part 2 per review comments, carrying r+.
Attachment #8983155 - Attachment is obsolete: true
Attachment #8983363 - Flags: review+
Fix more places where ReportOutOfMemory is called after JSContext allocation.
Attachment #8983369 - Flags: review?(jcoppeard)
Comment on attachment 8983369 [details] [diff] [review]
bug1466626-part3-split-oom-reporting.patch

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

Thanks for fixing these.
Attachment #8983369 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #8)
> (In reply to André Bargull [:anba] from comment #6)
> > Hmm okay, in that case I'll probably also simply rip out the unused feature that 
> > JSRope::copyCharsInternal(...)'s JSContext* parameter can be nullptr [1], if that's okay with you. 
> 
> Sounds good to me.

I spoke too soon, EqualStringsPure(...) and StoreStringChars(...) still pass nullptr for the context [1]. (Eclipse CDT didn't grok the templatized calls :-(


[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/js/src/vm/MemoryMetrics.cpp#90,100,157
Comment on attachment 8983144 [details] [diff] [review]
bug1466626-part1-binast-oom.patch

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

lgtm, thanks!

::: js/src/frontend/BinSource.yaml
@@ +578,5 @@
>  
>                  ParseContext::Scope lexicalScope(cx_, parseContext_, usedNames_);
>                  BINJS_TRY(lexicalScope.init(parseContext_));
>      build: |
> +        ParseNode* params;

If this works with BINJS_TRY_DECL (instead of a two line declaration), that would be a bit nicer.
Attachment #8983144 - Flags: review?(dteller) → review+
Updated to apply review comments, carrying r+.
Attachment #8983144 - Attachment is obsolete: true
Attachment #8987490 - Flags: review+
Updated part 2 to apply cleanly on inbound, carrying r+.
Attachment #8983363 - Attachment is obsolete: true
Attachment #8987491 - Flags: review+
Updated part 3 to apply cleanly on inbound, carrying r+.
Attachment #8983369 - Attachment is obsolete: true
Attachment #8987492 - Flags: review+
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8cb4420363
Part 1: Add missing OOM and stack overflow checks in bin-ast parser. r=yoric
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd737ab7af6f
Part 2: Add missing OOM handling in various places. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/3967e4340805
Part 3: Don't call ReportOutOfMemory twice when used with JSContext allocation. r=jonco
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c8cb4420363
https://hg.mozilla.org/mozilla-central/rev/fd737ab7af6f
https://hg.mozilla.org/mozilla-central/rev/3967e4340805
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Since it's only a few days since the merge to mozilla-beta, is this something you'd like to uplift for beta 62?  Or is it better/less risky to let it ride with 63?
Flags: needinfo?(andrebargull)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20)
> Since it's only a few days since the merge to mozilla-beta, is this
> something you'd like to uplift for beta 62?  Or is it better/less risky to
> let it ride with 63?

Hmm, theoretically it should be safe to uplift the patches, because the changes are all non-risky. But part 1 only touches BinAST things, which are only enabled in Nightly builds, so uplifting it to Beta isn't really necessary. Part 3 fixes some double OOM error reporting, so we only avoid overwriting one OOM error with another OOM error in the JSContext class. This is nice to have, but probably doesn't warrant an uplift. That leaves part 2, which adds missing OOM error reporting in code which is actually executed in Beta and Release. But when we want to uplift part 2, we either need to uplift bug 1471931, too (I don't think we want to do that), or alternatively update the patch to apply cleanly on Beta without the changes from bug 1471931 (Not difficult to do, because only minor changes are needed for this).
So yes, we could uplift one of the patches from this bug, but only after making some changes to that one patch. Therefore I'm not really sure it's worth the extra work for an uplift.
Flags: needinfo?(andrebargull)
Thanks for the great explanation! Let's keep that in 63 then.
You need to log in before you can comment on or make changes to this bug.