Closed Bug 914581 Opened 7 years ago Closed 7 years ago

[Session Restore] Ensure that we do not lose data when we write it

Categories

(Firefox :: Session Restore, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox26 + fixed
firefox27 + fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss, Whiteboard: [Async][Async Shutdown])

Attachments

(1 file, 3 obsolete files)

As studied in bug 913899, there is a chance that we can lose data written with OS.File. We need a plan of action.
Note that data will not be corrupted.
In the worst case scenario I can imagine, we will lose ~15 seconds of browsing.
Followup to the discussion/patch started on bug 913899. WIP.
Assignee: nobody → dteller
Attachment #802991 - Flags: feedback?(ttaubert)
Attachment #802991 - Attachment is obsolete: true
Attachment #802991 - Flags: feedback?(ttaubert)
Attachment #804347 - Flags: feedback?(ttaubert)
Comment on attachment 804347 [details] [diff] [review]
Ensure that we don't quit while  writing sessionstore.js, v2

Now that the blocker is landing, we can move this to r?.
Tim, I seem to remember that the next uplift is Monday, so I'll need a quick review if we want to land this in FF26.
Attachment #804347 - Flags: feedback?(ttaubert) → review?(ttaubert)
Keywords: dataloss
Whiteboard: [Async][Async Shutdown]
Comment on attachment 804347 [details] [diff] [review]
Ensure that we don't quit while  writing sessionstore.js, v2

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

Sorry for the delay. Conferences and stuff.

I'd be much happier if SessionFile could decide on its own when to close and when not. How about this:

After every SessionFile.write() call we check whether nsIAppStartup.shuttingDown and if that's true we close. That would allow a single write being issued after quit-application has been fired (which would be triggered by uninit()).

That way the SessionStore and SessionSaver could just continue doing their thing without dealing with worker shutdown problems.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +558,5 @@
>      }
>  
>      // save all data for session resuming
>      if (this._sessionInitialized) {
> +      SessionSaver.run({ closeAfterWrite: true });

Calling SessionSaver with 'closeAfterWrite' reveals a little too much of how saving files is implemented. SessionStore.jsm is just interested in saving its session data and shouldn't know about 'closing'.

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +166,5 @@
> +
> +  /**
> +   * |false| once close() has been called
> +   */
> +  _isOpen: true,

We have a function called close() and are checking whether we're closed in write(). Maybe call this 'isClosed'?

@@ +311,5 @@
>    };
>  })();
> +
> +// Since writes proceed off main thread, ensure that we cannot be
> +// interrupted while writing.

Would be great to mention that otherwise our SessionWorker is killed on shutdown before it may have had a chance to write.
Attachment #804347 - Flags: review?(ttaubert) → feedback+
Comment on attachment 804347 [details] [diff] [review]
Ensure that we don't quit while  writing sessionstore.js, v2

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

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +258,5 @@
>          TelemetryStopwatch.cancel("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
>          Cu.reportError("Could not write session state file " + this.path
>                         + ": " + ex);
>        }
> +      this.writeInProgress = null;

We should only clear this field if (this.writeInProgress == promise). Otherwise there's a new pending write that overwrite the field. If we now shut down there's nothing that makes us wait.
(In reply to Tim Taubert [:ttaubert] from comment #6)
> We should only clear this field if (this.writeInProgress == promise).
> Otherwise there's a new pending write that overwrite the field. If we now
> shut down there's nothing that makes us wait.

Actually, let's just not overwrite |this.writeInProgress| with |null|, this will simplify things.
(In reply to Tim Taubert [:ttaubert] from comment #5)
> Comment on attachment 804347 [details] [diff] [review]
> Ensure that we don't quit while  writing sessionstore.js, v2
> 
> Review of attachment 804347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. Conferences and stuff.
> 
> I'd be much happier if SessionFile could decide on its own when to close and
> when not. How about this:
> 
> After every SessionFile.write() call we check whether
> nsIAppStartup.shuttingDown and if that's true we close. That would allow a
> single write being issued after quit-application has been fired (which would
> be triggered by uninit()).

With this, the first write started after nsIAppStartup has decided that we should quit is also the last. Smells like data loss if we have pending e10s collections. I'm not sure we wish to take the chance.

Let me think of alternatives.
We collect data synchronously when quit-application-requested is fired. And we write that data when quit-application is fired. There shouldn't be data loss, right?
Ok, let's go for the simple strategy. It might not be sufficient once we have e10s, but it should hold until then.
Attachment #804347 - Attachment is obsolete: true
Attachment #805255 - Flags: review?(ttaubert)
Comment on attachment 805255 [details] [diff] [review]
Ensure that we don't quit while writing sessionstore.js, v3

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

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +244,5 @@
>                         + ": " + ex);
>        }
> +      // At this stage, we are done writing. If shutdown has started,
> +      // we will want to stop receiving write instructions.
> +      if (this.Services.startup.shuttingDown) {

Nit: this.Services? Just Services?
Attachment #805255 - Flags: review?(ttaubert) → review+
Same one, minus typo.
Attachment #805255 - Attachment is obsolete: true
Attachment #805261 - Flags: review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/661b76876e39
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment on attachment 805261 [details] [diff] [review]
Ensure that we don't quit while writing sessionstore.js, v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 794091.
User impact if declined: Rare case of data loss (sessionstore.js missing some data).
Testing completed (on m-c, etc.): This has been on m-c for about one week.
Risk to taking this patch (and alternatives if risky): This is a simple patch, so I don't think it will fail. However, if for some reason I have messed up that patch, we could theoretically have shutdown freezes.
String or IDL/UUID changes made by this patch: None.
Attachment #805261 - Flags: approval-mozilla-aurora?
Attachment #805261 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking in case this does have any unexpected fallout.
I believe that this shouldn't be tracked anymore, shouldn't it?
Depends on: 922785
You need to log in before you can comment on or make changes to this bug.