1547760 - Crash in [@ mozilla::layers::UiCompositorControllerChild::OpenForSameProcess]

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

This bug is for crash report bp-0939ce82-5ef7-4f59-878f-0ba400190427.

Top 10 frames of crashing thread:

0 libxul.so mozilla::layers::UiCompositorControllerChild::OpenForSameProcess gfx/layers/ipc/UiCompositorControllerChild.cpp:236
1 libxul.so mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::AudioTrackEncoder> const, void  xpcom/threads/nsThreadUtils.h:1128
2 libxul.so long long mozilla::jni::NativeStub<mozilla::java::GeckoThread::RunUiThreadCallback_t, GeckoThreadSupport, mozilla::jni::Args<> >::Wrap<&GeckoThreadSupport::RunUiThreadCallback> widget/android/AndroidUiThread.cpp:331
3 base.odex base.odex@0x20a2f 
4 dalvik-LinearAlloc (deleted) dalvik-LinearAlloc @0x3f3a 
5 dalvik-main space (region space) (deleted) dalvik-main space @0x70023e 
6 dalvik-main space (region space) (deleted) dalvik-main space @0x94943e 
7 dalvik-main space (region space) (deleted) dalvik-main space @0x78638e 
8 dalvik-main space (region space) (deleted) dalvik-main space @0x72c89e 
9 dalvik-main space (region space) (deleted) dalvik-main space @0x7b3b4e 

Comment 1

[Chris Peterson [:cpeterson]]

Could this crash be similar to UiCompositorControllerParent::Initialize crash bug 1515689, which was supposedly fixed in GV 64 but is now a top crash for Focus 8.0.9 with GV 68 Nightly?

TBD whether this crash is seen in Fenix with GV 68.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Crash address of the crash in comment 0 is 0xe5e5e859 so looks like a UAF

Comment 3

[Jessie [:jbonisteel] pls NI]

Sotaro, could you take a peak at this one? Thank you

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #0)

This bug is for crash report bp-0939ce82-5ef7-4f59-878f-0ba400190427.

The crash report is weird, the signature says UiCompositorControllerChild::OpenForSameProcess(), but the source code link suggested UiCompositorControllerChild::RecvToolbarAnimatorMessageFromCompositor().

One possibility is that the crash happened at RecvToolbarAnimatorMessageFromCompositor() in UiCompositorControllerChild::OpenForSameProcess().

Comment 5

[Sotaro Ikeda [:sotaro]]

From the crash report, UiCompositorControllerChild::mWidget seemed invalid. But if UiCompositorControllerChild::Destroy() is called correctly, it should not happen. The UiCompositorControllerChild::Destroy() is called from nsWindow::Destroy().

• https://searchfox.org/mozilla-central/source/widget/android/nsWindow.cpp#1496
• https://searchfox.org/mozilla-central/source/gfx/layers/ipc/UiCompositorControllerChild.cpp#176

From it, when the crash happened, nsWindow might be destroyed without nsWindow::Destroy().

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

This is our topcrash in Focus.

Comment 7

[Sotaro Ikeda [:sotaro]]

On possible fix is ensuring UiCompositorControllerChild::mWidget alive when it is used. Since crash seemed to be caused by uaf of mWidget.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: Bug 1547760 - Ensuring UiCompositorControllerChild::mWidget alive when it is used

Comment 9

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ec07702c5fcb9b2de8570df088fb794f22c8319

Comment 10

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/adc12b4c5a9d8f5139d182441f7fe2b672f08677

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/adc12b4c5a9d

This issue also affects beta according to the status flags and is classified as sec-high. It would have required sec-approval before landing, see https://wiki.mozilla.org/Security/Bug_Approval_Process
Please request the sec-approval and uplift to beta (patch from central applies cleanly).

Comment 12

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9072491 [details]
Bug 1547760 - Ensuring UiCompositorControllerChild::mWidget alive when it is used

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is not easy. We do not know a STR to cause the crash. The crash happened only with Focus.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 64
• If not all supported branches, which bug introduced the flaw?: It is not clear
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It is easy to create a backports.
• How likely is this patch to cause regressions; how much testing does it need?: It seems not cause a regression.

Beta/Release Uplift Approval Request

• User impact if declined: Crash might happen during using Focus.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1559859
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is relatively simple. It changed nsBaseWidget* to RefPtr<nsBaseWidget>.
• String changes made/needed: None

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9072491 [details]
Bug 1547760 - Ensuring UiCompositorControllerChild::mWidget alive when it is used

sec-approval+ for mozilla-central and beta approval given.

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/01f0e8d1402c

Comment 15

[Chris Peterson [:cpeterson]]

Sotaro, can this crash affect GeckoView in Fenix? This crash signature is top crash for GeckoView 68 in Focus, but I don't see any instances from GeckoView 68 in Fenix.

Comment 16

[Sotaro Ikeda [:sotaro]]

If the crash did not happen in Fenix, it seems not affect to GeckoView 68 in Fenix. To cause the crash, widget needs to be destroyed just after creating compositor.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

It might also be specific to the focus usage pattern, maybe the button to trash your session triggers this somehow.