1445403 - FocusState isn't really threadsafe

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

FocusState is owned by APZCTreeManager, but is touched from both the controller and sampler threads. As an example, the Update method is called on the sampler thread at [1], while the ReceiveFocusChangingEvent is called on the controller thread at [2]. There is no locking in FocusState, so there are potentially race conditions and other failures that can result.

Given that FocusState is pretty well self-contained it's probably simplest to just add a Mutex and ensure all the methods hold the mutex.

[1] https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/gfx/layers/apz/src/APZCTreeManager.cpp#475,481
[2] https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/gfx/layers/apz/src/APZCTreeManager.cpp#1866

Comment 1

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

Attachment: Bug 1445403 - Move FocusState method implementations into .cpp file.

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

Comment 2

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

Attachment: Bug 1445403 - Sprinkle some magical Mutex dust on FocusState.

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

Comment 3

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

Attachment: Bug 1445403 - Add thread assertions as documentation.

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

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4afbb0ebd35953d2bf8b7967c718618a4d21fb2 includes these patches

Comment 5

[Ryan Hunt [:rhunt]]

Comment on attachment 8958787 [details]
Bug 1445403 - Move FocusState method implementations into .cpp file.

https://reviewboard.mozilla.org/r/227696/#review233470

Comment 6

[Ryan Hunt [:rhunt]]

Comment on attachment 8958788 [details]
Bug 1445403 - Sprinkle some magical Mutex dust on FocusState.

https://reviewboard.mozilla.org/r/227698/#review233474

Nice, I like the compile time proof in IsCurrent().

Comment 7

[Ryan Hunt [:rhunt]]

Comment on attachment 8958789 [details]
Bug 1445403 - Add thread assertions as documentation.

https://reviewboard.mozilla.org/r/227700/#review233478

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2252a254712
Move FocusState method implementations into .cpp file. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/76ecb95e795f
Sprinkle some magical Mutex dust on FocusState. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/4c417b678b5d
Add thread assertions as documentation. r=rhunt

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d2252a254712
https://hg.mozilla.org/mozilla-central/rev/76ecb95e795f
https://hg.mozilla.org/mozilla-central/rev/4c417b678b5d