342454 - SessionStore timer loop

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

It occasionally happens that the timer fires a little too early, leading to an unnecessary timer loop which doesn't stop until the originally intended time interval is reached.

Comment 1

[Simon Bünzli]

Attachment: always save when the timer is fired

The call to saveStateDelayed is unneeded, since the timer is guaranteed to not fire more often than the specified interval (no more than one timer possible and an interval verification before setting the timer).

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 226700 [details] [diff] [review]
always save when the timer is fired

Tested, looks good.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you give us a little more information about what this is fixing, how serious that is, and how risky the patch is?

Comment 4

[Dietrich Ayala (:dietrich)]

(In reply to comment #3)
> Could you give us a little more information about what this is fixing, how
> serious that is, and how risky the patch is?
> 

I apologize for not explaining when asking for 1.8.1 approval.
 
What this is fixing: The timer enforces a minimum time interval between the saving of session data to disk. When the timer fires, there is no need to evaluate whether that interval has passed or not, as that was decided when the timer was set. However, by re-evaluating at that time, it is possible for the timer to be reset for a short period instead of saving, thereby delaying the save unnecessarily.

How serious it is: Now that the user has the expectation that their session can be restored, this could be considered a data-loss issue. If a crash occurs after the extra timer loop starts, then any session state changes during the extra loop would not be saved, and therefore not restored.

How risky the patch is: Low. Session data is saved to disk slightly more often than before.

Comment 5

[Simon Bünzli]

This is really just a zero-risk code path clean up which - as Dietrich explained - omits one unnecessary check which could occasionally lead to a millisecond timer being set several times in short succession.

I originally suspected this timer loop to be responsible for occasional dataloss, but it's probably just a benign annoyance while debugging SessionStore's timer code.

Comment 6

[Mike Schroepfer]

Comment on attachment 226700 [details] [diff] [review]
always save when the timer is fired

Thanks for the more detailed explanation.  Approved for drivers.

Comment 7

[Simon Bünzli]

Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060828 BonEcho/2.0b2 ID:2006082803.