1284013 - Improve session store file writing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

This is for landing parts 0 - 3 of bug 1190627, i.e.:
- fixing a small logic error in logging
- using a temp file for synchronous writes
- deferring additional writes if an async write is already running
- reducing save delays while in background

as it might take a little while until the remaining parts will be ready for landing, too.

Comment 1

[Jan Henning [:JanH]]

I'm having second thoughts about the write deferring, as deferring a sync write means to effectively turn it into an async write after all, which could e.g. intermittently reintroduce bug 1228593 if e.g. a write was deferred during quitting. Also I'm no longer quite as sure how much of a problem (theoretically) overlapping write attempts really are in practice.
Hence I want to leave that part either for bug 1190627 or possibly even not at all for the time being, unless it turns out that there really is a problem in that area...

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1284013 - Part 0 - Fix session store logging logic.

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

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1284013 - Part 1 - Use temp file for synchronous writes, too.

Currently, sync writes go directly to the destination file, so an interrupted write will leave the session store data in an inconsistent state. To minimise the incidence of this occurring as far as possible, we now mimic the behaviour of atomicWrite when a tmpPath is set and write to a temporary file which is then renamed to the actual destination file after writing has finished.

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

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1284013 - Part 2 - Reduce session store save delays when in background.

When we are backgrounded and Android's onPause() handler runs, we try to synchronously flush out any pending session store to storage. If however some tab events (e.g. tab closing) have been dispatched shortly before the application backgrounding, it is possible that they'll arrive at the session store after the "application-background" event. In this case, we need to process and write them to storage as fast as possible, as we can be killed at any moment now.

Therefore the delay between successive writes is completely abolished while the application is in background.
The minimum delay between a call to saveStateDelayed() and a write operation however is not completely eliminated and instead only reduced to 200 ms, so as to allow for closely following tab events (e.g. closing a tab involves both a TabSelect and TabClose event) to be batched together in one write operation.

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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8768149 [details]
Bug 1284013 - Part 0 - Fix session store logging logic.

https://reviewboard.mozilla.org/r/62444/#review59690

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8768150 [details]
Bug 1284013 - Part 1 - Use temp file for synchronous writes, too.

https://reviewboard.mozilla.org/r/62446/#review59692

Nice!

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8768151 [details]
Bug 1284013 - Part 2 - Reduce session store save delays when in background.

https://reviewboard.mozilla.org/r/62450/#review59694

::: mobile/android/components/SessionStore.js:266
(Diff revision 1)
>          // pending save state to ensure that this data does not get lost.
>          log("application-background");
> +        // Tab events dispatched immediately before the application was backgrounded
> +        // might actually arrive after this point, therefore save them without delay.
> +        this._interval = 0;
> +        this._minSaveDelay = 200; // Small delay to allow successive tab events to be batched together.

nit: Should this be a constant too?

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8768149 [details]
Bug 1284013 - Part 0 - Fix session store logging logic.

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

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 8768150 [details]
Bug 1284013 - Part 1 - Use temp file for synchronous writes, too.

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

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8768151 [details]
Bug 1284013 - Part 2 - Reduce session store save delays when in background.

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

Comment 11

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32ebbc6475b3

Comment 12

[Carsten Book [:Tomcat]]

this failed to apply:

patching file mobile/android/components/SessionStore.js
Hunk #1 FAILED at 72
1 out of 5 hunks FAILED -- saving rejects to file mobile/android/components/SessionStore.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

can you take a look ? thanks!

Comment 13

[Jan Henning [:JanH]]

Probably a conflict with my other session store patches - I'll look at it later.

Comment 14

[Carsten Book [:Tomcat]]

cool thanks!

Comment 15

[Jan Henning [:JanH]]

Comment on attachment 8768149 [details]
Bug 1284013 - Part 0 - Fix session store logging logic.

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

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 8768150 [details]
Bug 1284013 - Part 1 - Use temp file for synchronous writes, too.

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

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8768151 [details]
Bug 1284013 - Part 2 - Reduce session store save delays when in background.

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

Comment 18

[Jan Henning [:JanH]]

Seems like a simple rebase was enough, didn't have to do any manual merging.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4bdbb2b40d61
Part 0 - Fix session store logging logic. r=sebastian
https://hg.mozilla.org/integration/fx-team/rev/869cab164003
Part 1 - Use temp file for synchronous writes, too. r=sebastian
https://hg.mozilla.org/integration/fx-team/rev/cd9f3cfaa695
Part 2 - Reduce session store save delays when in background. r=sebastian

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bdbb2b40d61
https://hg.mozilla.org/mozilla-central/rev/869cab164003
https://hg.mozilla.org/mozilla-central/rev/cd9f3cfaa695