Crash in [@ mozilla::layers::UiCompositorControllerChild::OpenForSameProcess]
Categories
(Core :: Graphics: Layers, defect, P3)
Tracking
()
People
(Reporter: davidb, Assigned: sotaro)
References
Details
(4 keywords, Whiteboard: [geckoview:focus][post-critsmash-triage][adv-main68+])
Crash Data
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Review |
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
Updated•6 years ago
|
Comment 1•6 years ago
|
||
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•6 years ago
|
||
Crash address of the crash in comment 0 is 0xe5e5e859 so looks like a UAF
Updated•6 years ago
|
Comment 3•6 years ago
|
||
Sotaro, could you take a peak at this one? Thank you
Assignee | ||
Comment 4•6 years ago
•
|
||
(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().
Assignee | ||
Comment 5•6 years ago
|
||
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().
Reporter | ||
Comment 6•6 years ago
|
||
This is our topcrash in Focus.
Assignee | ||
Comment 7•6 years ago
•
|
||
On possible fix is ensuring UiCompositorControllerChild::mWidget alive when it is used. Since crash seemed to be caused by uaf of mWidget.
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
Assignee | ||
Comment 10•6 years ago
|
||
![]() |
||
Comment 11•6 years ago
|
||
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).
![]() |
||
Updated•6 years ago
|
Assignee | ||
Comment 12•6 years ago
|
||
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•6 years ago
|
||
Comment on attachment 9072491 [details]
Bug 1547760 - Ensuring UiCompositorControllerChild::mWidget alive when it is used
sec-approval+ for mozilla-central and beta approval given.
Updated•6 years ago
|
Comment 14•6 years ago
|
||
uplift |
Comment 15•6 years ago
|
||
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.
Assignee | ||
Comment 16•6 years ago
|
||
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•6 years ago
|
||
It might also be specific to the focus usage pattern, maybe the button to trash your session triggers this somehow.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Description
•