Closed Bug 1284017 Opened 3 years ago Closed 3 years ago

Investigate telemetry probe for lost session files

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox48 --- affected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(2 files)

While there's anecdotal evidence of bug 1190627, it would be interesting to see how common a problem this actually is in the wild.

The problem is that without the backups implemented, simply watching for a SessionRestoreException here (https://dxr.mozilla.org/mozilla-central/rev/39dffbba764210b25bfc1e749b4f16db77fa0d46/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1296) introduces some false positives:
- on first startup
- on builds affected by bug 1228593, users who are (theoretically) restoring their tabs, but clearing their history on exist end up with a deleted sessionstore.js
- Should we implement bug 1275662, we'd hit that exception in that case, too

I need to see if I can positively verify this, however if I remember correctly, in those instances were we really lost the session's open tabs without wanting to, we always ended up with not a deleted, but instead an empty sessionstore.js of 0 bytes size, so it might be possible to distinguish that case from the above scenarios by looking for a 0 bytes session file. 
Maybe I could also include any file size >= 0 (i.e. file.exists()), with the exception of a 14 byte file, which corresponds to a minimal session store JS file with an emtpy window (what we would probably get in the case of bug 1275662).
Just watching for a SessionRestoreException during startup can introduce some false positives, because that exception is triggered in any case where we can't restore tabs, not just when the session file has been damaged, e.g.:
- on first startup
- on builds affected by bug 1228593, users who are (theoretically) restoring their tabs, but clearing their history on exist end up with a deleted sessionstore.js
- should we implement bug 1275662, we'd hit that exception in that case, too.

Therefore we only send the telemetry event if we hit that exception even though a sessionstore.js file is present. We also exclude the case where the file size of sessionstore.js is 14 bytes, because that is most likely corresponding to a file containing only {"windows":[]}, which means that the session store intentionally wanted to write a file containing no tabs.

Currently this is only the case for users who are clearing their history on exit and are also *not* restoring tabs, however if bug 1275662 should get implemented, we'd probably encounter those empty files for users who have their restore setting set to "Always restore", too.

Review commit: https://reviewboard.mozilla.org/r/62458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62458/
Attachment #8768173 - Flags: review?(margaret.leibovic)
Attachment #8768173 - Flags: review?(margaret.leibovic) → review?(s.kaspari)
Attachment #8768173 - Flags: review?(s.kaspari)
Bug 1261008 aborts a restore if the only tab in the session file is "about:home" without any session history, so in order to avoid false positives, I need to handle that case, too.
Depends on: 1261008
Comment on attachment 8768173 [details]
Bug 1284017 - Add telemetry for damaged session store files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62458/diff/1-2/
Attachment #8768173 - Flags: review?(s.kaspari)
https://reviewboard.mozilla.org/r/62458/#review60112

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1789
(Diff revision 2)
>                  } else {
> +                    if (parser.allTabsSkipped()) {
> +                        // If we intentionally skipped all tabs we've read from the session file, we
> +                        // set mShouldRestore back to false at this point already, so the calling code
> +                        // can infer that the exception wasn't due to a damaged session store file.
> +                        mShouldRestore = false;

No passing by reference makes this rather convoluted - maybe I should just give in and use a separate member variable in GeckoApp for maximum clarity? Or maybe there's some other, better solution?
Comment on attachment 8768173 [details]
Bug 1284017 - Add telemetry for damaged session store files.

https://reviewboard.mozilla.org/r/62458/#review60714

I think this looks better than what I thought after landing bug 1261008. :)

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1702
(Diff revision 2)
>              // updated before Gecko has restored.
>              if (mShouldRestore) {
>                  final JSONArray tabs = new JSONArray();
>                  final JSONObject windowObject = new JSONObject();
> -                SessionParser parser = new SessionParser() {
> +
> +                class LastSessionParser extends SessionParser {

Mh, if this is not an anonymous class anymore then let's move the class definition outside of the method, to the top of the outter class.
Attachment #8768173 - Flags: review?(s.kaspari) → review+
Comment on attachment 8768173 [details]
Bug 1284017 - Add telemetry for damaged session store files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62458/diff/2-3/
I think our robocop tests for session restoring on startup are disabled anyway, so here we go:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2c02833ea5f&selectedJob=23571011
Keywords: checkin-needed
Comment on attachment 8768173 [details]
Bug 1284017 - Add telemetry for damaged session store files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62458/diff/3-4/
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/30a29ca5fb3e
Add telemetry for damaged session store files. r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/30a29ca5fb3e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment on attachment 8768173 [details]
Bug 1284017 - Add telemetry for damaged session store files.

Approval Request Comment
[Feature/regressing bug #]: mobile session restoring, bug 1190627
[User impact if declined]: Progress on bug 1190627 would be slowed, which means that there continues to remain a risk that the session store file is damaged if Firefox crashes or is otherwise killed, resulting in the user loosing their open tabs.
[Describe test coverage new/current, TreeHerder]: manual
[Risks and why]: Low - with one small exception, the existing session restore flow is maintained, all other changes pertain only to whether we should signal the telemetry flag for that session or not.
[String/UUID change made/needed]: none
Attachment #8768173 - Flags: approval-mozilla-beta?
Attachment #8768173 - Flags: approval-mozilla-aurora?
Comment on attachment 8768173 [details]
Bug 1284017 - Add telemetry for damaged session store files.

This is too late in the 48 cycle but taking it in 49.
Attachment #8768173 - Flags: approval-mozilla-beta?
Attachment #8768173 - Flags: approval-mozilla-beta-
Attachment #8768173 - Flags: approval-mozilla-aurora?
Attachment #8768173 - Flags: approval-mozilla-aurora+
does not apply cleanly to aurora

warning: conflicts while merging toolkit/components/telemetry/Histograms.json! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg revert -a
reverting mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
reverting mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java
reverting toolkit/components/telemetry/Histograms.json
Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg up -C
Flags: needinfo?(jh+bugzilla)
If it's just the histograms.json, then the merge routine is probably just being picky. You can simply add the new histogram entry at the end of the file (similar to how it is done on central), just this time after ABOUTCRASHES_OPENED_COUNT.
If there are other merge conflicts, I'm away until Thursday, so won't be able to provide a dedicated Aurora patch until then.
Flags: needinfo?(jh+bugzilla)
would be cool if we would have a patch that we can uplift to aurora, thanks
Flags: needinfo?(jh+bugzilla)
Sure, but as I was saying, I was away from my development machine until today.
In any case, here you are.
Flags: needinfo?(jh+bugzilla)
... for Aurora.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.