852267 - Interruption during session restore results in loss of session data

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[CoJaBo]

There are several distinct ways to reproduce this, but I suspect they're all symptoms of the same underlying issue (if not, they can be split off into other bugs).
The problem is severe; it essentially means sessionstore is not reliable at all, and that tabs that aren't bookmarked could be lost at any moment.
I've noticed this behavior on the Desktop version as well, but it's generally not an issue there, as an interruption during startup is not a regular event.

Steps to Reproduce:
1. Open several tabs
2. Make Firefox unload (e.g., by switching to a memory-intensive app/game/phone call; this can also be forced by switching away [to the homescreen] and then killing it with a task manager)
3. Open Firefox; it will begin to recover tabs automatically to preserve the open session
4. Immediately, Make Firefox unload again
5. Open Firefox again

Steps to Reproduce (alternate):
1. Open several tabs
2. Exit Firefox (exit option, reboot, etc)
3. Open Firefox; it will display the "tabs from last time" page
4. Click to restore all tabs
5. Immediately, Make Firefox unload again
6. Open Firefox again

Expected: All tabs are still present
Actual: Some tabs are missing, or load about:blank instead of the actual page
Recurrence: Often, but not always; depends on timing
User Impact: Receiving a phone call right after switching to Firefox risks data loss


Steps to Reproduce:
1. Open several tabs
2. Exit Firefox (exit option, reboot, etc)
3. Open Firefox; it will display the "tabs from last time" page
4. Unload or exit Firefox
5. Open Firefox again

Expected: There is some way to restore the tabs
Actual: The session is lost forever
Recurrence: Always
User Impact: Receiving a phone call at the "tabs from last time" page always results in data loss

Comment 1

[liam]

This was filed on my behalf after my response to bug 656062.

Comment 2

[linne]

Hi, I'd like to know if anyone is fixing on this bug now, if not can I work on it?

If anyone have editing privileges see this, please assign the bug to me. 

Thanks!

Comment 3

[Brian Nicholson (:bnicholson)]

I was going to look into this, but if you're interested, you're welcome to take it!

To fix this bug, I was thinking we could do something along the lines of merging the session data from last time into the current session immediately at startup. That way, even if the tab hasn't loaded yet (which is what causes its session data to be added to the session state object), it will still be saved in case Fennec is closed again. Were you thinking of something similar?

If you'd like me to mentor you on this bug, let me know and I can help walk you through it. Or if you already know what you're doing, feel free to tackle it, and let me know if you have any questions. Thanks!

Comment 4

[linne]

Hi Brain;
I'm interested with this bug and I agree with what were you thinking. I'm very glad you could help me work though the problem and let me know the right way to go. 

Thanks for your time!



(In reply to Brian Nicholson (:bnicholson) from comment #3)

Comment 5

[Bruce Wu]

I agree with Brian, saving session data at the beginning of page load instead of the end would definitely fix this issue. However I notice in Fennec documentation that the reason for saving the session at the end of page load is to improve page loading performance, which is crucial especially on mobile devices. 

Therefore, is it possible to do an session store in the onDestroy part of the browser app ? So we will store current session both when a page is finish loading, and when the app is crashed / oom / terminated.

Comment 6

[linne]

Hi Brain: is the store session  in the android/chrome content/browser.js  addTab function? <Line 784>? 
I wanna make sure I'm work on the right track.
Thanks

Linne

Comment 7

[Brian Nicholson (:bnicholson)]

Hi Linne,

The session store logic is mostly contained in this file [1]. The session is written out at [2] and restored at [3]. There's also code at [4] that parses the session data and creates tab "stubs" that are created before Gecko has loaded.

This code is actually fairly complex since it's split between multiple places (Java and JS) and is intricately woven into the startup code. If you aren't very familiar with the codebase, I'd probably recommend looking for a simpler bug. I'm not sure this bug makes a good bug to mentor -- I'm not sure myself what the correct solution is yet!

That said, you can continue to work on this if you'd like, but it will probably take a good bit of studying/experimentation to reach a working patch. If not, I can take this over since I've been working on a number of other session-related bugs recently.

In case you want to take something more straightforward, you can look over the list of open unassigned bugs at [5]. Unfortunately, all mentor-designated bugs are already assigned, but I'm sure you could find something in that list.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#454
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#961
[4] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1471
[5] https://bugzilla.mozilla.org/buglist.cgi?list_id=6665521&resolution=---&emailtype1=exact&query_format=advanced&emailassigned_to1=1&email1=nobody%40mozilla.org&product=Firefox%20for%20Android

Comment 8

[Bruce Wu]

Attachment: Added saveState during quit-Application

Hi Brian, I am Linne's group partner for this bug. This is our simple attempt to address this issue with minimal change, it solved the problem pretty well during our testing. 

Please let us know what you think!

Thanks,

Bruce

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 756455 [details] [diff] [review]
Added saveState during quit-Application

Hi Bruce,

I think you uploaded the wrong patch. This one only contains:

-    let history = aBrowser.sessionHistory;S
+    let history = aBrowser.sessionHistory;

Comment 10

[Bruce Wu]

Attachment: added saveState during quit-Application

opps, the change is actually contained in this patch file

Comment 11

[linne]

Hi Brian: we  simply added a saveState during quit-application stage, it works in our testing, Let we know what you think!

Linne

Comment 12

[Brian Nicholson (:bnicholson)]

There are two parts to this bug: interrupted session restore, and interrupted load of tabs from last time. I used to be able to see this problem during session restores (part 1); however, I can no longer reproduce them, and I believe they were fixed by bug 869413. Before bug 869413 was fixed, we were firing off many simultaneous page loads at startup, and when the restore was interrupted, pages would either not load at all, be replaced with about:blank, or other weird symptoms.

But now that bug 869413 has landed, I can no longer reproduce the session restore aspect of this bug. I do, however, still run into problems if I try to load all tabs from last time and it gets interrupted. CoJaBo, do you still have problems with interrupted session restore, or do you only see this now when loading tabs from last time?

Linne/Bruce, which of the two parts does your patch fix? Can you elaborate on which steps you were using to reproduce this bug, and what happens when your patch is applied? Note that we already fire off a saveState() in quit-application if there's a write pending: http://hg.mozilla.org/mozilla-central/file/8b1bfcf0ce6e/mobile/android/components/SessionStore.js#l175. Also, in Android, we need to be ready to be killed by the OS at any time; normally, we won't even be receiving these shutdown events to begin with.

Comment 13

[Bruce Wu]

We are targeting #2, loading tabs from last time.

My steps to reproduce the bug in current nightly:

1. Open a page (preferably a page with long loading time)
2. Before the page finish loading, click home button to go to android home screen
3. Terminate Fennec
4. Reopen Fennec, notice the page that was loading do not appear in "tabs from last time" (located at the lower part of the page).

My observation:

Only page that is completely loaded, will be in "tabs from last time" after Fennec is terminated. 

My initial thought is to saveState in the beginning of a page load, rather then when
a page is finish loading. However this will reduce the performance of page loading. Also,
I see in: http://dxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js
line#333, a saveState() is called when a new tab is added (open). I am curious how come this
line seems have no effect. 

Our Patch:

The ideal location to save session data before closing the app is actually in onPause(): http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java
(Because onStop() and onDestroy() is not always called).

However I haven't figure out how to use sessionRestore in BrowserApp.java... I think what
Fennec is doing right now, is having a listener passing status back to sessionRestore.js,
and perform actions accordingly?

Brian, you mention a saveState() is already fired in quit-application.. but http://hg.mozilla.org/mozilla-central/file/8b1bfcf0ce6e/mobile/android/components/SessionStore.js#l175 is in a condition statement, and is only fired if a save has already been queued.... right ?


This is why I added this.saveStateDelayed() in http://dxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js line #185 ... to make sure state is always save when application is quitting

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Bruce Wu from comment #13)
> 
> My initial thought is to saveState in the beginning of a page load, rather
> then when a page is finish loading.

Note that saveState() only saves the _SS_data state that is already attached to each tab (you can see that in _collectWindowData() [1]). It doesn't actually iterate the set of all tabs in the browser to collect the session data (since this would be too costly). _SS_data is only updated onTabLoad(), which is a callback for pageshow [2]. When you click "open all tabs from last time" and then immediately close the browser, __SS_data won't yet exist for the tabs since those pages haven't loaded. So even if you were to call saveState() earlier, it wouldn't help -- __SS_data must be first be updated for the state to be saved.

[1] http://hg.mozilla.org/mozilla-central/file/204de5b7e0a6/mobile/android/components/SessionStore.js#l542
[2] http://hg.mozilla.org/mozilla-central/file/204de5b7e0a6/mobile/android/components/SessionStore.js#l265

> Also, I see in:
> http://dxr.mozilla.org/mozilla-central/source/mobile/android/components/
> SessionStore.js
> line#333, a saveState() is called when a new tab is added (open). I am
> curious how come this seems have no effect. 

The main reason is what I just said above. saveState() only writes out the state that has already been collected (which is __SS_data). Here's a quick rundown of what's happening.

1) Browser starts
2) __SS_data is null
3) A tab is added
  a) saveState() is called, but nothing is written since __SS_data is null
4) The page loads
  a) The pageshow event is fired
  b) This calls onTabLoad()
  c) This calls _collectTabData()
  d) This sets __SS_data for the tab
  e) saveState() is called, and written successfully since __SS_data has the tab data


> The ideal location to save session data before closing the app is actually
> in onPause():
> http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java
> (Because onStop() and onDestroy() is not always called).

onPause() is normally the ideal place to save state for Android applications (and we do in fact trigger a session restore in onPause()). But we want to save state even more frequently than that in case the browser crashes or is killed in the foreground, in which case onPause() wouldn't be called.

> Brian, you mention a saveState() is already fired in quit-application.. but
> http://hg.mozilla.org/mozilla-central/file/8b1bfcf0ce6e/mobile/android/
> components/SessionStore.js#l175 is in a condition statement, and is only
> fired if a save has already been queued.... right ?

Yes, and a saveState() will already be queued if there are any changes worth saving.


> This is why I added this.saveStateDelayed() in
> http://dxr.mozilla.org/mozilla-central/source/mobile/android/components/
> SessionStore.js line #185 ... to make sure state is always save when
> application is quitting

As you've already pointed out, onStop()/onDestroy() aren't guaranteed to be called. There's no way to know when the app is about to be killed, and that means these quit-application/shutdown events are not nearly as useful on Android because they aren't always called (and if you add logging, you'll probably find that these events are never fired for the STR you posted).


But as I've said, saveState() is only useful when we've already collected the state that needs to be saved. You could call saveState() 100 times before Fennec dies, but it won't actually help unless __SS_data has been updated (i.e., the the page has loaded and triggered pageshow).

I recommend adding lots of dump() calls into SessionStore.js to log the different callbacks (onTabLoad, saveState, etc) so you can see when/what is being saved. In saveState(), I recommend dumping the JSON to the log file so you can the exact session being written.

To fix this bug, you'll need to build __SS_data sooner. We already create placeholder tab data if the tab is opened using the delayLoad flag [3]. You might simply be able to move that chunk to outside of the if statement so all tabs are initialized with basic __SS_data.

[3] http://hg.mozilla.org/mozilla-central/file/204de5b7e0a6/mobile/android/chrome/content/browser.js#l2577

Comment 15

[Brian Nicholson (:bnicholson)]

Comment on attachment 756613 [details] [diff] [review]
added saveState during quit-Application

I don't think this patch will be sufficient since we're unlikely to even receive this event. Let's try updating __SS_data sooner as described in comment 14.

Comment 16

[Brian Nicholson (:bnicholson)]

Hi Bruce and Linne, are you guys still working on this bug?

Comment 17

[Jan Henning [:JanH]]

I've split of the last part of this bug (tabs from last time getting lost if Firefox is opened and killed even if the user didn't have any real chance of interacting with the session) into bug 1260005.

(In reply to Brian Nicholson (:bnicholson) from comment #14)
> To fix this bug, you'll need to build __SS_data sooner. We already create
> placeholder tab data if the tab is opened using the delayLoad flag [3]. You
> might simply be able to move that chunk to outside of the if statement so
> all tabs are initialized with basic __SS_data.
> 
> [3]
> http://hg.mozilla.org/mozilla-central/file/204de5b7e0a6/mobile/android/
> chrome/content/browser.js#l2577

While it is debatable whether we shouldn't create some basic session store data for all new tabs right away, in this case browser.js is actually a red herring.

With "Restore tabs" set to "Always restore", we normally don't even create any new tabs, but use the tab stubs created by the Java UI.
The actual problem is that when recreating the tabs, we only attach the old session store data for the background tabs which are created in zombified state, because otherwise delay loading them wouldn't work.
We don't attach it for the foreground [1] tab [2], and neither do we attach it when restoring a/all tab(s) from the recent tabs page [3]. Instead we rely on the fact that the session store data will be recreated anyway when tab loading has progressed sufficiently far to fire the DOMTitleChanged event. However until that time, those freshly restored tabs don't have any session store data attached to them, so if a session save is triggered and then Firefox subsequently killed during that period, those tabs will be missed and won't be saved to the session store.

In fact, thinking of the circumstances in which I have observed tabs getting lost, Firefox probably doesn't even have to be killed for this bug to be triggered. Switching to a different tab and triggering a memory pressure event - which will zombify all background tabs - is enough, because thanks to bug 1044556, the session store won't receive any subsequent events from once zombified tabs. So if a restored tab becomes zombified before the DOMTitleChanged event fired, its session store data will be empty and remain so forever.

[1] at the moment Firefox loads and session restore is initiated
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#1097
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#964 and https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js#964

Comment 18

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 852267 - Part 1 - Always attach session store data to restored tabs. r=margaret

Up till now, when restoring tabs, the previous session store data was only attached to the tab object where necessary to make delay loaded tabs work. In all other cases, the session store data was left empty and recreated only once page loading had progressed sufficiently far to fire the DOMTitleChanged event. This means that a tab could get lost if the session was saved before this point and Firefox then subsequently killed. In conjunction with bug 1044556, a memory pressure event could also prevent the session store data from being recreated for a tab, because the session store wouldn't recieve any new events for tabs that had been zombified.

Therefore, with this patch we always attach the previous session store data to a freshly restored tab, so it will always be included in a session save even if the save is triggered before tab loading has progressed far enough.

Review commit: https://reviewboard.mozilla.org/r/42785/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42785/

Comment 19

[:Margaret Leibovic]

Comment on attachment 8735459 [details]
MozReview Request: Bug 852267 - Part 1 - Always attach session store data to restored tabs. r=margaret

https://reviewboard.mozilla.org/r/42785/#review39615

Sorry for the slow review, I needed to read through all the session restore code to re-familiarzie myself with how this works. I'm still a bit concerned that I don't understand all the implications of this change, but I think it would be worth cleaning up some of this logic while we're in here, rather than papering over this issue with a seemingly simple patch.

::: mobile/android/components/SessionStore.js:1004
(Diff revision 1)
>      aBrowser.__SS_restore_data = aTabData || {};
> +
> +    // Attach the old session store data, so the tab doesn't get lost if we're
> +    // killed during restoration and onTabLoad() hasn't yet been called for the
> +    // freshly restored tab.
> +    aBrowser.__SS_data = aTabData;

This should probably be `aTabData || {}`, in case `aTabData` is null or undefined.

It's confusing that we have two variables, `__SS_restore_data` and `__SS_data`, and they seem to represent the same thing (here they literally represent the same thing!). Can we clean this up?

Looking through the code path where this `_restoreTab` method gets called, in many cases this call is redundant, since `aTabData` is passed in as `aBrowser.__SS_data`.

Can we instead update the call sites where this would actually make a difference? I think at least one of the relevant call sites is this one:
http://hg.mozilla.org/mozilla-central/annotate/d5d53a3b4e50/mobile/android/components/SessionStore.js#l1097

Instead of adding the line in this method, could we just update the call site to set `__SS_data` regardless of whether or not `_restoreTab` is called?

Comment 20

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/42785/#review39615

> This should probably be `aTabData || {}`, in case `aTabData` is null or undefined.
> 
> It's confusing that we have two variables, `__SS_restore_data` and `__SS_data`, and they seem to represent the same thing (here they literally represent the same thing!). Can we clean this up?
> 
> Looking through the code path where this `_restoreTab` method gets called, in many cases this call is redundant, since `aTabData` is passed in as `aBrowser.__SS_data`.
> 
> Can we instead update the call sites where this would actually make a difference? I think at least one of the relevant call sites is this one:
> http://hg.mozilla.org/mozilla-central/annotate/d5d53a3b4e50/mobile/android/components/SessionStore.js#l1097
> 
> Instead of adding the line in this method, could we just update the call site to set `__SS_data` regardless of whether or not `_restoreTab` is called?

Will change it to `aTabData || {}`, although judging from [Bug 1229259 comment 10](https://bugzilla.mozilla.org/show_bug.cgi?id=1229259#c10), the history restore will still keel over if `aTabData` is null or undefined, so there needs to be at least a null/length check before calling that method as well.

Regarding the call sites of `_restoreTab`, it looks like it's getting called from four places:
1. `onTabSelect`: Called when switching to a zombified tab. The zombified tab already has its session data attached, which is then extracted and passed to `_restoreTab`. In this case, reattaching the session data in `_restoreTab` would indeed be duplicated work. Doesn't do anything with the extra data (`__SS_extdata`, although currently I don't think anybody is actually using it to store/retrieve anything), but then it doesn't have to, because a zombified tab already has all its session store data present.
2. `_restoreTabs`: Called when restoring a tab from the recent tabs page. Currently, no session data is attached to the restored tabs and the extra data isn't restored, either (needs fixing).
3. `_restoreWindow`: Called if restoring tabs during start-up. Restores all tab data for background tabs, but only the extra data for the foreground tab.
4. `undoCloseTab`: Called from the snack bar that appears after closing a tab. Only restores the extra data.

So moving the re-attaching of the session data to the tab (both normal and extra data) would only lead to duplicated functionality in case 1.), whereas for cases 2.) through to 4.) it could be fixed in one central place. But it's your call - if you want, I can simply add the missing calls to restore the data in 2.) - 4.) as you proposed.

Regarding the apparent duplication between `__SS_restore_data` and `__SS_data`, `__SS_data` is what is actually captured by `_saveState`, whereas `__SS_restore_data` only holds a temporary copy used in restoring any form data that might be present.

Let's say we have a tab with some form data which we want to restore. This is roughly what happens:
- A new tab object is created for the URL to be restored. The new tab object doesn't have any session store data, so if a session save is triggered at this stage (e.g. because the user switches to some other tab), the tab won't be included in the new sessionstore.js and will consequently get lost if we're killed by any reason. (Actually it might be worth after all to take in [Brian's suggestion about moving this line](https://dxr.mozilla.org/mozilla-central/rev/d5d53a3b4e50b94cdf85d20690526e5a00d5b63e/mobile/android/chrome/content/browser.js#3479) to always run regardless of the state of `aParams.delayLoad`, so new tabs are always initialised with a minimum amout of session store data and will be included in the session store as soon as they're created).
- `_restoreTab` is called and restores the tab history for the UI and/or Gecko from the session store data. A copy of the session store data (which includes any saved form data) is saved into `__SS_restore_data`. At this point, `__SS_data`, will still be empty without my patch.
- Tab loading continues and eventually the `DOMTitleChanged` event fires, which runs `onTabLoad` which in turn runs `_collectTabData`. This populates `__SS_data` (and overwrites any previous data, if present), so the tab will finally be included in any session save activity. However `__SS_data` won't contain any saved form data, because a) the freshly loaded page doesn't have any user-entered form data yet and b) because `_collectTabData` doesn't collect any form data anyway.
- Finally tab loading completes and the `load` event fires. This triggers `_restoreTextData`, which restores the form data from the temporary copy in `__SS_restore_data` and then deletes it.

Things that come into my mind are:
- `__SS_restore_data` could be renamed to make its purpose clearer
- It might be possible to save only a copy of the form data and not the whole session data.
- Even with my currently proposed patch, there is a window between the `DOMTitleChangedEvent` firing and the next time `onTabInput` runs (probably at some point after the data has been restored in the `load` event), where the saved form data could get lost if a session save is triggered.

I'll try and put something together which addresses all the above points.

Comment 21

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/42785/#review39615

> Will change it to `aTabData || {}`, although judging from [Bug 1229259 comment 10](https://bugzilla.mozilla.org/show_bug.cgi?id=1229259#c10), the history restore will still keel over if `aTabData` is null or undefined, so there needs to be at least a null/length check before calling that method as well.
> 
> Regarding the call sites of `_restoreTab`, it looks like it's getting called from four places:
> 1. `onTabSelect`: Called when switching to a zombified tab. The zombified tab already has its session data attached, which is then extracted and passed to `_restoreTab`. In this case, reattaching the session data in `_restoreTab` would indeed be duplicated work. Doesn't do anything with the extra data (`__SS_extdata`, although currently I don't think anybody is actually using it to store/retrieve anything), but then it doesn't have to, because a zombified tab already has all its session store data present.
> 2. `_restoreTabs`: Called when restoring a tab from the recent tabs page. Currently, no session data is attached to the restored tabs and the extra data isn't restored, either (needs fixing).
> 3. `_restoreWindow`: Called if restoring tabs during start-up. Restores all tab data for background tabs, but only the extra data for the foreground tab.
> 4. `undoCloseTab`: Called from the snack bar that appears after closing a tab. Only restores the extra data.
> 
> So moving the re-attaching of the session data to the tab (both normal and extra data) would only lead to duplicated functionality in case 1.), whereas for cases 2.) through to 4.) it could be fixed in one central place. But it's your call - if you want, I can simply add the missing calls to restore the data in 2.) - 4.) as you proposed.
> 
> Regarding the apparent duplication between `__SS_restore_data` and `__SS_data`, `__SS_data` is what is actually captured by `_saveState`, whereas `__SS_restore_data` only holds a temporary copy used in restoring any form data that might be present.
> 
> Let's say we have a tab with some form data which we want to restore. This is roughly what happens:
> - A new tab object is created for the URL to be restored. The new tab object doesn't have any session store data, so if a session save is triggered at this stage (e.g. because the user switches to some other tab), the tab won't be included in the new sessionstore.js and will consequently get lost if we're killed by any reason. (Actually it might be worth after all to take in [Brian's suggestion about moving this line](https://dxr.mozilla.org/mozilla-central/rev/d5d53a3b4e50b94cdf85d20690526e5a00d5b63e/mobile/android/chrome/content/browser.js#3479) to always run regardless of the state of `aParams.delayLoad`, so new tabs are always initialised with a minimum amout of session store data and will be included in the session store as soon as they're created).
> - `_restoreTab` is called and restores the tab history for the UI and/or Gecko from the session store data. A copy of the session store data (which includes any saved form data) is saved into `__SS_restore_data`. At this point, `__SS_data`, will still be empty without my patch.
> - Tab loading continues and eventually the `DOMTitleChanged` event fires, which runs `onTabLoad` which in turn runs `_collectTabData`. This populates `__SS_data` (and overwrites any previous data, if present), so the tab will finally be included in any session save activity. However `__SS_data` won't contain any saved form data, because a) the freshly loaded page doesn't have any user-entered form data yet and b) because `_collectTabData` doesn't collect any form data anyway.
> - Finally tab loading completes and the `load` event fires. This triggers `_restoreTextData`, which restores the form data from the temporary copy in `__SS_restore_data` and then deletes it.
> 
> Things that come into my mind are:
> - `__SS_restore_data` could be renamed to make its purpose clearer
> - It might be possible to save only a copy of the form data and not the whole session data.
> - Even with my currently proposed patch, there is a window between the `DOMTitleChangedEvent` firing and the next time `onTabInput` runs (probably at some point after the data has been restored in the `load` event), where the saved form data could get lost if a session save is triggered.
> 
> I'll try and put something together which addresses all the above points.

Scratch that comment about the extra data, it looks like it's used somehow in conjunction with pinned tabs.

Comment 22

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 852267 - Part 2 - Add a null check before restoring history. r=margaret

Normally we shouldn't get into a situation where aTabData will be null/empty when trying to restore history, but bug 1229259 comment 10 shows that this can somehow still happen.

Review commit: https://reviewboard.mozilla.org/r/43859/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43859/

Comment 23

[Jan Henning [:JanH]]

Comment on attachment 8735459 [details]
MozReview Request: Bug 852267 - Part 1 - Always attach session store data to restored tabs. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42785/diff/1-2/

Comment 24

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/42785/#review39615

> Scratch that comment about the extra data, it looks like it's used somehow in conjunction with pinned tabs.

I've added the code to put back the previous session data individually to the three relevant callsites. I've also split off cleaning up the `__SS_restore_data` handling I mentioned above into [bug 1261225](https://bugzilla.mozilla.org/show_bug.cgi?id=1261225).

Comment 25

[:Margaret Leibovic]

Comment on attachment 8735459 [details]
MozReview Request: Bug 852267 - Part 1 - Always attach session store data to restored tabs. r=margaret

https://reviewboard.mozilla.org/r/42785/#review40495

Thanks for the detailed explanation! This looks good to me.

Comment 26

[:Margaret Leibovic]

Comment on attachment 8737271 [details]
MozReview Request: Bug 852267 - Part 2 - Add a null check before restoring history. r=margaret

https://reviewboard.mozilla.org/r/43859/#review40497

::: mobile/android/components/SessionStore.js:999
(Diff revision 1)
>    */
>    _restoreTab: function ss_restoreTab(aTabData, aBrowser) {
> +    // aTabData shouldn't be empty here, but if it is,
> +    // _restoreHistory() will crash otherwise.
> +    if (!aTabData || aTabData.entries.length == 0) {
> +      return;

I think we should report an error here, since this will probably still result in a broken state for the user.

::: mobile/android/components/SessionStore.js:1006
(Diff revision 1)
>      this._restoreHistory(aTabData, aBrowser.sessionHistory);
>  
>      // Restoring the text data requires waiting for the content to load. So
>      // we set a flag and delay this until the "load" event.
>      //this._restoreTextData(aTabData, aBrowser);
>      aBrowser.__SS_restore_data = aTabData || {};

With this change, we won't need the `|| {}` case anymore.

Comment 27

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/43859/#review40497

> I think we should report an error here, since this will probably still result in a broken state for the user.

Thinking some more about it, I think the only place we can hit this from is `onTabSelect()`, which calls `_restoreTab` when trying to revive a zombified tab. In the three other cases, we always try to access `tabdata.entries` or something like that before calling `_restoreTab`, so we'd fail there before reaching this function here.
If my theory is correct, this would mean that we somehow zombified a tab before it had any chance to build any session data.

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8737271 [details]
MozReview Request: Bug 852267 - Part 2 - Add a null check before restoring history. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43859/diff/1-2/

Comment 29

[Jan Henning [:JanH]]

Comment on attachment 8737271 [details]
MozReview Request: Bug 852267 - Part 2 - Add a null check before restoring history. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43859/diff/2-3/

Comment 30

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17c53b4907de

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5ce9826df5ee
https://hg.mozilla.org/integration/fx-team/rev/c85b2fc35df9

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ce9826df5ee
https://hg.mozilla.org/mozilla-central/rev/c85b2fc35df9