1503142 - [BinAST] Error thrown while off-main-thread compilation causes assertion failure: reader_.cx_->isExceptionPending(), at js/src/frontend/BinTokenReaderMultipart.cpp:435

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

it checks reader_.cx_->isExceptionPending()
https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/js/src/frontend/BinTokenReaderMultipart.cpp#435
> BinTokenReaderMultipart::AutoBase::~AutoBase()
> {
>     // By now, the `AutoBase` must have been deinitialized by calling `done()`.
>     // The only case in which we can accept not calling `done()` is if we have
>     // bailed out because of an error.
>     MOZ_ASSERT_IF(initialized_, reader_.cx_->isExceptionPending());
> }

but if it's off-main-thread, the exception is not pending but compilation error is pending.
https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/js/src/vm/ErrorReporting.cpp#113

we should check if it's main thread or not, and if not, check pending compilation error instead.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Check for pending compile error on off-main-thread parsing BinAST.

checking pending exception should do different thing for main thread and helper thread.

  * Added JSContext::isCompileErrorPending to check if compile error is pending on helper thread
  * Modified BinTokenReaderBase::hasRaisedError to check JSContext::isCompileErrorPending on helper thread
  * Changed all places in BinAST parser/tokenizer which checks pending exception to check BinTokenReaderBase::hasRaisedError, so that it works also on helper thread

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 9021063 [details] [diff] [review]
Check for pending compile error on off-main-thread parsing BinAST.

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

frontend/Parser.cpp
3055-                syntaxParser->clearAbortedSyntaxParse();
3056-                usedNames.rewind(token);
3057-                MOZ_ASSERT_IF(!syntaxParser->context->helperThread(),
3058:                              !syntaxParser->context->isExceptionPending());
3059-                break;
3060-            }
3061-            return false;

vm/JSContext.cpp
1294-{
1295-#ifdef DEBUG
1296-    if (!helperThread()) {
1297:        MOZ_ASSERT(isExceptionPending());
1298-    }
1299-#endif
1300-    return mozilla::Err(reportedError);

wasm/AsmJS.cpp
7296-static bool
7297-NoExceptionPending(JSContext* cx)
7298-{
7299:    return cx->helperThread() || !cx->isExceptionPending();
7300-}
7301-

Seems to me we should add a helper function for this hasPendingError test somewhere, seeing as there are other places that should use this (and possibly more past this, I just skimmed *.cpp */*.cpp ack-hits for isExceptionPending).

::: js/src/frontend/BinTokenReaderBase.cpp
@@ +72,5 @@
>  BinTokenReaderBase::hasRaisedError() const
>  {
> +    if (cx_->helperThread()) {
> +        // When performin off-main-thread parsing, we don't set pending
> +        // exception but adds pending compile error.

When performing...set a pending exception but instead add a pending...

Comment 3

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Jeff Walden [:Waldo] from comment #2)
> Seems to me we should add a helper function for this hasPendingError test
> somewhere, seeing as there are other places that should use this (and
> possibly more past this, I just skimmed *.cpp */*.cpp ack-hits for
> isExceptionPending).

Okay, I'll file another bug for it.

Comment 4

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/598f4654a06cb0bc7e1525e61b7d97acd6ba0d51
Bug 1503142 - Check for pending compile error on off-main-thread parsing BinAST. r=Waldo

Comment 5

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/598f4654a06c