Closed Bug 1504788 Opened 2 years ago Closed 2 years ago

[BinAST] Nightly Crashes when repeatedly reloading Instagram.com

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: efaust, Assigned: efaust)

Details

Attachments

(3 files)

This got reported internally at Facebook. Luckily, the fixes aren't terribly difficult, and all have to do with caching.
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.
Attachment #9022708 - Flags: review?(amarchesini)
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.
Attachment #9022711 - Flags: review?(jwalden+bmo)
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.
Attachment #9022708 - Flags: review?(amarchesini) → review+
Attachment #9022709 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #9022711 - Flags: review?(jwalden+bmo) → review+
Pushed by efaustbmo@gmail.com:
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)
Pushed by efaustbmo@gmail.com:
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)
Relanded after fixing locally, seems OK this time.
Flags: needinfo?(efaustbmo)
https://hg.mozilla.org/mozilla-central/rev/dc8c7d20444a
https://hg.mozilla.org/mozilla-central/rev/a377c59bcfd1
https://hg.mozilla.org/mozilla-central/rev/1bd06e0c74f5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.