Closed Bug 1031298 Opened 10 years ago Closed 10 years ago

[Session Restore] Measure the number of startups in which all Session Restore files are corrupted.

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: Yoric, Assigned: yarik.sheptykin, Mentored)

Details

(Whiteboard: [lang=js][good second bug])

Attachments

(1 file, 2 obsolete files)

      No description provided.
The SessionStore component is in charge of loading the browser in the same state as it was shutdown. In a few cases, though, the files are corrupted.

We already count how many files are corrupted. Since bug 883609, however, we have several backup files that may be used to recover if a file is corrupted. The objective of this bug is to count how many times we have no way of recovering at all.

This will involve modifying two files:
- http://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json, to define a boolean histogram;
- http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionFile.jsm, method `read`, to record values to the histogram.
Mentor: dteller
Whiteboard: [lang=js][good second bug]
Hi, David!

This sounds like a fun project. I would like to take it. Thanks for giving a detailed explanation of what to do. I will try to work on it in the next couple of days. If I make any feasible progress I will let you know so you can assign this "bug" to me.

Would be really awesome if you could give me a couple of hints on how to craft a test for this case. Maybe giving an example of how a corrupted file could look like and how to trigger the restore process from the test. Or pointing to a testcase where similar happens.
Flags: needinfo?(dteller)
Hi, Everyone!

I am thinking to use test_startup_invalid_session.js as an example test for this case. (http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/unit/test_startup_invalid_session.js). I want to copy it and modify according to this bug. Does it make sense?
Hi Iaroslav and thanks for picking that bug. I'll try and provide more complete answers early next week, but the short answer is: yes, it's a good starting point for testing.
Flags: needinfo?(dteller)
Flags: needinfo?(dteller)
Hello, David!

Thanks for the hint regarding the test. Here is my first patch for this issue. It is probably far from final, but I wanted to make sure that my first attempts go in the right direction.

I implemented your suggestions in comment 1. I created a new xpcshell test using test_startup_invalid_session.js (comment 3) and test_nsTelemetry.js [1] as a reference. The test passes on my setup, however my repo is out of sync since Saturday (syncing and rebuilding takes forever for me). If the patch looks promising, I can sync my repo and run tests against the latest tree.

Looking forward to get your feedback.

1. http://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
Attachment #8493006 - Flags: review?(dteller)
Comment on attachment 8493006 [details] [diff] [review]
A histogram for counting unrestorable sessions and a test for it.

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

Thanks for the patch, this looks good, although it requires a few changes.

You would need two more tests, to ensure that the behavior is correct:
- when at least one of the files is ok (histogram should mark that there is no corruption);
- when none of the session files exist (also no corruption).

Also, could you fix your endlines? I see many whitespaces that should be removed. (you can click on the "review" link, they will be higlighted)

::: browser/components/sessionstore/SessionFile.jsm
@@ +232,5 @@
>          }
>        }
>      }
> +    
> +    // if files were found then see of all were corrupted

"if", not "of" :)
Also, we generally start comments with an uppercase and end with a punctuation.

@@ +234,5 @@
>      }
> +    
> +    // if files were found then see of all were corrupted
> +    if (!noFilesFound) {
> +      // all files are corrupted in none of them could deliver result

I don't understand the sentence.

::: browser/components/sessionstore/test/unit/test_histogram_corrupt_files.js
@@ +1,4 @@
> +/* 
> + * The primary purpose of this test is to ensure that 
> + * the sessionstore component records information about 
> + * corrupted backup files into a histogram.

Could you move this comment outside of the license header?

@@ +6,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ 
> + *
> + */
> + 

"use strict";

@@ +7,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ 
> + *
> + */
> + 
> +const Telemetry = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);

Generally, we use import "resource://gre/modules/Services.jsm" and use `Services.telemetry`.

@@ +8,5 @@
> + *
> + */
> + 
> +const Telemetry = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);
> +Cu.import("resource://gre/modules/osfile.jsm");

Please add `this` as second argument to `Cu.import`. It doesn't matter in this specific test, but that's necessary for Firefox OS-compatible code, so it's a good habit to take.

@@ +24,5 @@
> +  curruptSession.copyTo(profd, Path.join("sessionstore-backups", "previous.js"));
> +  curruptSession.copyTo(profd, Path.join("sessionstore-backups", "recovery.js"));
> +  curruptSession.copyTo(profd, Path.join("sessionstore-backups", "recovery.bak"));
> +
> +  /* not sure if this is relevant

It shouldn't be.

@@ +31,5 @@
> +  //*/
> +
> +  do_test_pending();
> +  let startup = Cc["@mozilla.org/browser/sessionstartup;1"].
> +    getService(Ci.nsISessionStartup);

You probably don't need `startup`.

@@ +35,5 @@
> +    getService(Ci.nsISessionStartup);
> +
> +  afterSessionStartupInitialization(function cb() {
> +    
> +    let h = Telemetry.getHistogramById("FX_SESSION_RESTORE_ALL_FILES_CORRUPT");

You should also check this histogram before startup, to make sure that it's initially 0.

@@ +37,5 @@
> +  afterSessionStartupInitialization(function cb() {
> +    
> +    let h = Telemetry.getHistogramById("FX_SESSION_RESTORE_ALL_FILES_CORRUPT");
> +    let s = h.snapshot();
> +    do_check_eq(1, s.sum);    

It might be more precise to look at `counts` and ensure that we have exactly one hit in `counts[1]` and none in `counts[0]` (iirc).
Attachment #8493006 - Flags: review?(dteller) → feedback+
Clearing the needinfo, as you are obviously on the right track.
Flags: needinfo?(dteller)
Hello, David!

I modified the testfile to include the tests that you suggested in comment 6. I had to rewrite the whole file completely, so please take a close look on the testfile. I hope the approach I have taken with "add_test" functions is correct. 
I had to use OS.File functions to simulate corrupted backup files. They execute without exceptions on my Ubuntu setup but I am not sure about other platforms. I decided not to take care of failing promises if any, and just hope for the best. If these operations fail all tests will fail.
I had to modify my code in SessionFile because it did not pass the test without any backup files.

Looking forward to receive your feedback on this.
Attachment #8493006 - Attachment is obsolete: true
Attachment #8494055 - Flags: review?(dteller)
Comment on attachment 8494055 [details] [diff] [review]
A histogram for counting unrecoverable sessions

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

Looks good, with a few nits.
Do you have L1 access to launch this on the Try server?

::: browser/components/sessionstore/test/unit/test_histogram_corrupt_files.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */

Nit: Can you keep the exact layout for this?

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

We sometimes need to crawl through our source files, for statistics, rewriting, etc. and it's simpler if the copyright header is kept identical, including layout.

@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> + 

Nit: Can you remove that whitespace?

@@ +22,5 @@
> + * In order to use FX_SESSION_RESTORE_ALL_FILES_CORRUPT histogram
> + * it has to be registered in "toolkit/components/telemetry/Histograms.json".
> + * This test ensures that the histogram is registered and empty.
> + */
> +add_test(function test_ensure_histogram_exists_and_empty() {

These days, we prefer using `add_task` instead of `add_test`.
You will need to replace `function` with `function*`.
(same for the other tests)

@@ +25,5 @@
> + */
> +add_test(function test_ensure_histogram_exists_and_empty() {
> +  let h;
> +  try {
> +    h = Telemetry.getHistogramById("FX_SESSION_RESTORE_ALL_FILES_CORRUPT")

Let's make the name of the histogram a constant.

@@ +28,5 @@
> +  try {
> +    h = Telemetry.getHistogramById("FX_SESSION_RESTORE_ALL_FILES_CORRUPT")
> +  } catch (e) {
> +    do_throw("Histogram FX_SESSION_RESTORE_ALL_FILES_CORRUPT does not exist.");
> +  }

Let's not add this `try`/`catch`, as it hides some of the information that may appear in `e`.

@@ +31,5 @@
> +    do_throw("Histogram FX_SESSION_RESTORE_ALL_FILES_CORRUPT does not exist.");
> +  }
> +  let s = h.snapshot();
> +  Assert.equal(s.sum, 0, "Initially, the sum of probes is 0");
> +  run_next_test();

You won't need this if you move to `add_task`.

@@ +38,5 @@
> +
> +/*
> + * Resets the histogram and the contents of the backup directory.
> + */
> +function do_session_reset(backups = {}) {

Generally, we put the definition of these utility functions either at the start or at the end of the file.
Also, could you rename it `reset_session()`?

@@ +49,5 @@
> +  for (let key of SessionFile.Paths.loadOrder) {
> +    if (backups.hasOwnProperty(key)) {
> +      OS.File.copy(backups[key], SessionFile.Paths[key]);
> +    } else {
> +      let removed = OS.File.remove(SessionFile.Paths[key]);

That `let removed` isn't used.

@@ +54,5 @@
> +    }
> +  }
> +}
> +
> +/*

Nit: /**

@@ +68,5 @@
> +    let s = h.snapshot();
> +    Assert.equal(s.counts[0], 1, "One probe for the 'false' bucket.");
> +    Assert.equal(s.counts[1], 0, "No probes in the 'true' bucket.");
> +    run_next_test();
> +  });

If you move to `add_task`, you can rewrite this as follows:

let result = yield SessionFile.read();
let h = ...

and you don't need `run_next_test`.
Attachment #8494055 - Flags: review?(dteller) → feedback+
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #9)
>
Guten Abend, David!
Thanks again that you take time to give a detailed feedback!

> Do you have L1 access to launch this on the Try server?
No, I am new to mozilla. I have found instructions on how to get one (https://www.mozilla.org/hacking/committer/), but it seems it might take some time before I can submit this for testing. Maybe you can do this one last time for me and I will file a request for L1 access the next days.

> Nit: Can you keep the exact layout for this? 
> /* Any copyright is dedicated to the Public Domain.
>  * http://creativecommons.org/publicdomain/zero/1.0/ */
I copied the licence from the other files in the same folder and replaced mine with it. Their layout don"t have a * in the second row.
It's like this:

/* Any copyright is dedicated to the Public Domain.
   http://creativecommons.org/publicdomain/zero/1.0/ */

> Nit: Can you remove that whitespace?
Yes and sorry, I though I got rid of all.

> Nit: /**
I made all function comments to start with /**

> If you move to `add_task`, you can rewrite this as follows:
> 
> let result = yield SessionFile.read();
> let h = ...
I dropped "let result = " as I am not sure what I need it for when the function resumes after yield.
Attachment #8494055 - Attachment is obsolete: true
Attachment #8495497 - Flags: review?(dteller)
Comment on attachment 8495497 [details] [diff] [review]
A histogram for counting unrecoverable sessions

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

Looks good to me.
Let's run this through the Try server: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=266e7e51d1d0
Attachment #8495497 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11)

> Let's run this through the Try server:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=266e7e51d1d0
Hi, David! Thanks for pushing the patch to the try-server. I have been checking the link you posted since Sat (27-09-2014) and it keeps saying:
"Unknown revision ID.
This could be because your push has not been processed yet, or the revision ID could be invalid. This page will refresh occasionally, so your push should show up within a few minutes if it does exist."

It makes me suspect that something went wrong.

On Monday (29-09) I am planning to file a bug for getting L1 access, so I can run tests myself. As far as I understand I need some kind of approval (voucher) from another developer who already has L1 or higher. I was curious if you could vouch for me?
I filed a bug 1074148 for getting L1 access.
Ah, right, there seems to be a problem with my link.
Here's another one that should do better: https://tbpl.mozilla.org/?tree=Try&rev=266e7e51d1d0

This seems to indicate that the patch is fine, so if you are ready, let's mark it checkin-needed.
> This seems to indicate that the patch is fine, so if you are ready, let's
> mark it checkin-needed.

I haven't changed the code since your positive review (comment 11), so I am ready. As far as I recall from bug 1023685 I need to add a "checkin-needed" keyword and write fx-team on the whiteboard. Is that correct? May I do that?
The fx-team stuff will be added by whoever actually lands the patch.
Do you have the Bugzilla privileges to add checkin-needed? I would be a bit surprised, but if you do, yes, please mark the bug as checkin-needed.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #16)

> Do you have the Bugzilla privileges to add checkin-needed? I would be a bit
> surprised, but if you do, yes, please mark the bug as checkin-needed.

Doh!
Sorry, I don't have those privileges.
https://hg.mozilla.org/integration/fx-team/rev/e86ba26dbae9
Assignee: nobody → yarik.sheptykin
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][good second bug] → [lang=js][good second bug][fixed-in-fx-team]
(In reply to Iaroslav Sheptykin from comment #17)
> Doh!
> Sorry, I don't have those privileges.

You do now!
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #19)
> You do now!

Wow, Thanks!
https://hg.mozilla.org/mozilla-central/rev/e86ba26dbae9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good second bug][fixed-in-fx-team] → [lang=js][good second bug]
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: