1707979 - Delazification Crashes, loading immutableFlags_ off null (0x38 crashes)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Prex-existing crash but frequency increased for Firefox 88 (already 62 crashes vs. 53 for the whole Firefox 87 cycle).

Crash report: https://crash-stats.mozilla.org/report/index/1675336d-9266-4288-bfec-507080210427

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::statementListItem js/src/frontend/Parser.cpp:8888
1 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::functionFormalParametersAndBody js/src/frontend/Parser.cpp:3461
2 xul.dll js::frontend::Parser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::standaloneLazyFunction js/src/frontend/Parser.cpp:3326
3 xul.dll js::frontend::DelazifyCanonicalScriptedFunction js/src/frontend/BytecodeCompiler.cpp:1145
4 xul.dll static JSFunction::delazifyLazilyInterpretedFunction js/src/vm/JSFunction.cpp:1548
5 xul.dll static JSFunction::getOrCreateScript js/src/vm/JSFunction.h:406
6 xul.dll static JSFunction::delazifyLazilyInterpretedFunction js/src/vm/JSFunction.cpp:1536
7 xul.dll Interpret js/src/vm/Interpreter.cpp:3260
8 xul.dll js::ExecuteKernel js/src/vm/Interpreter.cpp:776
9 xul.dll js::Execute js/src/vm/Interpreter.cpp:808

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

A good chunk of these are at offset 0x38 (56). A repeated theme of the parser crashes is the crashing lines calling pos().begin

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Edit This analysis seems to be incorrect. Not sure how, but I seem to have computed sizes wrong.

---

So, doing a little bit of offset investigation: Offset 0x38 would correspond to accessing a member of the u union inside a nullptr Token.

However, that's not super helpful, and ultimately raises more questions than provides answers. Inspecting the uses of Token, it appears that the only extant Tokens are the four allocated in TokenStreamAnyChars, and all pointers and references are part of that array.

Access to that array is managed through manipulation of the cursor_ member of TokenStreamAnyChars, but the only manipulation of that field is all masked by nTokensMask, which should mean that we never see a value outside of the array.

I'm going to request mindump access, as I think that's the next logical step to investigate.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

I have to walk back the above: I somehow goofed my offsets and sizes, which makes the above wrong.

Here are the values I'm getting from 0d0cc6a6168d, which is the revision from the crash report linked in Comment 1:

• sizeof(Token) == 32 == 0x20
• offsetof(Token,u) == 16 == 0x10

This rules out a Token de-reference hypothesis from above (which is nice; it means that at least one less confusing aspect).

Looking at the disassembly around the crashing instruction pointer in a crash here, and a crash in Bug 1706429, I see the following dereference sequence:

    7ff9bc9b48f8:	48 8b 41 28          	mov    0x28(%rcx),%rax
    7ff9bc9b48fc:	8b 48 38             	mov    0x38(%rax),%ecx

In the dump I can see rcx has a long live range, and according to MS documentation should be the first integer argument.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

See analysis in Bug 1706429, Comment 9.

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

So in Bug 1706429, Comment 9, my analysis led me to a JSFunction in the skipped-inner-function list that seems to report having a BaseScript according to Function flags, but has nullptr in the union.

Arai: This seems relevant to your delazification work; not sure if the above gives you any hypotheses.

Comment 11

[Florent Fayolle]

Hello,

For what it's worth, my crashes of yesterday (→ bug1705761) are solved by the update from 89.0b13 to 89.0b14 (Firefox DevEdition, Linux).

Cheers,
Florent

Comment 12

[Ted Campbell [:tcampbell]]

(In reply to Florent Fayolle from comment #11)

Hello,

For what it's worth, my crashes of yesterday (→ bug1705761) are solved by the update from 89.0b13 to 89.0b14 (Firefox DevEdition, Linux).

Cheers,
Florent

Thanks for the update. Nothing relevant changed between those versions which suggests what changed was the cache file was cleared on update and there is a risk the issue will reappear :(

Comment 13

[Michael Hohner]

Had another crash just now: https://crash-stats.mozilla.org/report/index/0c096d1e-31c5-4c91-b757-ecc230210520

This time it had nothing to do with dev-tools, but it still happend when executing JavaScript. It was solved again by SHIFT-reloading the page (and all scripts).

Comment 14

[Ted Campbell [:tcampbell]]

Attachment: Bug 1707979 - Prevent debugger exposing "ghost" functions. r?arai!

These placeholder functions should not be exposed to the debugger. This adds a
few more early-returns to parts of the debugger that might leak. Also add a
diagnostic assert to delazification to get more meaningful stack traces. This is
an investigation and I do not have a concrete testcase that produces crashes.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/725361ac47bc
Prevent debugger exposing "ghost" functions. r=arai

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/725361ac47bc

Comment 18

[Matthew Gaudet (he/him) [:mgaudet]]

Given that the patch on this bug seems to have ameliorated the problem, should we consider uplift? Just duped Bug 1717480, which was on release, here.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:tcampbell, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 6 duplicates.
:tcampbell, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:tcampbell, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.