Closed Bug 1438502 Opened 4 years ago Closed 4 years ago

Missing exception when CacheEntry_getBytecode returns false

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: anba, Assigned: nbp)

Details

Attachments

(1 file)

Test case 1:
---
var code = cacheEntry("");
oomTest(function() {
    offThreadDecodeScript(code);
});
---

Test case 2:
---
var code = cacheEntry("");
oomTest(function() {
    evaluate(code, {loadBytecode: true});
});
---

Both test cases assert with
---
Assertion failure: cx->isExceptionPending() (Thunk execution failed but no exception was raised - missing call to js::ReportOutOfMemory()?)
---

because no error is thrown when CacheEntry_getBytecode(...) returns nullptr.
Assignee: nobody → nicolas.b.pierron
Priority: -- → P3
Comment on attachment 8951248 [details] [diff] [review]
Add error reports to CacheEntry internal methods.

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

Thanks!

::: js/src/shell/js.cpp
@@ +1590,5 @@
>      ArrayBufferObject::BufferContents contents =
>          ArrayBufferObject::BufferContents::create<ArrayBufferObject::PLAIN>(buffer);
>      Rooted<ArrayBufferObject*> arrayBuffer(cx, ArrayBufferObject::create(cx, length, contents));
> +    if (!arrayBuffer) {
> +        JS_ReportErrorASCII(cx, "CacheEntry_setBytecode: Unable to create an array buffer to hold the bytecode");

ArrayBufferObject::create(...) will already have reported an error if |!arrayBuffer| is true, so this change is not needed.

@@ +1663,5 @@
>      if (args[0].isString()) {
>          code = args[0].toString();
>      } else if (args[0].isObject() && CacheEntry_isCacheEntry(&args[0].toObject())) {
>          cacheEntry = &args[0].toObject();
> +        code = CacheEntry_getSource(cx, cacheEntry);

Needs |if (!code) return false;| otherwise the pending error will be overwritten a few lines below.
Attachment #8951248 - Flags: review?(andrebargull) → review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd98003fc0f
Add error reports to CacheEntry internal methods. r=anba
Backed out for spidermonkey build bustages at /builds/worker/workspace/sm-package/mozjs-60.0a1.0/js/src/jit-test/tests/self-test/cacheEntry.js:9

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cfd98003fc0f68673f6053d2800e3d0fa99868a8

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162441697&repo=mozilla-inbound&lineNumber=136944

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5626666b65bb27fe974d42b0997c47ec4e1a1daa
Flags: needinfo?(nicolas.b.pierron)
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e48b1c8130b9
Add error reports to CacheEntry internal methods. r=anba
https://hg.mozilla.org/mozilla-central/rev/e48b1c8130b9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.