1358906 - Convert session store to JSONFile async apis

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Comment 1

[alta88]

Attachment: sessionstore.patch

Use JSONFile and async/await instead of sync file reads/writes. Also a bonus tweak to prevent lost session state on failed startups (primarily a dev issue).

r? for whomever has time first.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 8860752 [details] [diff] [review]
sessionstore.patch

Review of attachment 8860752 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgMail3PaneWindow.js
@@ +695,5 @@
>   *
>   * @return true if the restoration was successful, false otherwise.
>   */
> +async function atStartupRestoreTabs(aDontRestoreFirstTab) {
> +  let state =  await sessionStoreManager.loadingWindow(window);

nit: is that two spaces after = ?

::: mail/base/modules/sessionStoreManager.js
@@ +32,5 @@
>  
> +  /**
> +   * Session restored successfully on startup; use this to test for an early
> +   * failed startup which does not restore user tab state to ensure a session
> +   * save on close will not overwrite the last good session state. 

nit: trailing space

@@ -326,5 @@
>      }
>    }
>  };
> -
> -AsyncShutdown.profileBeforeChange.addBlocker(

What is replacing this? Even with await, the application could close before completion

Comment 3

[alta88]

(In reply to Magnus Melin from comment #2)
> Comment on attachment 8860752 [details] [diff] [review]
> sessionstore.patch
> 
> Review of attachment 8860752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ -326,5 @@
> >      }
> >    }
> >  };
> > -
> > -AsyncShutdown.profileBeforeChange.addBlocker(
> 
> What is replacing this? Even with await, the application could close before
> completion

JSONFile has a finalize() to do this - somehow it got lost in the patch..

Comment 4

[alta88]

Comment on attachment 8860752 [details] [diff] [review]
sessionstore.patch


Actually, finalize() and shutdown protection is a freebie with JSONFile, but it requires using saveSoon() a deferred task. But that's just a timer for _save(), which I used instead since it returns a promise and can be used with await. And I did that because otherwise tests were a fail. So either tests are rewritten or it goes back to writeAtomic; the goal was for the read part to use the api moreso than the write part anyway.

Comment 5

[alta88]

Attachment: sessionstore.patch

This goes back to using JSONFile.saveSoon() so shutdown is handled, and tests are changed to get rid of the timer hack and instead call the real thing being tested, _saveState().

Comment 6

[alta88]

Attachment: sessionstore.patch

use FileUtils in the test, remove debug.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8860801 [details] [diff] [review]
sessionstore.patch

Review of attachment 8860801 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! r=mkmelin

Comment 8

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4bb4349aaab0e59ae5b46c7e7679595bda7d9fae

Comment 9

[Ulf Zibis]

Curious if bug 929406 would be solved from this patch.

Comment 10

[Wayne Mery (:wsmwk)]

(In reply to Ulf Zibis from comment #9)
> Curious if bug 929406 would be solved from this patch.

You can find out by using the beta from http://www.mozilla.org/en-US/thunderbird/channel/

Comment 13

[[e-mail]]

Comment on attachment 8860801 [details] [diff] [review]
sessionstore.patch

>-function atStartupRestoreTabs(aDontRestoreFirstTab) {
>+async function atStartupRestoreTabs(aDontRestoreFirstTab) {
Unfortunately you forgot to update the caller and they now get a promise which they mistake for a `true` value...