Bug 1065785 - Use session restore to reload crashed tabs
Ok, so now what have I gotten myself into?
So here's the story. Session Restore does this fun thing for when all
your tabs come back (either after a crash, or a restart, etc), where as
much of your previous state as possible is restored. That includes but
is not limited to:
- Back/forward history for each tab
- Filled in form fields
So maybe this ties into
bug 913651
- which is the tab crashed UX bug. ttaubert asks about automatically
restoring closed tabs, and showing a notification bar instead. billm is
concerned that this might result in a drop in submitted crash reports,
since the tab crash page opts in to sending crash reports.
Let's assume for now, however, that the tab crashed page is going to stay the same.
How does Session Store / Session Restore even work?
SessionStore API Docs
.. this is mostly for addons, but might help educate me.
nsISessionStore interface
So what's interesting is that in part, SessionStore is already being
used to restore tabs and stuff… it's just the history that isn't being
restored for some reason. Let me test.
OH! How convenient. I just had a tab crash. And INDEED, it seems as if my history in the tab was not restored. Excellent.
Right, so I'll bet the tab crash page is just flat-out just _not_ using SessionStore here, and that's what's up.
So, where is the tab crash page and how does it work?
browser/base/content/aboutTabCrashed.xhtml
The tab crash page was implemented in
bug 899348
.
Otay, after some dxr-ing around, here's the code in question:
http://hg.mozilla.org/mozilla-central/file/6a8b7740dc53/browser/modules/TabCrashReporter.jsm#l66
Basically, we get the URL that the browser is supposed to be viewing, and do a loadURIWithFlags on it.
So like… a new browser is generated, and we just load the URL in it. So yeah, no bfcache restoration, no form fill out. Nada.
So that's interesting - when a tab closes, we must serialize that
information - the history, etc, - and then when we restore it, it comes
back. And that works for remote tabs, because I'm able to close and
restore a remote tab, and all is well.
So, let's look at Undo Close Tab as a model. How does that even work?
Didn't I work on something like this with Undo Close Tab? OH YEAH,
bug 1059036
- I knew these notes would come in handy.
Hmmm… undoCloseTab seems to have a LIFO stack thing where it has
serialized tab info… so the last tab that was closed is on the top of
the stack. And you pop it off… and it gets restored.
Yeah, that's basically it. You create a brand new tab, and somehow
endow it with everything via restoreTabs in SessionStore.jsm. Ah…
we send some message - SessionStore:restoreHistory
, to tell the DocShell, presumably, about the history being restored. I think form data is in there as well. Nice.
Let's see who handles that message…
Ah, a content script for SessionStore, of course.
And it's calling restoreHistory on
a ContentRestore thing
… ah, and this thing will restore tab history, session cookies, form and scroll data. Excellent.
So it really does look like a ton of the infrastructure is in place
here. I just somehow need to call restoreTabs with the right tabData and
arguments. Piece…of… cake?
So where can we get this tabData from?
I think that will be a big challenge…
Or will it?
15:43
(mconley)
ttaubert: ping
15:43
(ttaubert)
mconley: heya
15:44
(mconley)
so, I've started looking at bug 1065785
15:44
(firebot)
https://bugzil.la/1065785
— ASSIGNED, mconley — Use session restore to reload crashed tabs
15:45
(ttaubert)
mconley: cool
15:45
(mconley)
ttaubert: and I was wondering if:
15:45
(mconley)
ttaubert: a) my assessment in comment 8 is accurate, and b) where and how I can get the latest saved tabData for a crashed tab
15:47
(ttaubert)
so without reading comment 8 and just point (b) it sounds like we might want some sessionstore support for crashed tabs
15:48
(ttaubert)
basically sessionstore listening for the crash notification
15:48
(ttaubert)
freezing tabstate
15:48
(ttaubert)
and preparing the tab for restoration automatically
15:48
(ttaubert)
somehow
15:52
(ttaubert)
great, attaching a patch crashes the content process
15:56
(mconley)
ttaubert: ok, that sounds right. At the time of a tab crash though,
isn't it too late to freeze tab state? Isn't all of that stuff gone?
15:58
(ttaubert)
mconley: some stuff. we'll lose 1s of data at most
15:59
(ttaubert)
shouldn't be too serious
15:59
(mconley)
ttaubert: so every second or so, the content process updates the
parent process with its current form / scroll / content state?
15:59
(ttaubert)
yup
15:59
(mconley)
I see
15:59
(mconley)
ok, excellent
15:59
(ttaubert)
we have a 1s buffer
16:00
(mconley)
very handy
16:00
(ttaubert)
if SS freezes tab state, ss.getTabState() should return what was there before
16:00
(mconley)
ttaubert: where the key is the tab DOM node.
16:00
(mconley)
ttaubert: that's the actual <xul:tab>?
16:01
(ttaubert)
mconley: indeed
16:01
(mconley)
wow
16:01
(mconley)
ok, this is great
16:01
(mconley)
ttaubert: I'm very pleased - all of the pieces seem to be here. :)
16:02
(ttaubert)
we could split the freezing off into a separate bug I think
So suppose my tab crashes. What happens? Well, we have this buffer that has been updated with the tab state…
MessageQueue seems to be the mechanism in content-sessionStore.jsm that sends updates every 1s or so.
So for some tab, if we crash, we have one of two states:
- Common-case - we're crashing, and we've sent updates to the parent in the past.
- Rare-case - we're crashing, and we've not yet sent any updates to the parent.
Let's see if we can take care of #1, and worry about #2 later.
So, we detect a crash in the parent process - what do we do? Well, need
to immediately prevent any updates from the tab crashed page from
reaching the parent - I think this is what ttaubert meant by "freezing".
How will freezing work? We need to stop listening to
SessionStore:update messages. I wonder if the Telemetry is still useful - like, we still listen to
SessionStore:update messages, but we don't update our notion of what the tabs data is.
The notion of what the tab data is is stored in a "TabState" -
see here.
So maybe TabState needs a frozen attribute. Let's start there.
So we freeze TabState for all crashed tabs when we crash. Then we show
the tab crashed page. When we say "try again", the tab that is selected
should be restored immediately, and depending
on browser.sessionstore.restore_on_demand, either restore the tab
state for all of those crashed tabs immediately, OR, prep all of them to
be restored when they are selected. So I think I need to figure out
how browser.sessionstore.restore_on_demand works, because I'm going
to be prepping for that stuff.
How does browser.sessionstore.restore_on_demand work?
Basically, here:
http://hg.mozilla.org/mozilla-central/file/f27ff178807d/browser/components/sessionstore/SessionStore.jsm#l1358
If the browser.sessionstore.restore_on_demand pref is true, we need to
set each browser to be in the TAB_STATE_NEEDS_RESTORE
state… and then unfreeze the data… and then put the unfrozen data
into the new browser.__SS_data… I think?
Ugh, wait - I've misunderstood. TabState is not a class where there's a
single instance per browser or something. It's a global tab state
manager. So putting a "frozen" attribute on that doesn't work.
Ok… so maybe we're talking about adding new freeze and thaw methods to TabStateCache.
Wait - this could be even simpler. Adding freeze and thaw methods might
be overkill - what I'm trying to prevent is messages from an
about:tabcrashed page from being sent up to the parent… why not just
prevent that single case from the about:tabparent page? Like… just don't
send messages up. That way, no need to manage freeze / thaw state. If
we're an about:tabcrashed page, we don't send history. If we aren't, we
do. Simple!
ttaubert seems to like that idea. Excellent!
Hm… so, I filed bug 1070096 to not collect information from
about:tabcrashed, but… it looks like when I kill the plugin-container
process, I get this gasp of life at the end:
Updating parent at about:blank with session information: formdata: null content-sessionStore.js:613
Updating parent at about:blank with session information: pageStyle: null content-sessionStore.js:613
Updating parent at about:blank with session information: history: {
"entries": []
} content-sessionStore.js:613
Updating parent at about:blank with session information: storage: null content-sessionStore.js:613
Updating parent at about:blank with session information: scroll: null content-sessionStore.js:613
Updating parent at about:blank with session information: pageStyle: null content-sessionStore.js:613
Updating parent at about:blank with session information: history: {
"entries": []
} content-sessionStore.js:613
Updating parent at about:blank with session information: storage: null content-sessionStore.js:613
Updating parent at about:blank with session information: scroll: null content-sessionStore.js:613
Updating parent at about:blank with session information: history: {
"entries": []
} content-sessionStore.js:613
Updating parent at http://www.reddit.com/ with session information: pageStyle: null content-sessionStore.js:613
I think that's actually the new browser to contain about:tabcrashed
starting up. That'd explain the about:blank stuff. The reddit.com URI…
that's suspicious. Do we attempt to load that URL, and then stop it, and
then show about:tabcrashed? How does tab crashing work?
When a tab crashes...
When a tab crashes, TabParent hears about it, and does 2 things:
- It sends a global observer notification saying that a browser crashed
- It fires an event from the <xul:browser> saying that it crashed.
tabbrowser.xml hears about the tab crashing, and does the following:
- Grabs the title, uri and icon of the crashed browser
- Updates the "browser remoteness" to false, and loads the about:tabcrashed page
- Has the docshell display a load error for the uri
Meanwhile, TabCrashReporter.jsm heard the notification that TabParent
sent out, and maps the browser that crashed to a crash report.
about:tabcrashed finishes loading fires a custom event
("AboutTabCrashedLoad"), which gBrowser picks up, which calls
TabCrashReporter.onAboutTabCrashedLoad.
TabCrashReporter.onAboutTabCrashedLoad then checks its internal
mapping, looking for the crash report that the notification from
TabParent sent up mapped to the crashing browser. If it finds one, it
exposes the checkbox for submitting a crash report.
BrowserOnClick hears click events in … well, everything… which calls
BrowserOnClick.onAboutTabCrashed… and that determines that we clicked on
the Try Again button, and that sends off the crash report if the
checkbox was checked, and then
calls TabCrashReporter.reloadCrashedTab.
And TabCrashReporter.reloadCrashedTab, as I said earlier in this
document, just reloads the document using the URI for the browser.
So I understand where that about:blank is coming from. What I don't
understand is where the reddit.com came from. Let me see if I can narrow
that down.
Ok… so getting rid of the load of about:tabcrashed, the update
remoteness, and the displayLoadError, gets rid of the messages getting
sent…
Now let's put some back.
Updating parent at about:blank with session information: formdata: null content-sessionStore.js:613
Updating parent at about:blank with session information: pageStyle: null content-sessionStore.js:613
Updating parent at about:blank with session information: history: {
"entries": []
} content-sessionStore.js:613
Updating parent at about:blank with session information: storage: null content-sessionStore.js:613
Updating parent at about:blank with session information: scroll: null content-sessionStore.js:613
Updating parent at about:blank with session information: history: {
"entries": []
} content-sessionStore.js:613
Updating parent at about:blank with session information: pageStyle: null content-sessionStore.js:613
Updating parent at about:blank with session information: history: {
"entries": [
{
"url": "about:blank",
"ID": 3,
"docshellID": 9,
"docIdentifier": 3
}
],
"index": 1
} content-sessionStore.js:613
Updating parent at about:blank with session information: storage: null content-sessionStore.js:613
Updating parent at about:blank with session information: scroll: null
No reddit.com! So updating the remoteness of the browser is what leaks
the about:blank stuff… and I guess displayLoadError is what's
causing the other "pageStyle" message to display.
So…. plan time. What the hell do we do?…
Well, the about:blank stuff… that shouldn't probably be sent up anyhow.
That's really uninteresting stuff if the browser is just starting up.
Can I somehow special-case the very first about:blank? Thinking…
What's happening when we create this browser? It's in a funny state -
it's a brand new, empty browser. It's in a gMultiProcessBrowser true
window, but the browser itself is not remote. It's pointed at
about:blank. It has no history whatsoever. Is that enough to distinguish
it?
SessionHistoryListener seems to be the thing responsible for sending
the empty history up - SessionHistory.isEmpty(docShell) must
evaluate to false… somehow.
How is it failing that? Hrm. It's not. It's showing up later.
So, formData shows up once… pageStyle shows up twice, history shows up 3
times… scroll shows up twice, storage shows up twice… that's everybody.
How do these show up? Time to dump some stacks.
FRAME TREE RESET PHASE
FormDataListener hit onFrameTreeReset:
FormDataListener.onFrameTreeReset@chrome://browser/content/content-sessionStore.js:377:64
FrameTreeInternal.prototype.notifyObservers@resource:///modules/sessionstore/FrameTree.jsm:95:9
FrameTreeInternal.prototype.onStateChange@resource:///modules/sessionstore/FrameTree.jsm:227:7
updateBrowserRemoteness@chrome://browser/content/tabbrowser.xml:1455:13
updateBrowserRemotenessByURL@chrome://browser/content/tabbrowser.xml:1480:20
onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:3397:11
content-sessionStore.js:377
Updating parent at about:blank with session information: formdata: null content-sessionStore.js:618
PageStyleListener hit onFrameTreeReset:
PageStyleListener.onFrameTreeReset@chrome://browser/content/content-sessionStore.js:426:65
FrameTreeInternal.prototype.notifyObservers@resource:///modules/sessionstore/FrameTree.jsm:95:9
FrameTreeInternal.prototype.onStateChange@resource:///modules/sessionstore/FrameTree.jsm:227:7
updateBrowserRemoteness@chrome://browser/content/tabbrowser.xml:1455:13
updateBrowserRemotenessByURL@chrome://browser/content/tabbrowser.xml:1480:20
onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:3397:11
content-sessionStore.js:426
Updating parent at about:blank with session information: pageStyle: null content-sessionStore.js:618
SessionHistoryListener hit collect: SessionHistoryListener.collect@chrome://browser/content/content-sessionStore.js:245:61
SessionHistoryListener.onFrameTreeReset@chrome://browser/content/content-sessionStore.js:256:5
FrameTreeInternal.prototype.notifyObservers@resource:///modules/sessionstore/FrameTree.jsm:95:9
FrameTreeInternal.prototype.onStateChange@resource:///modules/sessionstore/FrameTree.jsm:227:7
updateBrowserRemoteness@chrome://browser/content/tabbrowser.xml:1455:13
updateBrowserRemotenessByURL@chrome://browser/content/tabbrowser.xml:1480:20
onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:3397:11
content-sessionStore.js:245
Updating parent at about:blank with session information: history: {
"entries": []
} content-sessionStore.js:618
SessionStorageListener hit collect: SessionStorageListener.collect@chrome://browser/content/content-sessionStore.js:501:70
SessionStorageListener.onFrameTreeReset@chrome://browser/content/content-sessionStore.js:512:5
FrameTreeInternal.prototype.notifyObservers@resource:///modules/sessionstore/FrameTree.jsm:95:9
FrameTreeInternal.prototype.onStateChange@resource:///modules/sessionstore/FrameTree.jsm:227:7
updateBrowserRemoteness@chrome://browser/content/tabbrowser.xml:1455:13
updateBrowserRemotenessByURL@chrome://browser/content/tabbrowser.xml:1480:20
onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:3397:11
content-sessionStore.js:501
Updating parent at about:blank with session information: storage: null content-sessionStore.js:618
ScrollPositionListener hit onFrameTreeReset:
ScrollPositionListener.onFrameTreeReset@chrome://browser/content/content-sessionStore.js:331:70
FrameTreeInternal.prototype.notifyObservers@resource:///modules/sessionstore/FrameTree.jsm:95:9
FrameTreeInternal.prototype.onStateChange@resource:///modules/sessionstore/FrameTree.jsm:227:7
updateBrowserRemoteness@chrome://browser/content/tabbrowser.xml:1455:13
updateBrowserRemotenessByURL@chrome://browser/content/tabbrowser.xml:1480:20
onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:3397:11
content-sessionStore.js:331
Updating parent at about:blank with session information: scroll: null content-sessionStore.js:618
Updating parent at about:blank with session information: pageStyle: null content-sessionStore.js:618 ???
(Where did this come from?)
FRAME TREE COLLECTED PHASE
SessionHistoryListener hit collect: SessionHistoryListener.collect@chrome://browser/content/content-sessionStore.js:245:61
SessionHistoryListener.onFrameTreeCollected@chrome://browser/content/content-sessionStore.js:252:5
FrameTreeInternal.prototype.notifyObservers@resource:///modules/sessionstore/FrameTree.jsm:95:9
FrameTreeInternal.prototype.onStateChange@resource:///modules/sessionstore/FrameTree.jsm:233:7
onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:3400:11
content-sessionStore.js:245
Updating parent at about:blank with session information: history: {
"entries": []
} content-sessionStore.js:618
SessionStorageListener hit collect: SessionStorageListener.collect@chrome://browser/content/content-sessionStore.js:501:70
SessionStorageListener.onFrameTreeCollected@chrome://browser/content/content-sessionStore.js:508:5
FrameTreeInternal.prototype.notifyObservers@resource:///modules/sessionstore/FrameTree.jsm:95:9
FrameTreeInternal.prototype.onStateChange@resource:///modules/sessionstore/FrameTree.jsm:233:7
onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:3400:11
content-sessionStore.js:501
Updating parent at about:blank with session information: storage: null content-sessionStore.js:618
Updating parent at about:blank with session information: scroll: null content-sessionStore.js:618
(Where did this come from?)
SessionHistoryListener hit collect: SessionHistoryListener.collect@chrome://browser/content/content-sessionStore.js:245:61
SessionHistoryListener.OnHistoryNewEntry@chrome://browser/content/content-sessionStore.js:260:5
content-sessionStore.js:245
Updating parent at about:blank with session information: history: {
"entries": []
} content-sessionStore.js:618
Updating parent at http://www.reddit.com/ with session information: pageStyle: null
So what did that gain me? Well… Great question. Do we need a FrameTree loaded on the initial about:blank?
Right now we create a FrameTree when content-sessionStore.js is loaded,
and that hooks up listeners to nsIWebProgress on initialization.
Let's blue sky this thing… what'd be the ideal scenario? My "desired outcome"…
Tab crashes… notification gets sent, and TabCrashReporter.jsm hears about it if we're doing crash reporting…
Up in the parent process, we see that the tab has crashed. We convert
the browser to a non-remote one… and while that browser loads up, it
realizes that it's in a crashed state… and doesn't send information to
the parent process…
How can a newly created non-remote <xul:browser> know that it's about to go to about:tabcrashed?
There doesn't seem to be a great way… options:
- Strap a special attribute to the <xul:browser> in the event that we're going to tabcrashed, which the content-sessionStore.js can find out about, and know not to send any communications…
- Do the thawing and freezing on the parent side, though this adds a bunch of state that SessionStore now has to manage
- Somehow, without an attribute, communicate to content-sessionStore.js that the about:blank tab we're loading is not to have any history… maybe through some XPCOM interface thing. Again, more state.
Well, we've really created a sort of a temporary browser thing that
we're hoping to destroy as soon as we can - like, what's eventually
going to happen if we restore, is we're going to be destroying this
one-off non-remote browser and replacing it entirely with a remote
browser, and pouring all of our old cached state into it.
So… somehow mark this one-off browser as special. And have SessionStore ignore it right away.
I think… I think that might work. Ok, let me think… mark browser, with
like an attribute on the xul:browser, as a browser that will not have
its session information recorded when received… to get the timing right,
that'll probably involve setting that attribute on the
<xul:browser> before it is re-appended to the document in
updateBrowserRemoteness.
Ok, I've written a patch that kinda does that, and stuck it in
bug 1070096
. Let's see what ttaubert thinks.
Here's what he responded with:
"
I like the overall approach. Wouldn't it be better if we listened for
"oop-browser-crashed" in onTabAdd()? SessionStore could then maintain its own
WeakSet of crashed browsers. We would then remove the browser from the set when
the tab is revived or overriden. Navigating away manually should also do that."
Hm…
What does that look like? What is onTabAdd? It's a "TabOpen" event
listener, and, as expected, is for setting up things on new tabs.
One of the things it already does is adds event listeners for
SwapDocShells, so I suppose I can add one for oop-browser-crashed too.
So that's easy - we can start ignoring the crashed tabs by adding them to a WeakSet or something. But what about reviving them?
There are two cases to handle:
- The tab is revived by the user clicking on "Try again" to reload the tab, causing it to go remote and start browsing.
- The user types into the URL bar causing it to go remote again and start browsing
I need a choke point where those two things intersect in a sensical way.
My first instinct is the updateRemotenessByURL stuff - but there are
other cases that use that method. For example, browsing to or from our
about: blacklisted pages.
The other thing that comes to mind is the actual about:tabcrashed page
itself. That, I think, might be the more correct route. Suppose we
detect when the browser that it belongs to is being unloaded… and when
that happens, we send a message to the parent saying, "Yo, I'm
unloaded", which then communicates with SessionStore.jsm saying that the
tab has been revived.
Let me give that a shot.
Ok, that seems to work. I've posted the patch to bug 1070096 to see
what ttaubert thinks. In the meantime… how am I going to test that
patch?
Suppose… suppose I open a new e10s window, and in the selected tab,
send a framescript that causes a crash. Maybe use jsctypes, and cause a
segfault. The Nightly Tester Tools do something that I can probably
copy. Ok, so the content process crashes, and what do I want to do? I
want to revive the tab and then make sure that SessionStore never got
any update traffic while the browser was crashed.
So SessionStore allows me to retrieve state for a browser. That's what
I'll do - I'll browse to a page A, crash the tab, revive the tab to
page B, and then retrieve the browser state, and make sure
about:tabcrashed is never in there.
From ttaubert in bug 1070096:
(In reply to Tim Taubert [:ttaubert] from comment #10)
> Comment on attachment 8493352
> Collect no SessionStore information for about:tabcrashed pages. r=?
>
> Review of attachment 8493352:
> -----------------------------------------------------------------
>
> ::: browser/base/content/aboutTabCrashed.js
> @@ +14,5 @@
> >
> > +addEventListener("unload", () => {
> > + let event = new CustomEvent("AboutTabCrashedUnload", {bubbles: true});
> > + document.dispatchEvent(event);
> > +});
>
> It seems rather weird that about:tabcrashed has to help sessionstore here.
> How about adding a pagehide listener to content-sessionStore.js that if (url
> == about:tabcrashed) sends a message to the parent saying it should
> revive/unignore the crashed browser? Would that work?
>
> ::: browser/components/sessionstore/SessionStore.jsm
> @@ +1421,5 @@
> > // events due to changing groups in Panorama.
> > this.saveStateDelayed(aWindow);
> > },
> >
> > + onRemoteTabCrashed: function ssi_onRemoteTabCrashed(aBrowser) {
>
> Nit:
>
> onRemoteTabCrashed: function (aBrowser) {
>
> or fancier:
>
> onRemoteTabCrashed(aBrowser) {
>
> :)
> Comment on attachment 8493352
> Collect no SessionStore information for about:tabcrashed pages. r=?
>
> Review of attachment 8493352:
> -----------------------------------------------------------------
>
> ::: browser/base/content/aboutTabCrashed.js
> @@ +14,5 @@
> >
> > +addEventListener("unload", () => {
> > + let event = new CustomEvent("AboutTabCrashedUnload", {bubbles: true});
> > + document.dispatchEvent(event);
> > +});
>
> It seems rather weird that about:tabcrashed has to help sessionstore here.
> How about adding a pagehide listener to content-sessionStore.js that if (url
> == about:tabcrashed) sends a message to the parent saying it should
> revive/unignore the crashed browser? Would that work?
>
> ::: browser/components/sessionstore/SessionStore.jsm
> @@ +1421,5 @@
> > // events due to changing groups in Panorama.
> > this.saveStateDelayed(aWindow);
> > },
> >
> > + onRemoteTabCrashed: function ssi_onRemoteTabCrashed(aBrowser) {
>
> Nit:
>
> onRemoteTabCrashed: function (aBrowser) {
>
> or fancier:
>
> onRemoteTabCrashed(aBrowser) {
>
> :)
ttaubert's suggestion about moving the logic into
content-sessionStore.js sounds like a fine one, and that keeps things
nice and encapsulated within the scope of /sessionstore/.
And then I get to use fancy new JS ES6 function syntax sugar. :D
Sounds like we're really close to resolving bug 1070096 - I'll work on
those corrections, and then I also want to get that test written.
Garrrr - so, one fly in the ointment (there's always at least one): I
can't just compare the document URI against about:tabcrashed, because
the content still thinks it is pointed at the original crashed page! I
need to understand how
displayLoadError works.
Hrm. So now I've just put the pagehide event listener in
SessionStore.jsm when a tab crashes, and then check the URL there. That
seems to work. Let's see if ttaubert is cool with the handling being in
SessionStore.jsm over content-sessionStore.js.
Ok, ttaubert has some more feedback… hm, he's found multiple issues. I'm going to try to boil them down:
Yes, let's use a message from content-sessionStore.js. He has an idea that should work properly.
Add a test for navigating away from about:tabcrashed to a blacklisted and non-blacklisted URL
Move the guard against listening to crashed browsers so that we just ignore updates from crashed browsers.
Store browser permanentKey's in the WeakSet instead, as this allows us
to do crazy things like swap docshells and keep the right notion of who
is who.
File a follow-up for ttaubert's comment 16 in bug 1070096.
Let's get started. I'd like to confirm that first bit about getting a message sent form content-sessionStore.js.
Hm, still no dice. I'm getting about:blank as the location of these pages.
Mossop has an idea - I can't use frame message manager from "unload"
handlers, but I can use the child process message manager.
Ok, this is kinda crazy, but here's my solution - I use the
child-process message manager (even though we're in the parent process,
thanks to
bug 680413
), and send a message up to the "parent" saying that we're unloading
about:tabcrashed, so revive us. But because the receiver of the message
doesn't have the browser set as the message target, I'll pass the
docShell's chromeEventHandler along in the message "objects" argument.
This maps directly to the <xul:browser> in the same-process case.
I'll want to put some sanity checks in here to make sure we're actually
in the parent process, and we didn't do something nuts like load
about:tabcrashed in a subframe of a remote browser.
This seems to work. Amazingly. Let's see how ttaubert feels about it, once I get all of these other things addressed.
While I'm waiting for a review, let's write a test for this thing.
Let's look at some prior art and head.js to see what tools I have to
test SessionStore… looking in browser/components/sessionstore/test..
Good - lots of modern, "add_task" style tests in here. That's a good sign.
UGH - I just lost an hour because of this:
mm.loadFrameScript("data,(" + frame_script.toString() + ")();", false);
That's supposed to load a framescript, but won't, because I forgot the
colon after data. loadFrameScript was failing silently after I passed
it that invalid URI. GARRLRKJRLKJDS:LKRJDLS:KRJ. I filed
this bug
about throwing an error if we pass an invalid URI.
Yay! I can crash a tab. I can't use CrashTestUtils.jsm because that seems to just be for xpcshell tests - but I was able to
adapt some code from here.
Ok, tests posted.
ALRIGHT, once and for all, _how_ come these tests don't pass unless I
run with --e10s? What in sessionstore is paying attention to
browser.tabs.remote.*? Hm… no direct reads of the browser.tabs.remote
prefs, nor of the gMultiProcessBrowser stuff… Nor the LoadContext's
UseRemoteTabs.
Interesting.
Let's pretend that I get bug 1070096 landed. What's next? Based on the direction of
bug 913651
, we want SessionStore to be able to revive an individual tab, or all crashed tabs.
So how about this - user clicks on "Revive this tab" or whatever.
Click event bubbles up…SessionStore either hears about it, or gets told
about that event, and then…well, reacts accordingly. Add a
SessionStore.reviveTabs that can take an array of one or more tabs, and
just frickin' do it.
And by "frickin' do it", I mean:
- Blast the history and state down to the tab.
- If selected tab, kick off restoring the state from cache / network (thus reviving tab)
- If not selected tab and we're set to load content on selection, just prep each revived tab to restore on selection
- If not selected tab and we're set to load content on restoration, just do the full restoration.
And that's it, really.
Remove sleep
Add crash handler setup and cleanup stuff to account for these being expected crashes
Change the skip-if stuff
Remove the pagehide event handler if handleRevivedTab
So with bug 1070096 almost out the door, and the plan above, what are my next concrete steps?
Let's start by just reviving a single tab for now, with an eye on
allowing the revival of all tabs. That means we don't have to change the
UI for about:tabcrashed, which is good because it sounds like bug
913651 isn't solid just yet.
So let's refresh my memory - some things have changed. What exactly does TabCrashReporter currently do?
Ah, so it mostly does just crash report things - the only thing I
probably want to move out and into SessionStore is the
"reloadCrashedTab" business. I think what we should do is add:
SessionStore.reviveTabs(aBrowsers)
(Yeah, I know - maybe that should be reviveBrowsers or aTabs… dunno just yet. Will bikeshed on names and parameters later)
Hey! That was pretty easy! I added a reviveCrashedBrowsers method to
SessionStore, and we just get the state from TabState for the tab, and
call restoreTab on the associated tab with the state! Wow!!
So, if bug 913651 has its way, how are we going to reload all crashed
tabs? The WeakSet we added in bug 1070096 is non-iterable… maybe we
should just make that an iterable Set?
While I'm waiting for feedback, what are some good tests for this?
- Crash a tab, use SessionStore to revive it, browse it to a new page, ensure that the new page is added to history.
- Crash multiple tabs, use SessionStore to revive all. Browse all to new pages, ensure that new pages are added to their history.
- Crash multiple tabs, use SessionStore to revive just one, browse to a new page, ensure that new pages are added to its history.
Make content-sessionStore.jsm not send up updates if we're at about:tabcrashed
See what felipe did with the "Try again" button, because I think that behaves all differently now
Make SessionStore.jsm care about tabs crashing
Talk with mmaslaney about tab crashed UX - specifically, what happens
if we crash right after restoration? When single-process Fx crashes
multiple times, we show the "Well this is embarassing" page and allow
the user to select which items they restore.
OH FFS another memory leak. :(((
First, confirming that it's the not-remembering-tab-crashed bit:
https://tbpl.mozilla.org/?tree=Try&rev=becef983acb8
Nice that I can use -u mochitest-e10s-browser-chrome-1 and not build bc2 or bc3. Nice to save some machine time.
So, while that's building… hypotheses…
Hypothesis 1: We're not removing the PPMM message listener at the right time.
Hm… no, I don't think so. We're removing it on uninit. Still, I could test it.
Hypothesis 2: We're not removing the FMM message listeners at the right time.
Also doubtful - we remove them the way we always used to. This seems highly unlikely.
Hypothesis 3: We're not removing the oop-browser-crashed event handler at the right time.
Doubtful… we remove it at the same time we remove the SwapDocShells event listener. This seems highly unlikely as well.
Hypothesis 4: We need to remove the pagehide event handler in content-sessionStore.js after handleRevivedTab fires.
…maybe? This is probably my best lead. Here goes:
https://tbpl.mozilla.org/?tree=Try&rev=cad4b86f268c
GARRRRRRRR - mochitest-e10s-browser-chrome-1 doesn't work. :(((
Hypothesis 4 seems to have been correct! Re-landed.