1822175 - Crash in [@ mozilla::widget::AndroidVsync::OnMaybeUpdateRefreshRate] on jemalloc poison value

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/e6d4ef59-9688-4b02-ad83-af8750230313

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0  libxul.so  mozilla::widget::AndroidVsync::OnMaybeUpdateRefreshRate  widget/android/AndroidVsync.cpp:142
0  libxul.so  mozilla::widget::ScreenHelperAndroid::Refresh  widget/android/ScreenHelperAndroid.cpp:68
1  libxul.so  mozilla::jni::detail::ProxyNativeCall<mozilla::widget::ScreenHelperAndroid::ScreenHelperSupport, mozilla::java::ScreenManagerHelper, true, false, >::Call<true, false, > const  widget/android/jni/Natives.h:1200
1  libxul.so  mozilla::jni::detail::ProxyNativeCall<mozilla::widget::ScreenHelperAndroid::ScreenHelperSupport, mozilla::java::ScreenManagerHelper, true, false, >::operator  widget/android/jni/Natives.h:1281
1  libxul.so  mozilla::detail::RunnableFunction<mozilla::jni::detail::ProxyNativeCall<mozilla::widget::ScreenHelperAndroid::ScreenHelperSupport, mozilla::java::ScreenManagerHelper, true, false, > >::Run  xpcom/threads/nsThreadUtils.h:547
2  libxul.so  mozilla::RunnableTask::Run  xpcom/threads/TaskController.cpp:553
2  libxul.so  mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal  xpcom/threads/TaskController.cpp:867
2  libxul.so  mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal  xpcom/threads/TaskController.cpp:698
2  libxul.so  mozilla::TaskController::ProcessPendingMTTask  xpcom/threads/TaskController.cpp:464
3  libxul.so  mozilla::TaskController::InitializeInternal const  xpcom/threads/TaskController.cpp:191

I only see 4 of these crashes, which isn't a lot, but they are all on the jemalloc poison value, indicating a use after free. The first build I see is 20230307211537 (in 112 Nightly) so this might be a regression.

Comment 1

[Andrew McCreight [:mccr8]]

m_kato, could this be related to bug 1813573? That looks related and landed recently. Thanks.

Comment 2

[Makoto Kato [:m_kato]]

Maybe yes.

Comment 3

[Andrew McCreight [:mccr8]]

I'll speculatively mark it as a regression, then.

Comment 4

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: use-after-free that is possibly a regression

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Bug 1822175 - Clean up dynamic refresh rate support on GeckoView.

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 9323458 [details]
Bug 1822175 - Clean up dynamic refresh rate support on GeckoView.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Difficult. From this patch, this may be crash issue by race condition. Attacker need to know the address that is destroyed object.
• 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?: 112
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Low risk. I remove unused path that may cause UAF. Also, although I add more lock, it is more careful.
• Is Android affected?: Yes

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9323458 [details]
Bug 1822175 - Clean up dynamic refresh rate support on GeckoView.

Approved to land and request uplift

Comment 8

[Sandor Molnar[:smolnar]]

Backed out for causing xpc failures in widget/headless/tests/test_headless.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/eacf2631297e06b4d521e2cab122beb02eddc740

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=a1261c0eb985f566a9b6ae7ba33830de6c8482df&searchStr=xpc&selectedTaskRun=RxRI2Y8VQGuJg_f2_Qi6qA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=409811001&repo=autoland&lineNumber=3900

Comment 9

[Sandor Molnar[:smolnar]]

Was backed out by mistake.
Relanded: https://hg.mozilla.org/integration/autoland/rev/3521f955b11fdab2ea654f2dc4a4a91c76a420a5

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3521f955b11f

Comment 11

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

The patch landed in nightly and beta is affected.
:m_kato, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 9323458 [details]
Bug 1822175 - Clean up dynamic refresh rate support on GeckoView.

Beta/Release Uplift Approval Request

• User impact if declined: Crash firefox when device updates screen information
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): I remove unused path that may cause UAF. Also, although I add more lock, it is more careful.
• String changes made/needed:
• Is Android affected?: Yes

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9323458 [details]
Bug 1822175 - Clean up dynamic refresh rate support on GeckoView.

Approved for 112.0b7

Comment 14

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/20e78c19001c