Closed
Bug 1504788
Opened 4 years ago
Closed 4 years ago
[BinAST] Nightly Crashes when repeatedly reloading Instagram.com
Categories
(Core :: JavaScript Engine, enhancement, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: efaust, Assigned: efaust)
Details
Attachments
(3 files)
2.59 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
5.72 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.73 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
This got reported internally at Facebook. Luckily, the fixes aren't terribly difficult, and all have to do with caching.
Assignee | ||
Comment 1•4 years ago
|
||
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)
Assignee | ||
Comment 2•4 years ago
|
||
Attachment #9022709 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 3•4 years ago
|
||
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 4•4 years ago
|
||
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+
Updated•4 years ago
|
Attachment #9022709 -
Flags: review?(jwalden+bmo) → review+
Comment 5•4 years ago
|
||
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)
Comment 7•4 years ago
|
||
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
Flags: needinfo?(efaustbmo)
Comment 8•4 years ago
|
||
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
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)
Assignee | ||
Comment 10•4 years ago
|
||
Relanded after fixing locally, seems OK this time.
Flags: needinfo?(efaustbmo)
Comment 11•4 years ago
|
||
bugherder |
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: 4 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in
before you can comment on or make changes to this bug.
Description
•