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

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

So, the exception is set from here:
https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/js/src/jsscript.cpp#1055

The code path leading to that is:
https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/js/src/vm/Xdr.cpp#136
https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/js/src/jsapi.cpp#6127
https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/js/xpconnect/loader/mozJSLoaderUtils.cpp#67
https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/js/xpconnect/loader/mozJSComponentLoader.cpp#899

And this is where things go wrong: there are many ways an error can be returned by WriteCachedScript, and only one of them sets an exception pending. But the code that follows explicitly doesn't treat the failure as fatal, but leaves the exception pending.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Clear pending exception from script cache writing failure

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Clear pending exception from script cache writing failure

Is this what you had in mind?

Comment 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Clear pending exception from script cache writing failure

Let's go with this then.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83a0a377faa4

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83a0a377faa4

Comment 9

[Mike Hommey [:glandium]]

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

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0607729b3d90

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ed535350b36

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0607729b3d90