1262387 - Race condition between Tab.setIsRTL/Tab.getIsRTL in Compositor and Gecko threads

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[chhsiao]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160315153207

Steps to reproduce:

I'm currently developing a race detector for unordered messages on Android systems.
The execution I tested had the following steps on an Nexus 4:

1. Visited developers.android.com
2. Scrolled down and pressed "Material Design for Developers"
3. Scrolled down then pressed back.
4. Repeat the above steps a couple times.

This race is found by my race detector so in the actual execution
there was no erroneous behavior but it is a race between two the Compositor thread and the Gecko thread so theoretically it could manifest.



Actual results:

The following lines contain the stack traces between the two accesses in the race:

Write: opcode=+iput-quick addr=1115013672 value=0
[1] at org.mozilla.gecko.Tab.setIsRTL(Tab.java:401)
[2] at org.mozilla.gecko.Tabs.handleMessage(Tabs.java:575)
[3] at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:325)
[4] at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:248)
[5] at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2315)
[6] at org.mozilla.gecko.GeckoThread.run(GeckoThread.java)

Read: opcode=+iget-quick addr=1115013672 value=0
[1] at org.mozilla.gecko.Tab.getIsRTL(Tab.java:405)
[2] at org.mozilla.gecko.gfx.GeckoLayerClient.setFirstPaintViewport(GeckoLayerClient.java:575)

Summary: the write happened in Tab.setIsRTL() when the Gecko thread processed "Tab:ViewportMetadata" to decide if the language is right-to-left upon a tab was added; the read happened in Tab.getIsRTL() when the Compositor thread tried to get the viewport metrics.


Expected results:

Some kind of synchronization should be used to make sure that these two function calls, or to make sure that GeckoLayerClient.setFirstPaintViewport() should be called after "Tab:ViewportMetadata" is processed.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

What version of Firefox for Android are you testing with? If you did not use Nightly would you please retest with it? https://nightly.mozilla.org

Comment 2

[chhsiao]

I'll update my test recently. Thanks!

Comment 3

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

So yes, this appears to be an unsynchronized multithreaded access of the mIsRTL field in Tab.java. However the race will probably not actually manifest in practice, because if the setIsRTL() happens after the getIsRTL(), then the code at [1] will run, which will update the field anyway, via [2]. It's probably a good idea to synchronize access to the mIsRTL field anyway, but not really critical I think.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java?rev=fa90692bddbb#581
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/gfx/LayerView.java?rev=b6683e141c47#711

Comment 4

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.