1725107 - SessionStoreUtils_Binding::restoreDocShellState Crash in [@ mozilla::dom::ToJSValue]

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/77ebc3f1-8163-4b5e-b236-5bc7d0210811

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0 XUL mozilla::dom::ToJSValue dom/bindings/ToJSValue.cpp:64
1 XUL mozilla::dom::SessionStoreUtils_Binding::restoreDocShellState dom/bindings/SessionStoreUtilsBinding.cpp:2470
2 XUL mozilla::dom::StaticMethodPromiseWrapper dom/bindings/BindingUtils.cpp:3338
3 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:489
4 XUL Interpret js/src/vm/Interpreter.cpp:3242
5 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:521
6 XUL js::jit::DoCallFallback js/src/jit/BaselineIC.cpp:1606
7  @0x1aafaeef7d87 
8  @0x1326dfb3f 
9  @0x1aafaeef5314 

Comment 1

[Chris Peterson [:cpeterson]]

Kashav, do you know of any recent Session Store changes that might have made this crash in mozilla::dom::SessionStoreUtils_Binding::restoreDocShellState more common starting around 2021-08-06?

Comment 2

[u608768]

We're returning a nullptr promise without throwing an error. Probably because we sometimes don't have a window global parent here.

This is a session restore bug, but I'm not too sure why this is only coming up now. None of the recent session store changes could have caused this, but maybe something changed to make us start restoring before PWindowGlobal is set up?

(There's also the issue that there's a few ToJSValue crashes with a different stack. Filed https://github.com/mozilla-services/socorro/pull/5850 for that.)

Comment 3

[u608768]

Attachment: Bug 1725107 - Ensure we're throwing whenever we return a null promise, r?farre

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f1b1e94f22c
Ensure we're throwing whenever we return a null promise, r=farre

Comment 5

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4f1b1e94f22c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 7

[u608768]

Comment on attachment 9236533 [details]
Bug 1725107 - Ensure we're throwing whenever we return a null promise, r?farre

Beta/Release Uplift Approval Request

• User impact if declined: Browser may crash while restoring.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): Low risk - we'll throw an error instead of crashing now.
• String changes made/needed:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9236533 [details]
Bug 1725107 - Ensure we're throwing whenever we return a null promise, r?farre

Fix looks good in Nightly, approved for 92.0b8.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/551bc51ad7c1