1693946 - Crash at @ mozilla::detail::MutexImpl::lock | nsBrowsingContextReadyCallback::BrowsingContextReady

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://crash-stats.mozilla.org/report/index/ccb26a4d-fb18-46ac-b5cf-e72e60210214#tab-details

Signature mozilla::detail::MutexImpl::lock | nsBrowsingContextReadyCallback::BrowsingContextReady More Reports Search
UUID f74bcae9-1ae1-4951-aa81-76c9a0210213
Date Processed 2021-02-13 21:30:29 UTC
Uptime 2,019 seconds (33 minutes and 39 seconds)
Last Crash 2,059 seconds before submission (34 minutes and 19 seconds)
Install Age 1,428,628 seconds since version was first installed (2 weeks, 2 days and 12 hours)
Install Time 2021-01-28 08:39:51
Product Firefox
Release Channel esr
Version 78.7.0esr
Build ID 20210119174753 (2021-01-19) Buildhub data
OS Debian GNU/Linux 10 (buster)
OS Version 0.0.0 Linux 4.19.0-14-amd64 #1 SMP Debian 4.19.171-2 (2021-01-30) x86_64
Build Architecture amd64
CPU Info family 6 model 45 stepping 7
CPU Count 8
Adapter Vendor ID

NVIDIA Corporation (0x10de)

Adapter Device ID

GK104 [GeForce GTX 670] (0x1189)

Startup Crash

False

Crash Reason SIGSEGV /SEGV_MAPERR
Crash Address 0x28
EMCheckCompatibility

False

App Notes

Debian GNU/Linux 10 (buster)FP(D00-L1000-W00000000-T000) WR? WR- OMTP? OMTP+4

Processor Notes

processor_ip-172-31-46-251_us-west-2_compute_internal_7; ProcessorPipeline

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Frame Module Signature Source Trust
0 libpthread.so.0 __pthread_mutex_lock /build/glibc-vjB4T1/glibc-2.28/nptl/../nptl/pthread_mutex_lock.c:67 context
1 firefox-esr mozilla::detail::MutexImpl::lock() build-browser/mozglue/misc/mozglue/misc/Mutex_posix.cpp:118 cfi
2 libxul.so nsBrowsingContextReadyCallback::BrowsingContextReady(mozilla::dom::BrowsingContext*) cfi
3 libxul.so nsFrameLoader::InvokeBrowsingContextReadyCallback() build-browser/dom/base/dom/base/nsFrameLoader.cpp:3578 cfi
4 libxul.so nsFrameLoader::TryRemoteBrowserInternal() cfi
5 libxul.so nsFrameLoader::TryRemoteBrowser() build-browser/dom/base/dom/base/nsFrameLoader.cpp:2657 cfi
6 libxul.so nsFrameLoader::GetBrowsingContext() build-browser/dom/base/dom/base/nsFrameLoader.cpp:3195 cfi
7 libxul.so mozilla::dom::FrameLoader_Binding::get_browsingContext(JSContext*, JS::Handle<JSObject*>, void*, JSJitGetterCallArgs) build-browser/dom/bindings/build-browser/dom/bindings/FrameLoaderBinding.cpp:163 cfi
8 libxul.so bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) build-browser/dom/bindings/dom/bindings/BindingUtils.cpp:3079 cfi
9 libxul.so js::jit::CallNativeGetter(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) build-browser/js/src/jit/js/src/jit/VMFunctions.cpp:1478 cfi

Comment 2

[Anne (:annevk)]

Andreas, is this something you could look at?

Comment 3

[Chris Peterson [:cpeterson]]

Nika says we are crashing on null promise here:

https://searchfox.org/mozilla-central/rev/c8ce16e4299a3afd560320d8d094556f2b5504cd/toolkit/components/windowwatcher/nsOpenWindowInfo.cpp#77

We should null check the promise, but include a MOZ_DIAGNOSTIC_ASSERT so we can catch bugs like this in Nightly. No need to uplift to ESR 78 because crash volume is so low (only about 5 crash reports in the last six months).

But why are we getting second promise call?? The same open window is being passed to a BrowsingContext twice? Process switch appears to reuse same nsIOpenWindowInfo and then create a remote browser in that same process, but we should probably create a non-remote browser? Non-remoteness attribute was overwritten?

The ESR 78 code:

https://searchfox.org/mozilla-esr78/rev/b2ea949e954c346e69c617a333b5253ff3cb8db2/dom/xul/XULFrameElement.cpp#78-86

is missing some checks from: https://phabricator.services.mozilla.com/D85446

Assigning to Anny because she touched this code in bug 1630323.

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I could reproduce this crash randomly, let me know if you need my help, although I haven't found deterministic STR.

Comment 5

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1693946 - Null check the browsing context ready callback in nsOpenWindowInfo, r=nika!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5640ed7747b5
Null check the browsing context ready callback in nsOpenWindowInfo, r=nika

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/5640ed7747b5

Comment 8

[Anny Gakhokidze [:annyG]]

Reopening because of the need to uplift to esr78

Comment 9

[Anny Gakhokidze [:annyG]]

<cut>

Comment 10

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1693946 - Null check the browsing context ready callback in nsOpenWindowInfo, r=nika!

Comment 11

[Anny Gakhokidze [:annyG]]

Comment on attachment 9205222 [details]
Bug 1693946 - Null check the browsing context ready callback in nsOpenWindowInfo, r=nika!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: want to prevent browser crashes
• User impact if declined: browser crash
• Fix Landed on Version: 88
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): low because it's just a null check
• String or UUID changes made by this patch:

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Bug resolution tracks landing on m-c. Please don't reopen them for uplifts.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I don't see any signs of this crash signature on esr78. Was there a reason we felt it may be needed there?

Comment 14

[Anny Gakhokidze [:annyG]]

Comment 1 has the link to a crash on ESR 78 https://crash-stats.mozilla.org/report/index/ccb26a4d-fb18-46ac-b5cf-e72e60210214#tab-details

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9205222 [details]
Bug 1693946 - Null check the browsing context ready callback in nsOpenWindowInfo, r=nika!

Not a particularly high volume crash on ESR, but also a trivial patch to take. Approved for 78.9esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/e657d09ce414