1284017 - Investigate telemetry probe for lost session files

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

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).

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1284017 - Add telemetry for damaged session store files.

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/

Comment 2

[Jan Henning [:JanH]]

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.

Comment 3

[Jan Henning [:JanH]]

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/

Comment 4

[Jan Henning [:JanH]]

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 5

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 6

[Jan Henning [:JanH]]

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/

Comment 7

[Jan Henning [:JanH]]

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

Comment 8

[Jan Henning [:JanH]]

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/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/30a29ca5fb3e
Add telemetry for damaged session store files. r=sebastian

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30a29ca5fb3e

Comment 11

[Jan Henning [:JanH]]

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Jan Henning [:JanH]]

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.

Comment 15

[Carsten Book [:Tomcat]]

would be cool if we would have a patch that we can uplift to aurora, thanks

Comment 16

[Jan Henning [:JanH]]

Attachment: Bug 1284017 - Aurora.patch

Sure, but as I was saying, I was away from my development machine until today.
In any case, here you are.

Comment 17

[Jan Henning [:JanH]]

... for Aurora.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fd80dfc8f7d