Closed Bug 1065785 Opened 10 years ago Closed 10 years ago

[e10s] Use session restore to reload crashed tabs

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 36
Tracking Flags:
Tracking Status
e10s m3+ ---

People

(Reporter: billm, Assigned: mconley)

References

Details

Attachments

(3 files, 4 obsolete files)

tab-crasher.xpi
18.53 KB, application/x-xpinstall
Details
[e10s] Use SessionStore to reload crashed tabs. r=?
11.30 KB, patch
smacleod
: review+
Details | Diff | Splinter Review
Bugnotes
17.24 KB, application/zip
Details 
Right now e10s offers a worse crash experience in some ways that without e10s. It won't fill in form fields for crashed tabs and it also won't save history.
That sounds like a good idea, did we think about automatically restoring closed tabs? We could show a notification bar at the top that says we automatically restored the tab because it crashed.
Assignee: nobody → mconley
tracking-e10s: ? → m3+
Status: NEW → ASSIGNED
(In reply to Tim Taubert [:ttaubert] from comment #1)
> That sounds like a good idea, did we think about automatically restoring
> closed tabs? We could show a notification bar at the top that says we
> automatically restored the tab because it crashed.

Good question. I'll sync up with UX to see how we were planning on dealing with crashed tabs.
The reason we did it this way was to give users an option to send a crash report.
An option in a notification bar to send a crash report would fulfill the same purpose, and might be less disruptive. *shrug*, I'll see what phlsa and co have in mind.
We just need to make sure that "do nothing" doesn't mean "don't send a crash report". Otherwise we'll get a lot fewer crash reports.
Yeah, agreed.
Anywho, while I wait on that information, I'll carry on as if we're going to keep our current crashed tab page.
So a quick analysis here, it looks like SessionStoreInternal.restoreTabs will do exactly what we want here, assuming I can get the right tabData out of the cache.

I think this will require me to extend nsISessionStore to accommodate the restoration of one or more crashed tabs. The rest of SessionStore looks 100% already set up to do the rest of the work. We can, for example, already undo-close a remote tab and get all of the back/forward cache, form data, and scroll position all restored.

TL;DR: I think most of the hard stuff is already done, and now we just need to expose some methods on nsISessionStore for TabCrashReporter to restore crashed tabs with.
Another benefit of this bug is that we'll be able to reload the tabs on-demand. People have been asking for that.
Ok, so I spent the afternoon catching up on how SessionStore works. I think I have a rough idea how this can work. Here's my game plan here:

1) Add a new "frozen" attribute to TabState. If a TabState has frozen set to true, calls to update will be ignored.
2) Have our TabCrash handling code immediately freeze the TabState of each crashed tab - that way, we don't accidentally overwrite our tab state with stuff from the crashed tab page.
3) Make it so that when the user clicks "Try again" to restore the saved tab, we restore the selected tab with the frozen TabState, and then unfreeze the state.
4) For the other crashed tabs across each window, we should do the following:
  i) If browser.sessionstore.restore_on_demand is set to true, prep each new browser to restore as soon as the tab is selected
  ii) If browser.sessionstore.restore_on_demand is set to false, immediately restore each crashed tab with the frozen TabStates, and then unfreeze the state.

Currently TabCrashReporter does the reloading of crashed tabs, but I suggest we extend SessionStore to do most if not all of the above work instead, as it seems to fall under its domain.

Does this sound sane, ttaubert?
Flags: needinfo?(ttaubert)
And now I've just realized that TabState might not be the right place to put a "frozen" attribute. I originally thought there was a single TabState per browser, but it appears to be a global TabState management thing... so, back to the drawing board, slightly.
Sounds reasonable. You could probably add a WeakMap in TabState that records whether a tab is frozen. That would cause us to return the cached data unconditionally.

Also, you should be able to use SessionStoreInternal.restoreTabs as a way to restore frozen tabs. You could write a function that would iterate over all frozen windows. For each one, it would call restoreTabs on all its frozen tabs.

One issue we might want to consider is what happens if the user quits before hitting "Try again". Should we restore everything at startup as if there were no crash? I think that's the behavior we'll get with the freezing, which seems good.
(In reply to Bill McCloskey (:billm) from comment #12)
> One issue we might want to consider is what happens if the user quits before
> hitting "Try again". Should we restore everything at startup as if there
> were no crash? I think that's the behavior we'll get with the freezing,
> which seems good.

Yes, I think we should do that.

(In reply to Bill McCloskey (:billm) from comment #12)
> Sounds reasonable. You could probably add a WeakMap in TabState that records
> whether a tab is frozen. That would cause us to return the cached data
> unconditionally.

I wonder if we actually need that? As the content process died and we receive no more updates from the frame script that old tab state should just be "frozen", right? The only thing _collectBaseTabData() returns that could have changed is probably the tab's favicon. But I don't remember - the favicon stays the same when tab crashes, right?

So maybe that should just work.

(In reply to Bill McCloskey (:billm) from comment #12)
> Also, you should be able to use SessionStoreInternal.restoreTabs as a way to
> restore frozen tabs. You could write a function that would iterate over all
> frozen windows. For each one, it would call restoreTabs on all its frozen
> tabs.

If .getTabState() works you could use .setTabState() here. IIRC that should also support deferred restore for unpinned tabs other than the selected one.
Flags: needinfo?(ttaubert)
BTW, related: currently the TabCrashReporter isn't available when building without the crash reporter. That means I do not want to report crashes from custom builds but I might still want to be able to restore crashed tabs. Now we could always define |TabCrashReporter| in browser.js to make it work but shouldn't we maybe split that in two? The restoration and the reporting part?
(In reply to Tim Taubert [:ttaubert] from comment #14)
> BTW, related: currently the TabCrashReporter isn't available when building
> without the crash reporter. That means I do not want to report crashes from
> custom builds but I might still want to be able to restore crashed tabs. Now
> we could always define |TabCrashReporter| in browser.js to make it work but
> shouldn't we maybe split that in two? The restoration and the reporting part?

I'd say yes, the two should be separated. I ran into this as well as I had crash reporting disabled in my configure file and saw zero actions from clicking on try again. Not that that's a common configuration, but regardless, splitting them seems like the right approach.
Blocks: e10s
Summary: Use session restore to reload crashed tabs → [e10s] Use session restore to reload crashed tabs
(In reply to Tim Taubert [:ttaubert] from comment #13)
> 
> I wonder if we actually need that? As the content process died and we
> receive no more updates from the frame script that old tab state should just
> be "frozen", right? The only thing _collectBaseTabData() returns that could
> have changed is probably the tab's favicon. But I don't remember - the
> favicon stays the same when tab crashes, right?
> 
> So maybe that should just work.

My concern is that because the frame script is loaded in every browser (remote or otherwise), that the crashed tab browser (which can still send messages to the parent even though they're in the same process) might overwrite useful information that is not relevant to the content that was loaded in there.

For example, suppose we supply a text input in the tab crashed page that allows a user to type in what they were doing at the time of the crash. That form field, as far as I can tell, is still going to get sync'd with SessionStore, and get mapped to the same tab.

So that's what I'm hoping to avoid when we "freeze" a tab. Although, now that I think about it, perhaps the simpler solution would just be to not send any tab state communications up to the parent if we're at about:tabcrashed...
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17)
> For example, suppose we supply a text input in the tab crashed page that
> allows a user to type in what they were doing at the time of the crash. That
> form field, as far as I can tell, is still going to get sync'd with
> SessionStore, and get mapped to the same tab.

Right, I somehow forgot that about:tabcrashed is a page just like every other page and would have the smae script as well.

> So that's what I'm hoping to avoid when we "freeze" a tab. Although, now
> that I think about it, perhaps the simpler solution would just be to not
> send any tab state communications up to the parent if we're at
> about:tabcrashed...

That sounds indeed like a good idea.
Depends on: 1070096
OS: Linux → All
Hardware: x86_64 → All
Attached file tab-crasher.xpi — 
A simple add-on to crash the selected remote tab.
Attachment #8499056 - Attachment is obsolete: true
Comment on attachment 8499090 [details] [diff] [review]
[e10s] WIP - Use SessionStore to reload crashed tabs. r=?

This was a heck of a lot easier than I thought it'd be.

This moves the responsibility of reloading crashed tabs to SessionStore.jsm, and just blasts the last recorded state from TabState for that tab into the newly revived browser.

What do you think of this, Tim?
Attachment #8499090 - Flags: feedback?(ttaubert)
Comment on attachment 8499090 [details] [diff] [review]
[e10s] WIP - Use SessionStore to reload crashed tabs. r=?

I'm also interested in smacleod's feedback on this approach.

A few concerns:

1) Bug 913651 is started to look like we're going to want to be able to revive _all crashed tabs_. We used to do that by going through all tabs and assuming if the location of the browser was at about:tabcrashed, that it needed reloading (it has since changed so that we only reload the tab you're currently on).

I'm curious on opinions on the best way to make that work. We do have the WeakSet of crashed browsers that got added in bug 1070096, but that's not iterable.

Is it worth changing that WeakSet to an iterable Set, and ensuring that we remove entries from that Set once tabs close?

2) My terminology is getting a bit muddled - restore, revive, reload... I've been going with "revive", and I'm not sure it matters. Similarly, I seem to be using "browser" and "tab" interchangeably (example - we reviveCrashedBrowsers, which then causes a SessionStore:RemoteTabRevived message to be fired). Not sure how you want to go on that. Naming, right?
Attachment #8499090 - Flags: feedback?(smacleod)
Comment on attachment 8499090 [details] [diff] [review]
[e10s] WIP - Use SessionStore to reload crashed tabs. r=?

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

::: browser/components/sessionstore/SessionStore.jsm
@@ +1973,5 @@
> +        continue;
> +      }
> +
> +      // Sanity check - the browser to be revived should not be remote
> +      // at this point.

Isn't remoteness changed in restoreTab, which happens after this? What am I missing?
Comment on attachment 8499090 [details] [diff] [review]
[e10s] WIP - Use SessionStore to reload crashed tabs. r=?

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

This looks good to me.

::: browser/components/sessionstore/SessionStore.jsm
@@ +1973,5 @@
> +        continue;
> +      }
> +
> +      // Sanity check - the browser to be revived should not be remote
> +      // at this point.

Ah, totally misread and thought you were making sure it *was* remote before restoring.
Attachment #8499090 - Flags: feedback?(smacleod) → feedback+
Thanks for the feedback, smacleod! Did you have any input on my questions in comment 23?
Flags: needinfo?(smacleod)
Even though we discussed this in person, I thought I'd dump what was said in here too.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #23)
> 1) Bug 913651 is started to look like we're going to want to be able to
> revive _all crashed tabs_. We used to do that by going through all tabs and
> assuming if the location of the browser was at about:tabcrashed, that it
> needed reloading (it has since changed so that we only reload the tab you're
> currently on).
> 
> I'm curious on opinions on the best way to make that work. We do have the
> WeakSet of crashed browsers that got added in bug 1070096, but that's not
> iterable.
> 
> Is it worth changing that WeakSet to an iterable Set, and ensuring that we
> remove entries from that Set once tabs close?

Changing to an iterable Set sounds fine to me, we should be able to make sure we remove the entries.


> 2) My terminology is getting a bit muddled - restore, revive, reload... I've
> been going with "revive", and I'm not sure it matters. Similarly, I seem to
> be using "browser" and "tab" interchangeably (example - we
> reviveCrashedBrowsers, which then causes a SessionStore:RemoteTabRevived
> message to be fired). Not sure how you want to go on that. Naming, right?

I like 'revive' personally, but if Tim feels strongly about something else I'm not opposed to it. As for browser vs tab, throughout SessionStore we generally have the APIs refer to a Tab (e.g. setTabValue, restoreTab, TabStateCache), so I'd recommend keeping with that convention where it makes sense.
Flags: needinfo?(smacleod)
Attachment #8499090 - Attachment is obsolete: true
Attachment #8499090 - Flags: feedback?(ttaubert)
Comment on attachment 8503210 [details] [diff] [review]
[e10s] Use SessionStore to reload crashed tabs. r=?

Ok, how about this? (targeting review request at smacleod because ttaubert's queue appears to be quite full and I don't want to overlaod him - smacleod, lemme know if you want me to redirect parts or all of this).

I made it so that, at least for now, reviveCrashedTab just takes a single browser. Our tab crashed page only allows us to revive a single tab anyways - I figure we can do the strong Set and reviving all tabs later, when the new tab crashed page specced out in bug 913651 gets implemented.

I also added a test. The test just checks that we get back to the page we were on after reviving a crashed tab with reviveCrashedTab, and checks the history length. Let me know if you think it needs to be more comprehensive than that.

A note, if you want to test this patch or run the tests - you'll want to apply this patch on top of the patches from these bugs in this order:

Bug 1075658
Both patches from Bug 1070096
THEN this patch.
Attachment #8503210 - Flags: review?(smacleod)
Comment on attachment 8503263 [details] [diff] [review]
[e10s] Use SessionStore to reload crashed tabs. r=?

Rebased on tip.
Attachment #8503263 - Flags: review?(smacleod)
Attachment #8503210 - Attachment is obsolete: true
Attachment #8503210 - Flags: review?(smacleod)
Comment on attachment 8503263 [details] [diff] [review]
[e10s] Use SessionStore to reload crashed tabs. r=?

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

Looks good to me, just one issue.

> I also added a test. The test just checks that we get back to the page we
> were on after reviving a crashed tab with reviveCrashedTab, and checks the
> history length. Let me know if you think it needs to be more comprehensive
> than that.

I think that should be fine. If we're restoring the history we should be properly
restoring the rest since we're just using restore functionality tested elsewhere.

::: browser/components/sessionstore/SessionStore.jsm
@@ +1962,5 @@
> +   *        A crashed <xul:browser>. This is a no-op if the browser hasn't
> +   *        actually crashed, or is not associated with a tab. This function
> +   *        will also throw if the browser happens to be remote.
> +   */
> +  reviveCrashedTab(aBrowser) {

All of our other APIs actually take the tab itself, rather than the associated browser. I think we should probably be consistent here and switch this to take a tab.
Attachment #8503263 - Flags: review?(smacleod) → feedback+
Attachment #8503263 - Attachment is obsolete: true
Comment on attachment 8505058 [details] [diff] [review]
[e10s] Use SessionStore to reload crashed tabs. r=?

Switched to making reviveCrashedTab take a tab parameter instead of a browser parameter.
Attachment #8505058 - Flags: review?(smacleod)
Comment on attachment 8505058 [details] [diff] [review]
[e10s] Use SessionStore to reload crashed tabs. r=?

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

LGTM!
Attachment #8505058 - Flags: review?(smacleod) → review+
Landed in fx-team without the test:

https://hg.mozilla.org/integration/fx-team/rev/8477a0eca4fa

I'll file a new bug to land the test once it's unblocked by bug 1075658.
Whiteboard: [fixed-in-fx-team]
Re-landed, because I solved the leak in bug 1070096:

https://hg.mozilla.org/integration/fx-team/rev/4309ac87d551
Flags: needinfo?(mconley)
https://hg.mozilla.org/mozilla-central/rev/4309ac87d551
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #19)
> Created attachment 8499038 [details]
> tab-crasher.xpi
> 
> A simple add-on to crash the selected remote tab.

FYI, I recently (re-)added support for crashing the content process to crashme:
people.mozilla.org/~tmielczarek/crashme/

It's available as a button in the customize dialog.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: