1884686 - Crash in [@ nsDocShell::GetBrowsingContext | nsDocumentViewer::PermitUnload]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/9647b345-2f19-4f38-968b-cba910240311

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0  libxul.so  RefPtr<mozilla::dom::BrowsingContext>::get const  mfbt/RefPtr.h:314
0  libxul.so  RefPtr<mozilla::dom::BrowsingContext>::operator mozilla::dom::BrowsingContext* const&  mfbt/RefPtr.h:327
0  libxul.so  nsDocShell::GetBrowsingContext  docshell/base/nsDocShell.cpp:13533
0  libxul.so  nsDocumentViewer::PermitUnload  layout/base/nsDocumentViewer.cpp:1146
1  libxul.so  nsDocShell::InternalLoad  docshell/base/nsDocShell.cpp
2  libxul.so  nsDocShell::LoadHistoryEntry  docshell/base/nsDocShell.cpp:12018
3  libxul.so  nsDocShell::LoadHistoryEntry  docshell/base/nsDocShell.cpp:11939
4  libxul.so  nsDocShell::LoadURI  docshell/base/nsDocShell.cpp:783
5  libxul.so  mozilla::dom::BrowsingContext::LoadURI  docshell/base/BrowsingContext.cpp:1967
6  libxul.so  nsSHistory::LoadURIOrBFCache  docshell/shistory/nsSHistory.cpp:1403

While the bogus looking stacks with OffTheBooksMutex::Unlock in them from bug 1842606 are still present, while looking at this signature, it seems like this nsDocumentViewer::PermitUnload variation is much more common now.

It looks like the crash is on this line of nsDocumentViewer::PermitUnload:

RefPtr<BrowsingContext> bc = mContainer->GetBrowsingContext();

This looks like a missing null check on mContainer. I see two other places that do this call without an obvious null check, so maybe those need to be fixed, too.

Comment 1

[Andrew McCreight [:mccr8]]

Out of the 2687 crashes with this signature in the last 3 months, 2093 of them have PermitUnload in the proto signature (about 78%).

Comment 2

[Andrew McCreight [:mccr8]]

One bug 1884689 is deployed, the signature will change.

Comment 3

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

The severity field is not set for this bug.
:boris, could you have a look please?

For more information, please visit BugBot documentation.

Comment 4

[Boris Chiou [:boris]]

I'd like to move this into DOM related component, per the call stack in comment 0 (e.g. browsing context and nsSHistory).

Comment 5

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

Oh, this is Fenix. Non-SHIP, non-Fission.

Comment 6

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

Yeah, I think we just miss some null checks.

Comment 7

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

Attachment: Bug 1884686 - null check DocumentViewer::mContainer before using it, r=mccr8

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ef6aa240e2a
null check DocumentViewer::mContainer before using it, r=mccr8

Comment 9

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/2ef6aa240e2a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

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

Attachment: Bug 1884686 - null check DocumentViewer::mContainer before using it, r=mccr8

Original Revision: https://phabricator.services.mozilla.com/D205767

Comment 12

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: crashes
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: -
• Risk associated with taking this patch: Low
• Explanation of risk level: Simple null check based on crash reports
• String changes made/needed: NA
• Is Android affected?: yes

Comment 13

[Pulsebot]

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