1996570 - Intermittent SUMMARY: ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:338:45 in operator bool | single tracking bug

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

Filed by: amarc [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=533109479&repo=autoland&task=ZOJ5CrxrQt6RNIBDPs4XhA.0
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/ZOJ5CrxrQt6RNIBDPs4XhA/runs/0/artifacts/public/logs/live_backing.log

---

TEST-START | devtools/client/responsive/test/browser/browser_touch_event_iframes.js
[...]
TEST-PASS | devtools/client/responsive/test/browser/browser_touch_event_iframes.js | ID 0 - untranslated iframe with DPR 1 and path http://example.com/browser/devtools/client/responsive/test/browser/ got click at close enough Y 50, screen is 300. - true == true - 
[...]
WARNING: ThreadSanitizer: data race (pid=4308)
  Read of size 8 at 0x723c00020cf0 by thread T91 (mutexes: write M0, write M1):
    #0 operator bool /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:338:45 (libxul.so+0x4f08084) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #1 mozilla::layers::GestureEventListener::Destroy() /builds/worker/checkouts/gecko/gfx/layers/apz/src/GestureEventListener.cpp:92:7 (libxul.so+0x4f08084)
    #2 mozilla::layers::AsyncPanZoomController::Destroy() /builds/worker/checkouts/gecko/gfx/layers/apz/src/AsyncPanZoomController.cpp:932:30 (libxul.so+0x4ec956b) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #3 mozilla::layers::HitTestingTreeNode::Destroy() /builds/worker/checkouts/gecko/gfx/layers/apz/src/HitTestingTreeNode.cpp:67:14 (libxul.so+0x4f0b963) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #4 mozilla::layers::APZCTreeManager::UpdateHitTestingTree(mozilla::layers::WebRenderScrollDataWrapper const&, mozilla::layers::LayersId, unsigned int) /builds/worker/checkouts/gecko/gfx/layers/apz/src/APZCTreeManager.cpp:753:31 (libxul.so+0x4e98f3a) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #5 operator() /builds/worker/checkouts/gecko/gfx/layers/apz/src/APZUpdater.cpp:218:43 (libxul.so+0x4efe472) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #6 mozilla::detail::RunnableFunction<mozilla::layers::APZUpdater::UpdateScrollDataAndTreeState(mozilla::layers::LayersId, mozilla::layers::LayersId, mozilla::wr::Epoch const&, mozilla::layers::WebRenderScrollData&&)::$_1>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:550:5 (libxul.so+0x4efe472)
    #7 mozilla::layers::APZUpdater::ProcessQueue() /builds/worker/checkouts/gecko/gfx/layers/apz/src/APZUpdater.cpp:523:23 (libxul.so+0x4ec50e8) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
[...]
  Previous write of size 8 at 0x723c00020cf0 by main thread:
    #0 assign_assuming_AddRef /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:66:13 (libxul.so+0x4f0b21e) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #1 operator= /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:180:5 (libxul.so+0x4f0b21e)
    #2 mozilla::layers::GestureEventListener::HandleInputTimeoutMaxTap(bool) /builds/worker/checkouts/gecko/gfx/layers/apz/src/GestureEventListener.cpp:577:22 (libxul.so+0x4f0b21e)
    #3 operator()<StoreCopyPassByConstLRef<bool> &> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1084:18 (libxul.so+0x4f2090c) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #4 __invoke_impl<void, (lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1083:9), StoreCopyPassByConstLRef<bool> &> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:60:14 (libxul.so+0x4f2090c)
    #5 __invoke<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1083:9), StoreCopyPassByConstLRef<bool> &> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/invoke.h:95:14 (libxul.so+0x4f2090c)
    #6 __apply_impl<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1083:9), std::tuple<StoreCopyPassByConstLRef<bool> > &, 0UL> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/tuple:1740:14 (libxul.so+0x4f2090c)
    #7 apply<(lambda at /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1083:9), std::tuple<StoreCopyPassByConstLRef<bool> > &> /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/tuple:1751:14 (libxul.so+0x4f2090c)
    #8 apply<mozilla::layers::GestureEventListener, void (mozilla::layers::GestureEventListener::*)(bool)> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1082:12 (libxul.so+0x4f2090c)
    #9 mozilla::detail::RunnableMethodImpl<mozilla::layers::GestureEventListener*, void (mozilla::layers::GestureEventListener::*)(bool), true, (mozilla::RunnableKind)1, bool>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1133:13 (libxul.so+0x4f2090c)
    #10 mozilla::DelayedRunnable::Notify(nsITimer*) /builds/worker/checkouts/gecko/xpcom/threads/DelayedRunnable.cpp:92:20 (libxul.so+0x3abb41c) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #11 non-virtual thunk to mozilla::DelayedRunnable::Notify(nsITimer*) /builds/worker/checkouts/gecko/xpcom/threads/DelayedRunnable.cpp (libxul.so+0x3abb499) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
[...]
  Location is heap block of size 240 at 0x723c00020c10 allocated by thread T91:
    #0 malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:666:5 (firefox-bin+0xc29f0) (BuildId: a252c599578e106fec1bd0cece1c8d95ebcdee2e)
    #1 moz_xmalloc /builds/worker/checkouts/gecko/memory/mozalloc/mozalloc.cpp:52:15 (firefox-bin+0x14f838) (BuildId: a252c599578e106fec1bd0cece1c8d95ebcdee2e)
    #2 operator new /builds/worker/workspace/obj-build/dist/include/mozilla/cxxalloc.h:33:10 (libxul.so+0x4ec8ba7) (BuildId: a87aaa90bf9315b9417466d61770c6ac16c7a472)
    #3 mozilla::layers::AsyncPanZoomController::AsyncPanZoomController(mozilla::layers::LayersId, mozilla::layers::APZCTreeManager*, RefPtr<mozilla::layers::InputQueue> const&, mozilla::layers::GeckoContentController*, mozilla::layers::AsyncPanZoomController::GestureBehavior) /builds/worker/checkouts/gecko/gfx/layers/apz/src/AsyncPanZoomController.cpp:889:29 (libxul.so+0x4ec8ba7)

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: full TSan log

Comment 2

[Andrew McCreight [:mccr8]]

Hiro, it looks like you've done some work on the destruction of GestureEventListener in the last month, which seems to be related to this race. Any idea what might be going wrong here? Thanks.

It looks like this is a race on mMaxTapTimeoutTask which is a RefPtr so in the worst case it could cause a UAF, but GestureEventListener hasn't changed in about a month so maybe it isn't super easy to trigger so I'll mark it sec-moderate.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Yeah this is indeed a race in GestureEventListener, which, I think, it's been there before I recently touched the code in bug 1919411.

I can think of two solutions here;

a) Add a new mutex into GestureEventListner to guard those runnables by being accessed from multiple threads
b) Defer the destruction of relevant instances such as AsyncPanZoomController etc. to the main-thread in APZCTreeManager::UpdateHitTestingTree

I guess, b) would be a preferable way what Gecko has been doing?

Am I correctly understanding the Gecko's manner that destroying instances on the main-thread is preferable? Andrew?

Comment 4

[Andrew McCreight [:mccr8]]

That sounds reasonable to me, but I think asuth is probably more of an authority on how to deal with this kind of thing.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Seems like the dispatch should probably happen via mozilla::layers::APZThreadUtils::RunOnControllerThread rather than assuming main thread since per the below it sounds like the controller is the android UI thread on android rather than the gecko main thread. Presumably :botond can speak to that better (and might be a better reviewer given his reviews on bug 1919411 that introduced the destroy logic).

Restating (with me having fun with :arai's most recent field-layout updates to reconfirm the TSAN report; I know this was already identified):

• The allocation made here per TSAN is explicitly of a GestureEventListener.
• Searchfox's field-layout for GestureEventListener tells us the offset 0xe0 that the race is on is mozilla::layers::GestureEventListener::mMaxTapTimeoutTask. This lines up with the reported reading checking mMaxTapTimeoutTask and the previous write location nulling it out.
• The field gets initialized here in mozilla::layers::GestureEventListener::CreateMaxTapTimeoutTask with the call to mozilla::layers::AsyncPanZoomController::PostDelayedTask asserting that we're on the controller thread. On desktop that's the main thread, on android that's the Android UI thread where per FSD it sounds like that's explicitly not the main thread.
• I am not an authority for this subsystem, but it seems reasonable to do that given how the existing code is written and since destroy is new.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thank you both! Indeed it should be done on the controller thread. I've posted D270770 to do it, but there's no test. I will try it in gtest.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: (secure)

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: (secure)

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I gave up writing the gtest. D270790 has the gtest I wrote but it's incomplete. It doesn't hit the race. Maybe using NS_DispatchBackgroundTask isn't a good idea.

Comment 11

[Andrew McCreight [:mccr8]]

A comment in bug 1997844 says that some of these failures are being incorrectly classified as bug 1754608, so to get a better sense of the true volume you should look at that.

Comment 13

[Andrew McCreight [:mccr8]]

I'm increasing this to sec-high because it is being seen fairly often in the wild, which indicates it might be exploitable after all.

Comment 14

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: Probably too late for 145 and 146, but it would be good if we could fix this recent-ish regression soon.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9523644 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It would be pretty hard. It's a race condition in between two difference threads, even on GTest I couldn't write the condition
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: 145
• If not all supported branches, which bug introduced the flaw?: Bug 1919411
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It should be applied cleanly to beta/release branches, and it's not risky at all
• How likely is this patch to cause regressions; how much testing does it need?: It unlikely cause any regressions
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 16

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:hiro, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 17

[Tom Ritter [:tjr]]

Comment on attachment 9523644 [details]
(secure)

Approved to land and uplift

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e0bc0218be87
https://hg.mozilla.org/integration/autoland/rev/11939cd85262
Defer destroying mGestureEventListener to the controller thread. r=botond

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/11939cd85262

Comment 20

[Ryan VanderMeulen (PTO, back July 27)]

Please nominate this for Beta & Release uplift when you get a chance.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Thanks, I was about to do it. :)

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D270770

Comment 23

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: The GPU process is crashed by some kind of scenarios that user's touch interactions on a web document and the document gets destroyed
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: The code change is pretty simple and straight forward, it just defers the destruction process to the main-thread to avoid the race between multi threads. So it should not cause any negative side-effects.
• String changes made/needed: None
• Is Android affected?: yes

Comment 24

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined: The GPU process is crashed by some kind of scenarios that user's touch interactions on a web document and the document gets destroyed
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: The code change is pretty simple and straight forward, it just defers the destruction process to the main-thread to avoid the race between multi threads. So it should not cause any negative side-effects.
• String changes made/needed: None
• Is Android affected?: yes

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D270770

Comment 26

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/d331efa8a8c5
https://hg.mozilla.org/releases/mozilla-beta/rev/4f4283be8735

Comment 27

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/fd895a25066b
https://hg.mozilla.org/releases/mozilla-release/rev/3f9d90ed65e9