1288186 - Possible deadlock when restoring session

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

RecentTabsAdapter can be created earlier than we call GeckoProfile.updateSessionFile (through AppCompatActivity.onCreate), and we'll deadlock as a result.

"main" prio=5 tid=1 Waiting
  | group="main" sCount=1 dsCount=0 obj=0x64668ad0 self=0x41ae1928
  | sysTid=6766 nice=0 cgrp=apps sched=0/0 handle=0x4010d154
  | state=S schedstat=( 0 0 0 ) utm=63 stm=5 core=0 HZ=100
  | stack=0xbe66e000-0xbe672000 stackSize=8MB
  at java.lang.Object.wait(Native method)
  - waiting on <0x64fcbc98> (a org.mozilla.gecko.GeckoProfile)
  at org.mozilla.gecko.GeckoProfile.waitForOldSessionDataProcessing(GeckoProfile.java:624)
  - locked <0x64fcbc98> (a org.mozilla.gecko.GeckoProfile)
  at org.mozilla.gecko.home.RecentTabsAdapter.<init>(RecentTabsAdapter.java:78)
  at org.mozilla.gecko.home.CombinedHistoryPanel.onCreate(CombinedHistoryPanel.java:121)
  at android.support.v4.app.Fragment.performCreate(unavailable:-1)
  at android.support.v4.app.FragmentManagerImpl.moveToState(unavailable:-1)
  at android.support.v4.app.FragmentManagerImpl.moveToState(unavailable:-1)
  at android.support.v4.app.FragmentManagerImpl.moveToState(unavailable:-1)
  at android.support.v4.app.FragmentManagerImpl.dispatchCreate(unavailable:-1)
  at android.support.v4.app.FragmentActivity.onCreate(unavailable:-1)
  at android.support.v7.app.AppCompatActivity.onCreate(unavailable:-1)
  at org.mozilla.gecko.GeckoActivity.onCreate(GeckoActivity.java:45)
  at org.mozilla.gecko.GeckoApp.onCreate(GeckoApp.java:1331)
  at org.mozilla.gecko.BrowserApp.onCreate(BrowserApp.java:564)
  at android.app.Activity.performCreate(Activity.java:5231)
  at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1087)
  at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2148)
  at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2233)
  at android.app.ActivityThread.access$800(ActivityThread.java:135)
  at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1196)
  at android.os.Handler.dispatchMessage(Handler.java:102)
  at android.os.Looper.loop(Looper.java:136)
  at android.app.ActivityThread.main(ActivityThread.java:5001)
  at java.lang.reflect.Method.invoke(Native method)
  at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785)
  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601)

Comment 1

[Jan Henning [:JanH]]

Ugh, yes. Am I right in assuming that postToBackgroundThread is still referring to *the* background thread (as opposed to *a* thread pool-like background thread)? In which case if my understanding is right we'd have to do the RecentTabsAdapter's wait from a completely separate thread (as opposed to just moving it into the following background thread runnable), because the call which finally relases that wait (https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1398) is already running on *the* background thread, so if the RecentTabsAdapter is created first, we'd just lock up the background thread instead.

Also I'm wondering whether this might in fact be the reason, behind bug 1282292?

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1288186 - Do the RecentTabsAdapter wait from a separate thread.

When reading the previous session file, the RecentTabsAdapter waits for GeckoApp (initialisation runs on the main thread) to have actually moved the previous session file to its correct location (happens from the background thread). Hence we can't do the wait from either of those two threads, because we risk a deadlock otherwise if the RecentTabsAdapter happens to be initialised *before* GeckoApp.

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

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8774219 [details]
Bug 1288186 - Do the RecentTabsAdapter wait from a separate thread.

https://reviewboard.mozilla.org/r/66548/#review63654

LGTM

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:126
(Diff revision 1)
>              }
>          });
>      }
>  
>      private void readPreviousSessionData() {
> +        final Thread parseThread = new Thread(new Runnable() {

Give the thread a name, either via the constructor or by calling setName.

Comment 4

[Jan Henning [:JanH]]

Comment on attachment 8774219 [details]
Bug 1288186 - Do the RecentTabsAdapter wait from a separate thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66548/diff/1-2/

Comment 5

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54b4ef1cd4a6

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/6f1eb74d5648
Do the RecentTabsAdapter wait from a separate thread. r=jchen

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f1eb74d5648

Comment 8

[Jan Henning [:JanH]]

Hopefully fixed by the Aurora patch from bug 1288816.