1459555 - Firefox crashes on BinAST encoded version of Facebook.com

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Vladan Djeric (:vladan)]

Attachment: consolelog.txt

This is a 100% reproducible crash from loading the BinAST versions of the following Facebook.com snapshots:

* https://vdjeric.github.io/fb_bench/newsfeed_bench.htm
* https://vdjeric.github.io/fb_bench/comments_bench.htm

Crash log attached, no Breakpad crash log cause it's a dev build.


How to reproduce the crash:

To reproduce:

1. Build mozilla-central with patches from bug 1454352 and bug 1444956
2. Toggle “dom.script_loader.binast_encoding.enabled” pref in about:config to true
3. Start webserver from binast/binast-server repo against the fb_bench/ directory in the same repo, as follows: ./serveBinAST.py ./fb_bench/
4.  Observe that the custom build crashes reliably on both of the following URLs if BinAST is enabled. It loads fine if BinAST is preffed off.

https://localhost:8443/comments_bench.htm
https://localhost:8443/newsfeed_bench.htm

Notes:

* The first URL is a simpler benchmark so might be a bit easier to debug
* The "simple.html" BinAST test case in the same repo does work end to end
* The files in that fb_bench directory are encoded with default encoder options, so that seems like an easy test

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Do you have a stack at hand, by any chance?

Comment 2

[Vladan Djeric (:vladan)]

I don't have one atm, but I'll get one tomorrow

Comment 3

[Vladan Djeric (:vladan)]

Attachment: Crash Stack

Comment 4

[Eric Faust [:efaust]]

Looks like we have two bugs:

1) We're not properly handling exceptions off-main-thread.
2) We've got an error in BinAST that's causing us to try to throw an exception :)

Comment 5

[Jon Coppeard (:jonco)]

At first glance it looks like BinTokenReaderBase::raiseError needs to be more like GeneralParser::error and call ReportCompileError instead of JS_ReportErrorASCII.

Comment 6

[Eric Faust [:efaust]]

Yup. Will patch.

Comment 7

[Jon Coppeard (:jonco)]

As suggested I've landed bug 1444956 with off-thread decoding disabled in CanDoOffThread.

Comment 8

[Eric Faust [:efaust]]

Attachment: [Part 1] Allow throwing BinAST errors off-main-thread

Comment 9

[Eric Faust [:efaust]]

Attachment: [Part 2] Improve off-main-thread BinAST performance

Comment 10

[Eric Faust [:efaust]]

Attachment: [Part 3] Re-enable off-main-thread BinAST parsing

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 9012841 [details] [diff] [review]
[Part 1] Allow throwing BinAST errors off-main-thread

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

::: js/src/js.msg
@@ +677,5 @@
>  MSG_DEF(JSMSG_BIGINT_INVALID_SYNTAX, 0, JSEXN_SYNTAXERR, "invalid BigInt syntax")
>  MSG_DEF(JSMSG_NOT_BIGINT, 0, JSEXN_TYPEERR, "not a BigInt")
>  MSG_DEF(JSMSG_BIGINT_NOT_SERIALIZABLE, 0, JSEXN_TYPEERR, "BigInt value can't be serialized in JSON")
> +
> +MSG_DEF(JSMSG_BINAST,                                    1, JSEXN_SYNTAXERR, "BinAST Parsing Error: {0}")

can you add "// BinAST" above this line?

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 9012842 [details] [diff] [review]
[Part 2] Improve off-main-thread BinAST performance

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

::: js/src/frontend/BinToken.cpp
@@ +89,2 @@
>  {
> +    MOZ_ASSERT(!cx->helperThread());

can you add a comment that explains this should be called in main thread before starting the helper thread, for off-main-thread case?
so it's clear what this assertion means.
(at a first glance, this looks like it's not initialized for helper thread case)

@@ +109,2 @@
>  {
> +    MOZ_ASSERT(!cx->helperThread());

same here.

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 9012843 [details] [diff] [review]
[Part 3] Re-enable off-main-thread BinAST parsing

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

\o/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f2f2bcd57d2
Part 1: Allow throwing BinAST errors off main thread. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccdbc1449e13
Part 2: Don't take locks in BinSourceRuntimeSuppert::*. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/90505ae78229
Part 3: Allow off-main-thread BinAST compilation. (r=arai)

Comment 15

[Narcis Beleuzu [:NarcisB]]

Backed out for build bustages on JSScript.cpp

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=202787142&repo=mozilla-inbound&lineNumber=12141

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1487cec16a9c
Part 1: Allow throwing BinAST errors off main thread. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5bef60b83b
Part 2: Don't take locks in BinSourceRuntimeSuppert::*. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a7453938961
Part 3: Allow off-main-thread BinAST compilation. (r=arai)

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1487cec16a9c
https://hg.mozilla.org/mozilla-central/rev/ac5bef60b83b
https://hg.mozilla.org/mozilla-central/rev/3a7453938961