1504788 - [BinAST] Nightly Crashes when repeatedly reloading Instagram.com

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Eric Faust [:efaust]]

This got reported internally at Facebook. Luckily, the fixes aren't terribly difficult, and all have to do with caching.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 1: Don't mistake request types when reloading bytecode cached BinAST

The structure of the logic is slightly changed here after the refactor, and I must admit I'm not sure if it matters. At any rate, this seems simpler, so maybe that's better.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Don't use forbidden operations when XDRing BinAST.

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 3: Properly initialize BinASTSourceMetadata when XDR decoding ScriptSource

If I had just made this a common method, like you said last time, this never would have happened. Still, I don't want to pay for the calloc unless it matters.

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 9022708 [details] [diff] [review]
Part 1: Don't mistake request types when reloading bytecode cached BinAST

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

::: dom/script/ScriptLoadHandler.cpp
@@ +300,5 @@
> +    if (altDataType.Equals(nsContentUtils::JSBytecodeMimeType())) {
> +      mRequest->SetBytecode();
> +      TRACE_FOR_TEST(mRequest->Element(), "scriptloader_load_bytecode");
> +      return NS_OK;
> +    } else {

Don't do an "} else {" after a return.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 9022711 [details] [diff] [review]
Part 3: Properly initialize BinASTSourceMetadata when XDR decoding ScriptSource

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

::: js/src/vm/JSScript.cpp
@@ +2571,5 @@
>              MOZ_TRY(xdr->codeUint32(&numStrings));
>  
>              if (mode == XDR_DECODE) {
>                  // Use calloc, since we're storing this immediately, and filling it might GC, to
>                  // avoid marking bogus atoms.

This is dodge.  If filling this might GC, we should do the GC-ful things manually here, then feed them all into an infallible constructor that properly GC-safely constructs its fields and initializes them consistent with the GC.  r=me for now, but this needs to be fixed.  calloc for memory that is non-trivially initialized is just wrong.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6dc8af5bca
Part 1: Allow reloads of bytecode cached BinAST. (r=baku)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fba05049f9c
Part 2: Don't use prohibited operations when XDRing BinAST. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed6a977ba83
Part 3: Properly initialize BinASTSourceMetadata when XDR decoding ScriptSource. (r=Waldo)

Comment 7

[Cosmin Sabou [:CosminS]]

Backed out for mochitest failures on test_script_loader_js_cache, test_script_loader_intercepted_js_cache.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&fromchange=8ed6a977ba83dfc52f46d4b9305e258a054c76c1&searchStr=mochitest&selectedJob=211319418&group_state=expanded

Failure log: 
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=211319418&repo=mozilla-inbound&lineNumber=5761
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=211319453&repo=mozilla-inbound&lineNumber=2927

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/97e52e72ecfe5058bd9b760d1157e2005f893858

Comment 8

[Cosmin Sabou [:CosminS]]

This caused also these wpt to fail: 
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=211325038&repo=mozilla-inbound&lineNumber=13280
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=211326759&repo=mozilla-inbound&lineNumber=3628

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc8c7d20444a
Part 1: Allow reloads of bytecode cached BinAST. (r=baku)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a377c59bcfd1
Part 2: Don't use prohibited operations when XDRing BinAST. (r=Waldo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd06e0c74f5
Part 3: Properly initialize BinASTSourceMetadata when XDR decoding ScriptSource. (r=Waldo)

Comment 10

[Eric Faust [:efaust]]

Relanded after fixing locally, seems OK this time.

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/dc8c7d20444a
https://hg.mozilla.org/mozilla-central/rev/a377c59bcfd1
https://hg.mozilla.org/mozilla-central/rev/1bd06e0c74f5