Firefox crashes on BinAST encoded version of Facebook.com

RESOLVED FIXED in Firefox 64

Status

()

defect
P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: vladan, Assigned: efaust)

Tracking

(Blocks 2 bugs)

Trunk
mozilla64
Unspecified
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 fixed)

Details

Attachments

(5 attachments)

Reporter

Description

a year ago
Posted file 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
Do you have a stack at hand, by any chance?
Reporter

Comment 2

a year ago
I don't have one atm, but I'll get one tomorrow
Reporter

Updated

a year ago
Flags: needinfo?(vladan.bugzilla)
Flags: needinfo?(jcoppeard)
Reporter

Comment 3

a year ago
Posted file Crash Stack
Flags: needinfo?(vladan.bugzilla)
Assignee

Comment 4

a year ago
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 :)
At first glance it looks like BinTokenReaderBase::raiseError needs to be more like GeneralParser::error and call ReportCompileError instead of JS_ReportErrorASCII.
Assignee

Comment 6

a year ago
Yup. Will patch.
Assignee

Updated

a year ago
Blocks: 1444956
As suggested I've landed bug 1444956 with off-thread decoding disabled in CanDoOffThread.
Flags: needinfo?(jcoppeard)
Priority: -- → P3
Assignee

Comment 8

8 months ago
Assignee: nobody → efaustbmo
Attachment #9012841 - Flags: review?(arai.unmht)
Assignee

Updated

8 months ago
Status: NEW → ASSIGNED
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?
Attachment #9012841 - Flags: review?(arai.unmht) → review+
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.
Attachment #9012842 - Flags: review?(arai.unmht) → review+
Comment on attachment 9012843 [details] [diff] [review]
[Part 3] Re-enable off-main-thread BinAST parsing

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

\o/
Attachment #9012843 - Flags: review?(arai.unmht) → review+

Comment 14

8 months ago
Pushed by efaustbmo@gmail.com:
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 16

8 months ago
Pushed by efaustbmo@gmail.com:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1487cec16a9c
https://hg.mozilla.org/mozilla-central/rev/ac5bef60b83b
https://hg.mozilla.org/mozilla-central/rev/3a7453938961
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.