Closed
Bug 1065785
Opened 10 years ago
Closed 10 years ago
[e10s] Use session restore to reload crashed tabs
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 36
Tracking | Status | |
---|---|---|
e10s | m3+ | --- |
People
(Reporter: billm, Assigned: mconley)
References
Details
Attachments
(3 files, 4 obsolete files)
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.
Comment 1•10 years ago
|
||
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.
Updated•10 years ago
|
Assignee: nobody → mconley
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Reporter | ||
Comment 3•10 years ago
|
||
The reason we did it this way was to give users an option to send a crash report.
Assignee | ||
Comment 4•10 years ago
|
||
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.
Reporter | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
Yeah, agreed.
Assignee | ||
Comment 7•10 years ago
|
||
Anywho, while I wait on that information, I'll carry on as if we're going to keep our current crashed tab page.
Assignee | ||
Comment 8•10 years ago
|
||
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.
Reporter | ||
Comment 9•10 years ago
|
||
Another benefit of this bug is that we'll be able to reload the tabs on-demand. People have been asking for that.
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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.
Reporter | ||
Comment 12•10 years ago
|
||
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.
Comment 13•10 years ago
|
||
(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)
Comment 14•10 years ago
|
||
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?
Comment 15•10 years ago
|
||
(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.
Updated•10 years ago
|
Blocks: e10s
Summary: Use session restore to reload crashed tabs → [e10s] Use session restore to reload crashed tabs
Assignee | ||
Comment 17•10 years ago
|
||
(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...
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 19•10 years ago
|
||
A simple add-on to crash the selected remote tab.
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8499056 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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 25•10 years ago
|
||
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+
Assignee | ||
Comment 26•10 years ago
|
||
Thanks for the feedback, smacleod! Did you have any input on my questions in comment 23?
Flags: needinfo?(smacleod)
Assignee | ||
Comment 27•10 years ago
|
||
Comment 28•10 years ago
|
||
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)
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8499090 -
Attachment is obsolete: true
Attachment #8499090 -
Flags: feedback?(ttaubert)
Assignee | ||
Comment 30•10 years ago
|
||
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)
Assignee | ||
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
Comment on attachment 8503263 [details] [diff] [review] [e10s] Use SessionStore to reload crashed tabs. r=? Rebased on tip.
Attachment #8503263 -
Flags: review?(smacleod)
Assignee | ||
Updated•10 years ago
|
Attachment #8503210 -
Attachment is obsolete: true
Attachment #8503210 -
Flags: review?(smacleod)
Comment 33•10 years ago
|
||
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+
Assignee | ||
Comment 34•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8503263 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
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 36•10 years ago
|
||
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+
Assignee | ||
Comment 37•10 years ago
|
||
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]
Backed out in https://hg.mozilla.org/integration/fx-team/rev/bdaf01be9e90 because either this patch or bug 1070096 introduced a browser-chrome leak: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=956143&repo=fx-team
Flags: needinfo?(mconley)
Assignee | ||
Comment 39•10 years ago
|
||
Re-landed, because I solved the leak in bug 1070096: https://hg.mozilla.org/integration/fx-team/rev/4309ac87d551
Flags: needinfo?(mconley)
Comment 40•10 years ago
|
||
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
Assignee | ||
Comment 41•10 years ago
|
||
bugnotes |
http://people.mozilla.org/~mconley2/bugnotes/bug-1065785.html
Comment 42•9 years ago
|
||
(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.
Description
•