1279273 - Main thread waiting for session parsing to finish isn't released after parsing has terminated

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

I am unable to start Nightly for some time (weeks) and have accepted perhaps half a dozen updates in this time.  Starting Nightly shows a white screen with no UI before the system prompts to kill the App.

I finally attached my device and grabbed two logcats, which I will attach.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: |adb logcat| while trying to start Gecko

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: |adb logcat| while trying to start Gecko

Comment 3

[Nick Alexander :nalexander [he/him]]

Further investigation reveals this to be a catastrophic failure during session restore.  I'm not comfortable posting my sessionstore.js publicly, but I've emailed it to Sebastian and JanH.

Comment 4

[Jan Henning [:JanH]]

What happens is that if the main thread arrives here (https://dxr.mozilla.org/mozilla-central/rev/b6f7d0eb61b1878d3d906bd231edf225463ece3f/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1539) before the background thread has finished parsing the session data, it will never be released again even though the background thread is calling notifyAll() (https://dxr.mozilla.org/mozilla-central/rev/b6f7d0eb61b1878d3d906bd231edf225463ece3f/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1304).

The problem is that the session data parsing is running from within a Runnable, so the "this" there is not the same "this" as on the main thread, so notifyAll() naturally won't work.

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1279273 - Use correct synchronization object for session data parsing.

If the main thread has to wait for the session data parsing to finish - which is more likely when you've got many tabs open, meaning a large sessionstore.js file - it does so on "this", i.e. GeckoApp.
The session data parsing itself is done from within a Runnable, so we need to qualify "this" there if we want it to refer to GeckoApp itself, so we can successfully release any waiting threads through notifyAll().

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

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8762403 [details]
Bug 1279273 - Use correct synchronization object for session data parsing.

https://reviewboard.mozilla.org/r/59094/#review56230

Ah, nice!

Comment 7

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64d0241361a0

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8762403 [details]
Bug 1279273 - Use correct synchronization object for session data parsing.

Approval Request Comment
[Feature/regressing bug #]: Mobile session restoring, bug 905223
[User impact if declined]: Firefox might hang during startup, especially for users with lots of open tabs.
[Describe test coverage new/current, TreeHerder]: manual
[Risks and why]: Low, this just ensures that waiting and notifying threads are actually using the same synchronization object.
[String/UUID change made/needed]: none

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/53f5b5c289fb
Use correct synchronization object for session data parsing. r=sebastian

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53f5b5c289fb

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8762403 [details]
Bug 1279273 - Use correct synchronization object for session data parsing.

Recent regression in session store, we definitely want to fix this on aurora 49.

Comment 12

[Carsten Book [:Tomcat]]

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