Closed
Bug 893009
Opened 11 years ago
Closed 11 years ago
Move _backupSessionFileOnce() to the SessionWorker
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 25
People
(Reporter: ttaubert, Assigned: smacleod)
References
(Blocks 1 open bug)
Details
(Whiteboard: [mentor=ttaubert])
Attachments
(1 file, 7 obsolete files)
19.33 KB,
patch
|
Details | Diff | Splinter Review |
SessionStoreInternal._backupSessionFileOnce() is currently used to create a backup of sessionstore.js exactly once, just before we write the session for the first time. This can happen automatically in the SessionWorker without that SessionStore.jsm needs to be aware of it. Just like 'hasWrittenLoadStateOnce' we should keep a flag the tracks whether we already created a sessionstore.js backup. SessionWorker.write() is the function that needs to be modified. It needs to incorporate the functionality of SessionWorker.moveToBackupPath(). The latter can then be removed as we don't need it be part of the API anymore. For clarification: the backup does not create a copy, it moves sessionstore.js to sessionstore.bak just before writing to sessionstore.js the first time. That's way faster than creating a copy if we're going to overwrite the original anyway. This bug moves more logic to the Worker so that SessionStore doesn't need to care about it. It also further reduces the amount of SessionStore <-> Worker communication.
Reporter | ||
Comment 1•11 years ago
|
||
This seems like a perfect bug for Steven :)
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #777906 -
Flags: feedback?(ttaubert)
Reporter | ||
Comment 3•11 years ago
|
||
Comment on attachment 777906 [details] [diff] [review] Patch - Move logic into worker Review of attachment 777906 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks great to me! We need to remove the call site that is left over, though. The (really) superb thing I totally forgot is that we can now actually test this code as the magic happens in _SessionFile and the SessionWorker. Here is a good example of what we can do: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/unit/test_backup.js Basically this means we can write an xpcshell test that calls _SessionFile.write() directly and checks the outcome. The first test should just have the sessionstore.js file and make sure that it's properly moved to sessionstore.bak. The second test can call .write() once again and check that sessionstore.js has the new content and sessionstore.bak didn't change and we didn't throw any errors. The third test can then call .write() again and check that only sessionstore.js has changed because we only backup once. That should be a nice test setup, I hope :) ::: browser/components/sessionstore/src/SessionStore.jsm @@ -458,5 @@ > catch (ex) { debug("The session file is invalid: " + ex); } > } > > - // A Lazy getter for the sessionstore.js backup promise. > - XPCOMUtils.defineLazyGetter(this, "_backupSessionFileOnce", function () { We also need to remove the call site of _backupSessionFileOnce in _saveStateObject().
Attachment #777906 -
Flags: feedback?(ttaubert) → feedback+
Assignee | ||
Comment 4•11 years ago
|
||
Added the removal of the calling code to the patch (Somehow missed it in the last commit). Tests to come in additional patch.
Attachment #777906 -
Attachment is obsolete: true
Attachment #778005 -
Flags: review?(ttaubert)
Reporter | ||
Comment 5•11 years ago
|
||
Comment on attachment 778005 [details] [diff] [review] Patch 1 - Move logic into worker v2 Review of attachment 778005 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ -3801,5 @@ > > - let promise; > - // If "sessionstore.resume_from_crash" is true, attempt to backup the > - // session file first, before writing to it. > - if (this._resume_from_crash) { Sorry, I totally forgot about the resume_from_crash pref. If this is false we shouldn't create a backup on startup. I think the only way we can keep this is by adding a second argument like _SessionFile.write(data, {backupOnFirstWrite: this._resume_from_crash}). The SessionWorker should then not create a backup if backupOnFirstWrite=false. This needs to be only checked once when hasMovedToBackupPathOnce=false.
Attachment #778005 -
Flags: review?(ttaubert) → feedback+
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #778005 -
Attachment is obsolete: true
Attachment #779272 -
Flags: review?(ttaubert)
Reporter | ||
Comment 7•11 years ago
|
||
Comment on attachment 779272 [details] [diff] [review] Patch - Move logic into worker v3 Review of attachment 779272 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionWorker.js @@ +113,5 @@ > * Write the session to disk. > */ > + write: function (stateString, options) { > + if (!this.hasMovedToBackupPathOnce) { > + //if ('backupOnFirstWrite' in options && options.backupOnFirstWrite) { This is commented out? @@ +121,5 @@ > + // Ignore exceptions about non-existent files. > + } > + //} > + > + this.hasMovedToBackupPathOnce = true; The name is not really ideal anymore because the meaning has changed now when we never actually do the backup. Is 'hasWrittenStateOnce' better? hasWrittenOnce? isFirstWrite? ::: browser/components/sessionstore/src/_SessionFile.jsm @@ +67,5 @@ > /** > * Write the contents of the session file, asynchronously. > */ > + write: function (aData, options = {}) { > + return SessionFileInternal.write(aData, options); Nit: Just to stick with the conventions of this file, the argument should be named 'aOptions'. @@ +205,5 @@ > read: function () { > return SessionWorker.post("read").then(msg => msg.ok); > }, > > + write: function (aData, options) { Nit: (see above) ::: browser/components/sessionstore/test/unit/test_backup_once.js @@ +37,5 @@ > + let content = "test_2"; > + let initial_content = decoder.decode(yield OS.File.read(pathStore)); > + let initial_backup_content = decoder.decode(yield OS.File.read(pathBackup)); > + > + yield _SessionFile.write(content, {backupOnFirstWrite: true}); Can you please add another test that calls write(backupOnFirstWrite:false) to check that no backup has been created. And then calls write(backupOnFirstWrite:true) to make sure we don't create a backup on the second write either? I think this needs to be in a new file because we need another instance to test this.
Attachment #779272 -
Flags: review?(ttaubert)
Assignee | ||
Comment 8•11 years ago
|
||
Updated based on feedback from Tim. Also, added an optional |aOptions| parameter to |writeLoadStateOnceAfterStartup()| as well. This method ends up calling |write()| on the worker, so it was needed to pass in |{backupOnFirstWrite: this._resume_from_crash}|
Attachment #779272 -
Attachment is obsolete: true
Attachment #779372 -
Flags: review?(ttaubert)
Reporter | ||
Comment 9•11 years ago
|
||
Comment on attachment 779372 [details] [diff] [review] Patch - Move logic into worker v4 Review of attachment 779372 [details] [diff] [review]: ----------------------------------------------------------------- I don't think that writeLoadStateOnceAfterStartup() should create a backup. This is done very early after startup only to detect startup crashes and doesn't actually change session data - it just flips a flag. You're totally right that it uses .write() as well so we should probably just copy the two lines from write() into writeLoadStateOnceAfterStartup() for now. Bug 888373 will make it obsolete soon. ::: browser/components/sessionstore/src/SessionWorker.js @@ +114,5 @@ > * Write the session to disk. > */ > + write: function (stateString, options = {}) { > + if (!this.hasWrittenState) { > + if ('backupOnFirstWrite' in options && options.backupOnFirstWrite) { Better: if (options && options.backupOnFirstWrite). That doesn't fail when options is null.
Attachment #779372 -
Flags: review?(ttaubert)
Assignee | ||
Comment 10•11 years ago
|
||
Updated the patch based on Tim's review.
Attachment #779372 -
Attachment is obsolete: true
Attachment #779913 -
Flags: review?(ttaubert)
Reporter | ||
Comment 11•11 years ago
|
||
Comment on attachment 779913 [details] [diff] [review] Patch - Move logic into worker v5 Review of attachment 779913 [details] [diff] [review]: ----------------------------------------------------------------- Cool, thank you! Did you push this to try, yet? ::: browser/components/sessionstore/src/SessionWorker.js @@ +57,5 @@ > // the loadState to disk once after startup. > hasWrittenLoadStateOnce: false, > > + // Boolean that tells whether we already made a > + // call to write. We will only attempt to move Nit: 'call to write()'. Makes it clearer that you're talking about the method.
Attachment #779913 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 12•11 years ago
|
||
Updated comment, from Tim's review.
Attachment #779913 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=e07ef3ee8325
Assignee | ||
Comment 14•11 years ago
|
||
Fixed and simplified the broken test. Try: https://tbpl.mozilla.org/?tree=Try&rev=025a918fffc5
Attachment #779934 -
Attachment is obsolete: true
Attachment #781024 -
Flags: review?(ttaubert)
Reporter | ||
Comment 15•11 years ago
|
||
Comment on attachment 781024 [details] [diff] [review] Patch - Move logic into worker v7 Review of attachment 781024 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. I think we can also remove the last 'testNoWriteBackup' test. AFAICT it doesn't actually test anything as the atomic backup test never actually writes, does it? r=me with the minor cleanup. Try looks good as well, let's land this! ::: browser/components/sessionstore/test/browser_833286_atomic_backup.js @@ +56,4 @@ > nextTest(testWriteNoBackup); > } > > function testWriteNoBackup() { We could maybe rename that to testInitialState()? Up to you. @@ +58,5 @@ > > function testWriteNoBackup() { > + // Ensure sessionstore.bak is not created. We start with a clean > + // profile so there was nothing to move to sessionstore.bak before > + // initially writing sessionstore.js Thank you for clarifying this. @@ +76,5 @@ > > nextTest(testWriteBackup); > } > > function testWriteBackup() { That should probably be named testReadBackup(). All we ever do is read stuff.
Attachment #781024 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #15) > This looks great. I think we can also remove the last 'testNoWriteBackup' > test. AFAICT it doesn't actually test anything as the atomic backup test > never actually writes, does it? The way this test was written was a little confusing. I've renamed a number of the functions as you've suggested, and added comments to make it more clear when writes to the session store happen.
Attachment #781024 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6fba91b781ee
Keywords: checkin-needed
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6fba91b781ee
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in
before you can comment on or make changes to this bug.
Description
•