Investigate telemetry probe for lost session files

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

Trunk
Firefox 50
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox49 fixed, firefox50 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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).
(Assignee)

Comment 1

3 years ago
Created attachment 8768173 [details]
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/
Attachment #8768173 - Flags: review?(margaret.leibovic)

Updated

3 years ago
Attachment #8768173 - Flags: review?(margaret.leibovic) → review?(s.kaspari)
(Assignee)

Updated

3 years ago
Attachment #8768173 - Flags: review?(s.kaspari)
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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+
(Assignee)

Comment 6

3 years ago
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/
(Assignee)

Comment 7

3 years ago
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
(Assignee)

Comment 8

3 years ago
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

3 years ago
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

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/30a29ca5fb3e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(Assignee)

Comment 11

3 years ago
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?
status-firefox48: --- → affected
status-firefox49: --- → affected
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)
(Assignee)

Comment 14

2 years ago
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)
(Assignee)

Comment 16

2 years ago
Created attachment 8773401 [details] [diff] [review]
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.
Flags: needinfo?(jh+bugzilla)
(Assignee)

Comment 17

2 years ago
... for Aurora.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/8fd80dfc8f7d
status-firefox49: affected → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.