1615306 - Crash in [@ nsFrameLoader::DestroyDocShell]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Pascal Chevrel:pascalc]

This bug is for crash report bp-ba14a585-cc45-4f54-a7c7-4a7940200212.

Top 10 frames of crashing thread:

0 xul.dll nsFrameLoader::DestroyDocShell dom/base/nsFrameLoader.cpp:1934
1 xul.dll nsFrameLoaderDestroyRunnable::Run dom/base/nsFrameLoader.cpp:1867
2 xul.dll mozilla::dom::Document::MaybeInitializeFinalizeFrameLoaders dom/base/Document.cpp:8521
3 xul.dll mozilla::detail::RunnableMethodImpl< xpcom/threads/nsThreadUtils.h:1215
4 xul.dll static nsContentUtils::RemoveScriptBlocker dom/base/nsContentUtils.cpp:5376
5 xul.dll nsDocumentViewer::InitInternal layout/base/nsDocumentViewer.cpp:973
6 xul.dll nsDocumentViewer::Init layout/base/nsDocumentViewer.cpp:758
7 xul.dll nsDocShell::SetupNewViewer docshell/base/nsDocShell.cpp:8004
8 xul.dll nsDocShell::Embed docshell/base/nsDocShell.cpp:5777
9 xul.dll nsDocShell::CreateAboutBlankContentViewer docshell/base/nsDocShell.cpp:6617

New signature that started appearing on 74.0b1/b2

Comment 1

[Pascal Chevrel:pascalc]

A couple of crashes in the last days on Nightly as well.

Comment 2

[John Dai[:jdai]]

Hi :kmag, I saw frame 0 was changed by bug 1582832. It seems that it's unable to read mBrowsingContext->EverAttached(). Could you help to take a look? Thank you.

Comment 3

[[:philipp]]

the signature stopped in 74.0b after the early beta phase - in 75.0b it's continuing and looking like it will go to release as well.
submitted urls in crash reports are dominated by mail.google.com, which is also referenced in the two available comments:

• Clicked to open a gmail message and BLAMMO!
• Hit refresh on GMail after computer sat in sleep mode over night

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to [:philipp] from comment #3)

the signature stopped in 74.0b after the early beta phase - in 75.0b it's continuing and looking like it will go to release as well.

@ Philipp: I see crash reports from 74.0b and 75.0b with about the same volume, but no crash reports from 74.0 release. I wonder why not. Any reason to think the crashes in 75.0b will continue into 75.0 release when they didn't in 74.0?

Peeking at crash report bp92c4a0e1-f42f-4e68-ba60-205620200330, looks like maybe a nsFrameLoaderDestroyRunnable::mFrameLoader is crashing on a null mPendingBrowsingContext?

https://hg.mozilla.org/mozilla-central/annotate/01b9bdb6e5ef2a19e1c3311a47569d62f275db1f/dom/base/nsFrameLoader.cpp#l1931

Comment 5

[[:philipp]]

during the 74 beta cycle, crashes were last recorded in 74.0b5 and have stopped in b6 and later on:
https://crash-stats.mozilla.org/search/?signature=%3DnsFrameLoader%3A%3ADestroyDocShell&release_channel=beta&date=>%3D2020-01-01&_facets=version#facet-version (sort by version)

first i thought this may have been due to reverting the EARLY_BETA_OR_EARLIER flag, but that happened later in b7.

this is the pushlog of changes happening between 74.0b5 and 74.0b6, so perhaps the backout at https://bugzilla.mozilla.org/show_bug.cgi?id=1582832#c29 took care of the issue for 74:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_74_0b5_RELEASE&tochange=23159c7ee39f9cb1ee182a72d0b3d1afdc6ceba4&full=1

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1615306: Add missing null check. r=nika

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2859427c530a
Add missing null check. r=nika

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2859427c530a

Comment 9

[[:philipp]]

hi, thanks for the patch. as it looks fairly simple/safe and a regressing issue, could you request uplift to beta? (although it's fairly late in the cycle already)

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 9136970 [details]
Bug 1615306: Add missing null check. r=nika

Beta/Release Uplift Approval Request

• User impact if declined: crashes
• Is this code covered by automated tests?: Yes
• 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): It just adds a null check. Maybe it won't fix anything, but it shouldn't cause any additional issues.
• String changes made/needed:

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9136970 [details]
Bug 1615306: Add missing null check. r=nika

nullptr check, approved for 75 rc1

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/5ceb92060689

Comment 13

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

There is nothing QE can do here as per comment 10.
Removing QE+ flag.