Convert session store to JSONFile async apis

RESOLVED FIXED in Thunderbird 55.0

Status

Thunderbird
General
RESOLVED FIXED
6 months ago
8 hours ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 55.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 months ago
Created attachment 8860752 [details] [diff] [review]
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.
Assignee: nobody → alta88
Attachment #8860752 - Flags: review?(mkmelin+mozilla)
Attachment #8860752 - Flags: review?(acelists)

Comment 2

6 months ago
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
(Assignee)

Comment 3

6 months ago
(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..
(Assignee)

Comment 4

6 months ago
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.
Attachment #8860752 - Flags: review?(mkmelin+mozilla)
Attachment #8860752 - Flags: review?(acelists)
(Assignee)

Comment 5

6 months ago
Created attachment 8860798 [details] [diff] [review]
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().
Attachment #8860752 - Attachment is obsolete: true
Attachment #8860798 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 6

6 months ago
Created attachment 8860801 [details] [diff] [review]
sessionstore.patch

use FileUtils in the test, remove debug.
Attachment #8860798 - Attachment is obsolete: true
Attachment #8860798 - Flags: review?(mkmelin+mozilla)
Attachment #8860801 - Flags: review?(mkmelin+mozilla)

Comment 7

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

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

LGTM! r=mkmelin
Attachment #8860801 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Comment 8

6 months ago
https://hg.mozilla.org/comm-central/rev/4bb4349aaab0e59ae5b46c7e7679595bda7d9fae
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0

Comment 9

8 hours ago
Curious if bug 929406 would be solved from this patch.
You need to log in before you can comment on or make changes to this bug.