Assertion failure: !cx->asJSContext()->isExceptionPending(), at BytecodeCompiler.cpp:555 during make package

RESOLVED FIXED in Firefox 43, Firefox OS v2.5

Status

()

Core
XPConnect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed, firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
On a try build with bug 1224000 landed, I'm getting the following error during make package:

 17:08:33     INFO -  JavaScript warning: resource://app/modules/translation/cld-worker.js, line 0: Successfully compiled asm.js code (total compilation time 521ms; unable to store in cache due to internal error (consider filing a bug))
 17:08:34     INFO -  Assertion failure: !cx->asJSContext()->isExceptionPending(), at /builds/slave/try-l64-d-00000000000000000000/build/src/js/src/frontend/BytecodeCompiler.cpp:555

This happens while populate_cache.js runs, and doesn't happen before bug 1224000 because resource://app/* things were mistakenly skipped for all of browser/ for a long time (that's what bug 1224000 is about).

This didn't happen on cld-worker.js back when resource://app/* things were still processed, so I did some bisecting with a minimal testcase with xpcshell instead of doing a full blown make package (which would have needed patching to trigger the problem), and git pointed me to bug 1162187, more specifically, to https://hg.mozilla.org/mozilla-central/rev/4285b458c45c

FWIW, this is what the log for make package looks like around the cld-worker.js import before 4285b458c45c:
  resource:///modules/translation/TranslationDocument.jsm
  resource:///modules/translation/cld-worker.js
  JavaScript warning: resource:///modules/translation/cld-worker.js, line 0: Successfully compiled asm.js code (total compilation time 190ms; unable to store in cache due to internal error (consider filing a bug))
  JavaScript error: , line 0: Error: AsmJS modules are not yet supported in XDR serialization.
  resource:///modules/webrtcUI.jsm
(which means it goes through to the next file to import, in the same xpcshell script)

And on 4285b458c45c:
  resource:///modules/translation/cld-worker.js
  JavaScript warning: resource:///modules/translation/cld-worker.js, line 0: Successfully compiled asm.js code (total compilation time 216ms; unable to store in cache due to internal error (consider filing a bug))
  Assertion failure: !cx->asJSContext()->isExceptionPending(), at /mnt/work/mozilla-central/js/src/frontend/BytecodeCompiler.cpp:458

So it would seem that changeset broke some kinds of exception propagation.
(Assignee)

Comment 2

2 years ago
Created attachment 8689846 [details] [diff] [review]
Clear pending exception from script cache writing failure
Assignee: nobody → mh+mozilla
Attachment #8689846 - Flags: review?(bobbyholley)
Comment on attachment 8689846 [details] [diff] [review]
Clear pending exception from script cache writing failure

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

Please put this in WriteCachedScript. Ditch the nsresult local, and if JS_EncodeScript fails, just clear the pending exception and return NS_ERROR_FAILURE.

r=me with that.
Attachment #8689846 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 4

2 years ago
Created attachment 8691113 [details] [diff] [review]
Clear pending exception from script cache writing failure

Is this what you had in mind?
Attachment #8689846 - Attachment is obsolete: true
Attachment #8691113 - Flags: review?(bobbyholley)
Comment on attachment 8691113 [details] [diff] [review]
Clear pending exception from script cache writing failure

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

::: js/xpconnect/loader/mozJSLoaderUtils.cpp
@@ +73,5 @@
>  
>      MOZ_ASSERT(size);
>      nsresult rv = cache->PutBuffer(PromiseFlatCString(uri).get(), static_cast<char*>(data), size);
>      js_free(data);
> +    return NS_SUCCEEDED(rv) ? NS_OK : NS_ERROR_FAILURE;

Sorry, I didn't notice that PutBuffer was fallible. Plain old "return rv" here would be fine (no need to coerce all failures to NS_ERROR_FAILURE), or you can put |data| in ScopedJSFreePtr and then |return cache->PutBuffer(...)|. Up to you.
Attachment #8691113 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8691126 [details] [diff] [review]
Clear pending exception from script cache writing failure

Let's go with this then.
Attachment #8691113 - Attachment is obsolete: true
Attachment #8691126 - Flags: review+

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/83a0a377faa4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Comment 9

2 years ago
Comment on attachment 8691126 [details] [diff] [review]
Clear pending exception from script cache writing failure

Approval Request Comment
[Feature/regressing bug #]: bug 1162187 exposed a preexisting bug in startup cache writing.
[User impact if declined]: No direct user impact, but this patch is needed to land bug 1224000.
[Describe test coverage new/current, TreeHerder]: There's nothing really to test here. Bug 1224000 triggers a build failure on debug builds without this, and doesn't with.
[Risks and why]: extremely low risk as per bholley. The only bad thing that can happen is if the javascript context given to the WriteCachedScript function already has a pending exception (in which case it would be ignored), but that in itself would already be a bug.
[String/UUID change made/needed]: None
Attachment #8691126 - Flags: approval-mozilla-beta?
Attachment #8691126 - Flags: approval-mozilla-aurora?
Comment on attachment 8691126 [details] [diff] [review]
Clear pending exception from script cache writing failure

Needed for fix to decrease startup time and fix addon issues, regression from 38. Please uplift to aurora and beta.
Attachment #8691126 - Flags: approval-mozilla-beta?
Attachment #8691126 - Flags: approval-mozilla-beta+
Attachment #8691126 - Flags: approval-mozilla-aurora?
Attachment #8691126 - Flags: approval-mozilla-aurora+

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0607729b3d90
status-firefox44: --- → fixed

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/0ed535350b36
status-firefox43: --- → fixed
You need to log in before you can comment on or make changes to this bug.