We call startFetchTabListTask() twice in TabPersistentStore's constructor and once in mergeState(). startFetchTabListTask() creates and starts an AsyncTask via #executeOnExecutor() which must be called from the UI thread. fetchTabList() doesn't exist, so I'm not sure what refactoring you're requesting. Can you please elaborate?

Generally speaking, the tab list needs to be read from disk before the user can do anything in Chrome, so it seems reasonable to me that this would block startup.

Yes, but tasks started in constructor are not blocking UI thread, at least not until loadState().

While mergeState() creates a task and then blocks on it, which is equivalent to running startFetchTabListTask's body synchronously on UI thread.

Regardless of how likely scenario from crbug.com/755042#c31 is, it's just more optimal to avoid using AsyncTask for synchronous calls.

So I'm proposing extracting startFetchTabListTask's body into synchronous fetchTabList(), and call it both from startFetchTabListTask's task and mergeState().

That seems reasonable. Right now the tab state file is being fetched on the serial executor, enforcing ordering with other fetches/reads/writes.

Perhaps we could do something like this to retain execution on the serial executor:

AsyncTask.SERIAL_EXECUTOR.execute(new Runnable() {
  /// fetchTabList()
}

Hmm, I missed the SERIAL_EXECUTOR part.

But reading the code I don't see who is writing to getStateToBeMergedFileName() path, it looks like we're just reading? There is saveListToFile() though, but it writes on the caller's thread (UI?), and synchronizes on SAVE_LIST_LOCK. 

Also, it looks like in the constructor we're potentially starting a task to read getStateToBeMergedFileName() file - can just wait for it in mergeState() if it's there? (And if it's not, then we're free to read synchronously?)



(As an unrelated note - shouldn't we assign mPrefetchTabListTask and mPrefetchTabListToMergeTask to null when we're done with them, to free ByteArrayInputStreams they are holding?)

> But reading the code I don't see who is writing to getStateToBeMergedFileName() path, it looks like we're just reading? There is saveListToFile() though, but it writes on the caller's thread (UI?), and synchronizes on SAVE_LIST_LOCK. 


The "merged" state file is used in multi-instance. Each ChromeTabbedActivity has its own state file. When multi-instance is exited (or on cold start), whichever ChromeTabbedActivity* is running in fullscreen will read the tab state file of the other instance. If the activity writes to tab_state0, it will read tab_state1 during a merge and vice-versa.


> Also, it looks like in the constructor we're potentially starting a task to read getStateToBeMergedFileName() file - can just wait for it in mergeState() if it's there? (And if it's not, then we're free to read synchronously?)

We initiate merges from two different places. TabPersistentStore's constructor handles cold-start merging. TabPersistentStore never actually calls mergeState(); it uses mPrefetchTabListToMergeTask to do its merge. If the mPrefetchTabListToMergeTask is present in mergeState(), it means we're not done with the initial tab load and we'll drop out of mergeState() early (due to the return on line 413).

mergeState() is called via ChromeTabbedActivity when multi-window mode is exited. When ChromeTabbedActivity is initially started, there may be no second tab sate file. If the user enters multi-window mode, then creates a second Chrome window (entering multi-instance), a second tab state file is created. When mergeState() is triggered, we look for this second tab state file.



>(As an unrelated note - shouldn't we assign mPrefetchTabListTask and mPrefetchTabListToMergeTask to null when we're done with them, to free ByteArrayInputStreams they are holding?)

Sure, that seems like the right thing to do.

Reusing the bug to clean up mPrefetchTabListTask and mPrefetchTabListToMergeTask variables.