1697952 - Crash in [@ js::frontend::CompilationStencil::instantiateStencilAfterPreparation]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/23f0c943-a23b-4e1a-b6be-634b50210311

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 9 frames of crashing thread:

0 XUL js::frontend::CompilationStencil::instantiateStencilAfterPreparation js/src/frontend/Stencil.cpp:1335
1 XUL js::frontend::InstantiateStencils js/src/frontend/BytecodeCompiler.cpp:386
2 XUL js::ParseTask::instantiateStencils js/src/vm/HelperThreads.cpp:723
3 XUL js::ScriptDecodeTask::parse js/src/vm/HelperThreads.cpp:836
4 XUL js::ParseTask::runHelperThreadTask js/src/vm/HelperThreads.cpp:619
5 XUL js::HelperThread::ThreadMain js/src/vm/HelperThreads.cpp:2366
6 XUL js::detail::ThreadTrampoline<void  js/src/threading/Thread.h:205
7 libsystem_pthread.dylib _pthread_start 
8 libsystem_pthread.dylib thread_start 

It looks like bug 1687095 fixed the big spike, but these crashes are still continuing in smaller number. They look like null derefs.

Comment 1

[Tooru Fujisawa [:arai]]

baseScript() is nullptr, when calling JSFunction::setEnclosingLazyScript

https://searchfox.org/mozilla-central/rev/9ae77e4ce3378bd683ac9a86b729ea6b6bd22cb8/js/src/frontend/Stencil.cpp#1187

static void LinkEnclosingLazyScript(const CompilationStencil& stencil,
                                    CompilationGCOutput& gcOutput) {
  for (auto item :
       CompilationStencil::functionScriptStencils(stencil, gcOutput)) {
...
    for (auto inner : script->gcthings()) {
      if (!inner.is<JSObject>()) {
        continue;
      }
      inner.as<JSObject>().as<JSFunction>().setEnclosingLazyScript(script);

https://searchfox.org/mozilla-central/rev/9ae77e4ce3378bd683ac9a86b729ea6b6bd22cb8/js/src/vm/JSFunction.h#455

class JSFunction : public js::NativeObject {
...
  void setEnclosingLazyScript(js::BaseScript* enclosingScript) {
    baseScript()->setEnclosingScript(enclosingScript);

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1697952 - Avoid null-deref in LinkEnclosingLazyScript. r?tcampbell!

Comment 3

[Ted Campbell [:tcampbell]]

We have a spot-fix proposed that should be quite safe for uplift. If crash is not how we expected, this null-ptr crash will move slightly later, but we will not be worse off.

We have a few ideas and test cases that we will continue to fix in central, but we are not sure they will solve this crash and those fixes are higher risk.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e5e4a8951ff
Avoid null-deref in LinkEnclosingLazyScript. r=tcampbell

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/7e5e4a8951ff

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance - it doesn't seem likely we're going to get a lot of stability data for this patch until it reaches Beta.

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 9214248 [details]
Bug 1697952 - Avoid null-deref in LinkEnclosingLazyScript. r?tcampbell!

Beta/Release Uplift Approval Request

• User impact if declined: This is an attempt to mitigate a null-deref crash.
  This doesn't have testcase, or manual test steps.
  We'll monitor the crash stats for the effect.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This skips a possibly unexpected null-deref.
  If this is really unexpected dereference, the crash won't happen.
  If this is expected dereference, the null-deref crash will re-appear with slightly different signature.
• String changes made/needed: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9214248 [details]
Bug 1697952 - Avoid null-deref in LinkEnclosingLazyScript. r?tcampbell!

Let's give it a shot! Approved for 88.0b9.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f29f1ef81249

Comment 11

[Tooru Fujisawa [:arai]]

Looks like the crash is still happening here:
https://searchfox.org/mozilla-central/rev/3ae6f9e4444f3fbf61034317540d36f621117600/js/src/frontend/Stencil.cpp#1180

We should apply the same check.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1697952 - Part 2: Avoid another null-deref in LinkEnclosingLazyScript. r?tcampbell!

Comment 13

[Tooru Fujisawa [:arai]]

will request approval once the another patch is landed.

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 9214816 [details]
Bug 1697952 - Part 2: Avoid another null-deref in LinkEnclosingLazyScript. r?tcampbell!

Beta/Release Uplift Approval Request

• User impact if declined: This is another attempt to mitigate a null-deref crash.
  The previous patch confirmed that it's really a function pointer.
  This will fix the later possply unexpected null-deref for the same pointer.
  This doesn't have testcase, or manual test steps.
  We'll monitor the crash stats for the effect.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This skips yet another possibly unexpected null-deref.
  If this is really unexpected dereference, the crash won't happen.
  If this is expected dereference, the null-deref crash will re-appear with slightly different signature.
• String changes made/needed: None

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5263c3efb98d
Part 2: Avoid another null-deref in LinkEnclosingLazyScript. r=tcampbell

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5263c3efb98d

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9214816 [details]
Bug 1697952 - Part 2: Avoid another null-deref in LinkEnclosingLazyScript. r?tcampbell!

Approved for 88.0rc1.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/abbbc5f54798

Comment 19

[Tooru Fujisawa [:arai]]

no crashes with js::frontend::CompilationStencil::instantiateStencils signature after 88.0b9.
There are a few crashes (6 crashes) with js::frontend::CompilationStencil::instantiateStencilAfterPreparation signature for both 88.0rc1 and recent 89.0a1, but they look very different issue. different stack and different address.
will keep eyes on them