1282283 - TSan: data race on APZCTreeManager::mRootNode

Status: RESOLVED FIXED

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

Description

[Julian Seward [:jseward]]

This is not easy to reproduce -- was observed when surfing with a
profile with around 100 tabs using a TSan-enabled build.  Sorry I
don't have better STR right now.

It appears that APZCTreeManager::UpdateHitTestingTree sets mRootNode to null
at APZCTreeManager.cpp:174, on the compositor thread

174  mRootNode = nullptr;

whilst APZCTreeManager::ReceiveInputEvent dereferences it, on the
controller thread

698      if (!apzc && mRootNode) {
699        apzc = mRootNode->GetApzc();
700      }

APZCTreeManager::UpdateHitTestingTree does indeed hold
APZCTreeManager::mTreeLock but APZCTreeManager::ReceiveInputEvent does
not.

This strikes me as distinctly ungood ;-)

According to APZCTreeManager.h, comment starting at line 542,
mTreeLock must be held whenever accessing the tree rooted at
mRootNode.  So it seems as if ::ReceiveInputEvent is in error, and
could conceivably be fixed just by putting "MutexAutoLock
lock(mTreeLock);" at the top of APZCTreeManager::ReceiveInputEvent.
Does that sound plausible?

Comment 1

[Julian Seward [:jseward]]

Attachment: TSan complaint

Comment 2

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

Yeah, that looks like a bug in ReceiveInputEvent. We should add a treelock, but I would prefer to scope it just around that hunk of code that reads mRootNode. ReceiveInputEvent does a lot of other stuff and I don't want to hold the lock for the whole function.

Comment 3

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

I can write the patch, it's pretty straightforward.

Comment 4

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

Attachment: Bug 1282283 - Fix data race found by TSan.

Review commit: https://reviewboard.mozilla.org/r/60748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60748/

Comment 5

[Julian Seward [:jseward]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Yeah, that looks like a bug in ReceiveInputEvent. We should add a treelock,
> but I would prefer to scope it just around that hunk of code that reads
> mRootNode. ReceiveInputEvent does a lot of other stuff and I don't want to
> hold the lock for the whole function.

Ok, so I see that in the patch you restrict the scope of the lock just to
that hunk.  I just want to check the following: with the patch, we now hold
the lock when testing |mRootNode| and then calling |mRootNode->GetApzc()|,
and then drop the lock.  Is it possible that the other thread could then
get the lock, set |mRootNode| to null, and thereby cause the |azpc|
value we just pulled out to become invalid?

In other words -- is the locked region you've added, big enough to guarantee
that the |azpc| value obtained from |mRootNode| remains valid for the rest
of its uses in APZCTreeManager::ReceiveInputEvent case MOUSE_INPUT ?

Comment 6

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

The apzc is held by a RefPtr so it won't become garbage. It has some internal pointers that may get nulled out if mRootNode is destroyed but even if that happens the code in ReceiveInputEvent should work fine. Access to member variables inside apzc has other locks to avoid races.

Comment 7

[Julian Seward [:jseward]]

Comment on attachment 8765284 [details]
Bug 1282283 - Fix data race found by TSan.

With that patch, I can no longer reproduce the race.  Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb7b5a8e6124
Fix data race found by TSan. r=sewardj

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

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

Comment on attachment 8765284 [details]
Bug 1282283 - Fix data race found by TSan.

Approval Request Comment
[Feature/regressing bug #]: bug 1211612
[User impact if declined]: Possible race condition that results in a crash while dragging. We haven't seen any reports of this in the wild, but it's a theoretical possibility
[Describe test coverage new/current, TreeHerder]: not easily testable
[Risks and why]: very low risk patch, just adds a bit of locking to avoid the race condition
[String/UUID change made/needed]: none

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8765284 [details]
Bug 1282283 - Fix data race found by TSan.

Fix a race, taking it.
Should be in 48 beta 5

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4d9753fc143

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/54cd52783afc