1799326 - Crash in [@ RefPtr<T>::assign_assuming_AddRef | RefPtr<T>::operator= | mozilla::dom::BrowsingContextWebProgress::ContextReplaced]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/bd9e6204-7d68-4d4c-bd7e-f435b0221104

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  RefPtr<mozilla::dom::CanonicalBrowsingContext>::assign_assuming_AddRef  mfbt/RefPtr.h:66
1  xul.dll  RefPtr<mozilla::dom::CanonicalBrowsingContext>::operator=  mfbt/RefPtr.h:190
1  xul.dll  mozilla::dom::BrowsingContextWebProgress::ContextReplaced  docshell/base/BrowsingContextWebProgress.cpp:164
1  xul.dll  mozilla::dom::CanonicalBrowsingContext::ReplacedBy  docshell/base/CanonicalBrowsingContext.cpp:305
2  xul.dll  FinishRestore  docshell/shistory/nsSHistory.cpp:1285
3  xul.dll  std::_Func_impl_no_alloc<`lambda at /builds/worker/checkouts/gecko/docshell/shistory/nsSHistory.cpp:1356:31', void, bool>::_Do_call  docshell/shistory/nsSHistory.cpp:1359
4  xul.dll  mozilla::dom::  dom/ipc/WindowGlobalParent.cpp:804
5  xul.dll  IPC::MessageReader::ReadSentinel  ipc/chromium/src/chrome/common/ipc_message_utils.h:175
5  xul.dll  mozilla::dom::PContentParent::OnMessageReceived  ipc/ipdl/PContentParent.cpp:7469
6  xul.dll  mozilla::ipc::MessageChannel::DispatchAsyncMessage  ipc/glue/MessageChannel.cpp:1756

We're accessing a NULL pointer though it's not clear where it's coming from. This seems to have started during the development of version 106, build ID 20221103095349 is the first affected.

Comment 1

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Peter, this may be interesting for you to take a look at.

Comment 2

[Gabriele Svelto [:gsvelto]]

Signature change due to the inlined functions being added to the stack

Comment 3

[Gabriele Svelto [:gsvelto]]

Some useful comments from the crashes:

hitting the back button crashed the browser

I alt-clicked the history forward vbutton in the top bar of the browser thinking it would open the page I was on in a new window. Instead the browser crashed.

pressed alt+scroll accidentially and kept going back in history at a high speed and the browser closed

Comment 4

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

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 20 desktop browser crashes on beta

For more information, please visit BugBot documentation.

Comment 5

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Hi Andreas, this looks at your wheelhouse. Can you take a look please?

Comment 6

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

Based on the topcrash criteria, the crash signatures linked to this bug are not in the topcrash signatures anymore.

For more information, please visit BugBot documentation.

Comment 7

[Andreas Farre [:farre]]

This happens in nsSHistory::LoadURIOrBFCache and has got to do with allowing BFCache for pages with beforeunload handlers. This was turned on 2023-06-08, which is roughly two months (the time it takes for it hits the release channel) before it started spiking.

The stack seems to be mostly (all?) from when the promise gets resolved after ContentParent::SendDispatchBeforeUnloadToSubtree. This bounces back from ContentChild, which makes me wonder: can we race this somehow so that we call CanonicalBrowsingContext::ReplacedBy twice? In CanonicalBrowsingContext::ReplacedBy we'd definitely be able to explain the crash in that case.

My gut feeling is that it shouldn't be allowed to restore an already replaced browsing context. Either that or we need epochs to keep track of CanonicalBrowsingContext::ReplacedBy so that we can't skip ahead.

:sefeng, you added the code to allow BFCache for beforeunload-handler pages, does this seem reasonable?

:peterv, :smaug, you being the shistory experts, what would you expect to happen?

I'll do:

index 885121760a767..24f3c0895ac17 100644
--- a/docshell/shistory/nsSHistory.cpp
+++ b/docshell/shistory/nsSHistory.cpp
@@ -1370,7 +1370,7 @@ void nsSHistory::LoadURIOrBFCache(LoadEntryResult& aLoadEntry) {
                       ->GetCurrentWindowGlobal()) {
             wgp->PermitUnload([canonicalBC, loadState, she, frameLoader,
                                currentFrameLoader, canSave](bool aAllow) {
-              if (aAllow) {
+              if (aAllow && !canonicalBC->IsReplaced()) {
                 FinishRestore(canonicalBC, loadState, she, frameLoader,
                               canSave && canonicalBC->AllowedInBFCache(
                                              Nothing(), nullptr));

and see what try says.

Comment 8

[Sean Feng [:sefeng211]]

I don't have opinions.....given my changes were more like a small refactor.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

That patch looks rather reasonable to me, but you need to run all the mochitests. Many of the bfcache tests are browser-chrome tests, or chrome or plain.

Comment 10

[Andreas Farre [:farre]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #9)

That patch looks rather reasonable to me, but you need to run all the mochitests. Many of the bfcache tests are browser-chrome tests, or chrome or plain.

All mochitests coming up!

Comment 11

[Andreas Farre [:farre]]

Attachment: Bug 1799326 - Don't restore replaced browsing contexts from BFCache. r=smaug!

Comment 12

[Andreas Farre [:farre]]

Yeah, so I think try is green enough. I pushed it to your review queue. Since this is a blind fix I don't think we should close this bug when the patch land, or at least not until we've verified that the crashes go down.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c2973c8a29b
Don't restore replaced browsing contexts from BFCache. r=smaug

Comment 14

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/9c2973c8a29b

Comment 15

[Andreas Farre [:farre]]

I'm looking at crash-stats for beta 19 (which I think this patch made it into) and there seems to be no more crashes.

Hsin-Yi, is this a release uplift candidate?

Comment 16

[Andreas Farre [:farre]]

Clearing Peter's ni, Olli answered all the questions I had.

Comment 17

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to Andreas Farre [:farre] from comment #15)

I'm looking at crash-stats for beta 19 (which I think this patch made it into) and there seems to be no more crashes.

Hsin-Yi, is this a release uplift candidate?

Sounds a reasonable uplift candidate for a ride-along.

Comment 18

[Andreas Farre [:farre]]

Comment on attachment 9354060 [details]
Bug 1799326 - Don't restore replaced browsing contexts from BFCache. r=smaug!

Beta/Release Uplift Approval Request

• User impact if declined: A fix for a fairly high crash bug gets delayed.
• 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): Small and contained fix.
• String changes made/needed:
• Is Android affected?: Yes

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9354060 [details]
Bug 1799326 - Don't restore replaced browsing contexts from BFCache. r=smaug!

Approved for 115.4esr

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/5e0affad68dc

Comment 21

[Pascal Chevrel:pascalc]

Comment on attachment 9354060 [details]
Bug 1799326 - Don't restore replaced browsing contexts from BFCache. r=smaug!

Approved for our next 118 dot release, thanks.

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/e48402d810da

Comment 23

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-release/rev/e48402d810da