1209689 - Crashed tab indicates all tabs have crashed and every tab loads the crashed tab page

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) open three tabs to content
2) crash the content process

result: the visible tab displays the crashed tab page and the background tabs have the crashed tab indicator on them.

3) select a background tab

result: another crashed tab ui

I'd like to propose we change this such that:

1) we display the crashed tab page in a new tab that is brought to the foreground.
2) we do not display a crashed tab indicator.
3) selecting a background tab while viewing the crashed tab ui launches a new content process and loads its content.

Comment 1

[Jim Mathies [:jimm]]

Attachment: crashed tabs

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I think this is the right direction, but how about this slight adjustment:

1) Crashed tab page is shown in currently visible tab (and all currently selected tabs in background windows)
2) Crashed tab indicator is only shown on the foreground tabs
3) selecting a background tab while viewing the crashed tab ui launches a new content process and loads its content.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #2)
> I think this is the right direction, but how about this slight adjustment:
> 
> 1) Crashed tab page is shown in currently visible tab (and all currently
> selected tabs in background windows)

This needs to happen since we are currently dealing with one content process - we can't paint the old content in tabs for other windows anyway. But this only applies to windows that are visible on the desktop.

We need to avoid loading the tabcrashed about page in windows that are minimized since some users have 1000's of windows. For them a crashed tab can cause the browser to hang for minutes while the browser loads 1000s of about:crashedtab pages. Avoiding the load in minimized/hidden windows solves this.

Related question: we have different crashtab uis, one of which has the crash report submit form in it. I'm thinking we display the submit crash report ui only in one tab, all other tabs in visible windows would not see the submit ui. (This is really pretty corner case since most users only use one window.)

> 2) Crashed tab indicator is only shown on the foreground tabs

Can we limit this icon to the tab that has the crash submit ui? I don't see the point of displaying this on a foreground tab in a background window. It's a bit confusing and makes the browser look more broken than it is.

> 3) selecting a background tab while viewing the crashed tab ui launches a
> new content process and loads its content.

Great. 

With these changes, can we get rid of the "Restore All Crashed Tabs" button in the crash ui? If we auto-reload when brought to foreground, it serves no purpose. This would also allow us to set the "Restore this tab" button as the default.

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Jim Mathies [:jimm] from comment #3)
> (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> comment #2)
> > I think this is the right direction, but how about this slight adjustment:
> > 
> > 1) Crashed tab page is shown in currently visible tab (and all currently
> > selected tabs in background windows)
> 
> This needs to happen since we are currently dealing with one content process
> - we can't paint the old content in tabs for other windows anyway. But this
> only applies to windows that are visible on the desktop.
> 
> We need to avoid loading the tabcrashed about page in windows that are
> minimized since some users have 1000's of windows. For them a crashed tab
> can cause the browser to hang for minutes while the browser loads 1000s of
> about:crashedtab pages. Avoiding the load in minimized/hidden windows solves
> this.
> 
> Related question: we have different crashtab uis, one of which has the crash
> report submit form in it. I'm thinking we display the submit crash report ui
> only in one tab, all other tabs in visible windows would not see the submit
> ui. (This is really pretty corner case since most users only use one window.)

This all sounds good to me :)

> > 2) Crashed tab indicator is only shown on the foreground tabs
> 
> Can we limit this icon to the tab that has the crash submit ui? I don't see
> the point of displaying this on a foreground tab in a background window.
> It's a bit confusing and makes the browser look more broken than it is.

The reasoning here was to show the indicator on all tabs that show the crashed tab UI, mostly for the sake of consistency. But since the crash UI will look different anyway, I'm OK with just showing the indicator on the tab that is showing the submit crash UI.

> > 3) selecting a background tab while viewing the crashed tab ui launches a
> > new content process and loads its content.
> 
> Great. 
> 
> With these changes, can we get rid of the "Restore All Crashed Tabs" button
> in the crash ui? If we auto-reload when brought to foreground, it serves no
> purpose. This would also allow us to set the "Restore this tab" button as
> the default.

Sounds good!

Comment 5

[Jim Mathies [:jimm]]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #4)
> (In reply to Jim Mathies [:jimm] from comment #3)
> > (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> > comment #2)
> > > I think this is the right direction, but how about this slight adjustment:
> > > 
> > > 1) Crashed tab page is shown in currently visible tab (and all currently
> > > selected tabs in background windows)
> > 
> > This needs to happen since we are currently dealing with one content process
> > - we can't paint the old content in tabs for other windows anyway. But this
> > only applies to windows that are visible on the desktop.
> > 
> > We need to avoid loading the tabcrashed about page in windows that are
> > minimized since some users have 1000's of windows. For them a crashed tab
> > can cause the browser to hang for minutes while the browser loads 1000s of
> > about:crashedtab pages. Avoiding the load in minimized/hidden windows solves
> > this.
> > 
> > Related question: we have different crashtab uis, one of which has the crash
> > report submit form in it. I'm thinking we display the submit crash report ui
> > only in one tab, all other tabs in visible windows would not see the submit
> > ui. (This is really pretty corner case since most users only use one window.)
> 
> This all sounds good to me :)
> 
> > > 2) Crashed tab indicator is only shown on the foreground tabs
> > 
> > Can we limit this icon to the tab that has the crash submit ui? I don't see
> > the point of displaying this on a foreground tab in a background window.
> > It's a bit confusing and makes the browser look more broken than it is.
> 
> The reasoning here was to show the indicator on all tabs that show the
> crashed tab UI, mostly for the sake of consistency. But since the crash UI
> will look different anyway, I'm OK with just showing the indicator on the
> tab that is showing the submit crash UI.
> 
> > > 3) selecting a background tab while viewing the crashed tab ui launches a
> > > new content process and loads its content.
> > 
> > Great. 
> > 
> > With these changes, can we get rid of the "Restore All Crashed Tabs" button
> > in the crash ui? If we auto-reload when brought to foreground, it serves no
> > purpose. This would also allow us to set the "Restore this tab" button as
> > the default.
> 
> Sounds good!

awesome, thanks!

Comment 6

[Jim Mathies [:jimm]]

An additional scenario that needs to be investigated:

1) open two tabs: tab-1, tab-2
2) tab-1 in foreground
3) content process crashes: tab-1 displays crash submit ui for crash-1
4) user switches to tab-2 to reload it to see if he/she lost data
5) tab-2 in foreground, content process restarts for tab-2
6) content process crashes: tab-2 displays crash submit ui for crash-2

In the above scenario, do we still have the opportunity to submit crash info for crash-1?

if yes: we're golden, both tabs have the crash submit form in them
if no: tab-1 needs to load the generic tabcrashed ui and tab-2/crash-2 get the submit form

Comment 7

[Jim Mathies [:jimm]]

Hey mike, how much work do you think this would be to implement? Wondering if we could slip this into m8.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jim Mathies [:jimm] from comment #7)
> Hey mike, how much work do you think this would be to implement? Wondering
> if we could slip this into m8.

This might actually not be too bad. I'm willing to take this on my M8 stack.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r?Mossop

Comment 10

[Mike Conley (:mconley) (:⚙️)]

No fix yet, but here's a test.

Comment 11

[Jim Mathies [:jimm]]

per a discussion with brad, functionality work should be in m8. renoming so we can find an owner.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

I can take this.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

And putting into M8 as per comment 11.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

https://reviewboard.mozilla.org/r/21915/#review19851

::: browser/components/sessionstore/test/browser_unrestored_crashedTabs.js:17
(Diff revision 1)
> +  Services.prefs.setBoolPref(PREF, true)

Semicolon at the end.

::: browser/components/sessionstore/test/browser_unrestored_crashedTabs.js:18
(Diff revision 1)
> +  registerCleanupFunction(() => Services.prefs.clearUserPref(PREF));

Boy I wish we had a test helper for setting a pref which automatically dealt with setting the pref back to its original value at the end of the test *nudge nudge wink wink*

::: browser/components/sessionstore/test/browser_unrestored_crashedTabs.js:52
(Diff revision 1)
> +

Presumably the original browser is still crashed at this point? Verify that and that you can reload it.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Bug 1209689 - Flesh out BrowserTestUtils.waitForAttribute's capabilities a bit more. r?Gijs

This adds the ability for waitForAttribute to check for when an attribute
is removed. It also will check for the desired state before attaching
the MutationObserver, in the event that the attribute was changed
before waitForAttribute was called.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

https://reviewboard.mozilla.org/r/23355/#review20805

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:619
(Diff revision 1)
>    waitForAttribute(attr, element, value) {

I know it's the case already but lets make value default to undefined explicitely here.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:626
(Diff revision 1)
> +                (value && element.getAttribute(attr) === value));

This is convoluted and tricky to follow. Thankfully getAttribute returns null when there is no attribute so:

    return value === undefined ? element.hasAttribute(attr) :
                                 element.getAttribute(attr) === value

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

https://reviewboard.mozilla.org/r/22503/#review20807

::: browser/components/sessionstore/SessionStore.jsm:2229
(Diff revision 2)
> +    browser.loadURI("about:blank", null, null);

Won't this tab be restored immediately anyway regardless of whether restoring on demand?

::: browser/components/sessionstore/SessionStore.jsm:2303
(Diff revision 2)
> +    tab.linkedBrowser.__SS_restoreState = TAB_STATE_WILL_RESTORE;

What is this useful for, you never check for it anywhere

Comment 22

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dave Townsend [:mossop] from comment #21)
> Comment on attachment 8675915 [details]
> MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should
> not crash. r?ttaubert
> 
> https://reviewboard.mozilla.org/r/22503/#review20807
> 
> ::: browser/components/sessionstore/SessionStore.jsm:2229
> (Diff revision 2)
> > +    browser.loadURI("about:blank", null, null);
> 
> Won't this tab be restored immediately anyway regardless of whether
> restoring on demand?

The history will, yes - but the remoteness won't be flipped, and the content will not be loaded until restoreTabContent is called now. That leaves a small window of time where about:tabcrashed is still visible for a frame or two when selecting a revived tab.

STR to see what I mean:

1) Remove the loadURI bit
2) Create a number of tabs in your window, and make sure they're all loaded
3) Crash the content process
4) Choose to Restore All Tabs
5) Choose one of the background tabs

You will probably see a flash of about:tabcrashed before the content is loaded.

> 
> ::: browser/components/sessionstore/SessionStore.jsm:2303
> (Diff revision 2)
> > +    tab.linkedBrowser.__SS_restoreState = TAB_STATE_WILL_RESTORE;
> 
> What is this useful for, you never check for it anywhere

This is a guard to prevent something like onTabSelect from attempting to restoreTabContent while a navigateAndRestore is still in progress.

That ended up being the solution for a test failure that the rest of this patch was causing in browser_replace_load.js.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/22503/#review20807

Sorry, I probably should have responded in MozReview instead of Bugzilla. Copying over to here for posterity.

> Won't this tab be restored immediately anyway regardless of whether restoring on demand?

The history will, yes - but the remoteness won't be flipped, and the content will not be loaded until restoreTabContent is called now. That leaves a small window of time where about:tabcrashed is still visible for a frame or two when selecting a revived tab.

STR to see what I mean:

1) Remove the loadURI bit
2) Create a number of tabs in your window, and make sure they're all loaded
3) Crash the content process
4) Choose to Restore All Tabs
5) Choose one of the background tabs

You will probably see a flash of about:tabcrashed before the content is loaded.

> What is this useful for, you never check for it anywhere

This is a guard to prevent something like onTabSelect from attempting to restoreTabContent while a navigateAndRestore is still in progress.

That ended up being the solution for a test failure that the rest of this patch was causing in browser_replace_load.js.

Or is there another solution here that I'm not seeing?

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Garrr... I just talked to jimm, and I misunderstood the requirements of this bug.

We don't just want unrestored tabs to not crash - we want background tabs to not go to about:tabcrashed as well. Only selected tabs should visit about:tabcrashed - the rest should go into the unrestored state.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jim Mathies [:jimm] from comment #6)
> An additional scenario that needs to be investigated:
> 
> 1) open two tabs: tab-1, tab-2
> 2) tab-1 in foreground
> 3) content process crashes: tab-1 displays crash submit ui for crash-1
> 4) user switches to tab-2 to reload it to see if he/she lost data
> 5) tab-2 in foreground, content process restarts for tab-2
> 6) content process crashes: tab-2 displays crash submit ui for crash-2
> 
> In the above scenario, do we still have the opportunity to submit crash info
> for crash-1?
> 
> if yes: we're golden, both tabs have the crash submit form in them
> if no: tab-1 needs to load the generic tabcrashed ui and tab-2/crash-2 get
> the submit form

Yes, I believe we have the opportunity to submit the crash info for crash-1 - as long as the crashed browser is still around, we still have the mapping from the browser permanent key to the ID of the child that crashed (and therefore to the dump ID).

If, however, the same browser re-crashes, then that information will be lost for that browser. But in the scenario you laid out, the two individual crash dumps should still be available and able to be submitted from each crashed tab.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

What should be the behaviour if a non-remote (and therefore, non-crashable) tab (like about:newtab) is selected when the content process crashes?

Do we just not show about:tabcrashed at all?

Comment 27

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #26)
> What should be the behaviour if a non-remote (and therefore, non-crashable)
> tab (like about:newtab) is selected when the content process crashes?
> 
> Do we just not show about:tabcrashed at all?

Yeah, that sounds like the right call.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Bug 1209689 - Flesh out BrowserTestUtils.waitForAttribute's capabilities a bit more. r?Gijs

This adds the ability for waitForAttribute to check for when an attribute
is removed. It also will check for the desired state before attaching
the MutationObserver, in the event that the attribute was changed
before waitForAttribute was called.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

Bug 1209689 - Only show about:tabcrashed for the selected tab. r?Mossop

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Bug 1209689 - Flesh out BrowserTestUtils.waitForAttribute's capabilities a bit more. r?Gijs

This adds the ability for waitForAttribute to check for when an attribute
is removed. It can also check for the desired state before attaching
the MutationObserver, in the event that the attribute was changed
before waitForAttribute was called.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

Bug 1209689 - Only show about:tabcrashed for the selected tab. r?Mossop

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682582 [details]
MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Bug 1209689 - Flesh out BrowserTestUtils.waitForAttribute's capabilities a bit more. r?Gijs

This adds the ability for waitForAttribute to check for when an attribute
is removed. It can also check for the desired state before attaching
the MutationObserver, in the event that the attribute was changed
before waitForAttribute was called.

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

Bug 1209689 - Only show about:tabcrashed for the selected tab. r?Mossop

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Comment 47

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682582 [details]
MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Comment 49

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/23355/#review20805

> This is convoluted and tricky to follow. Thankfully getAttribute returns null when there is no attribute so:
> 
>     return value === undefined ? element.hasAttribute(attr) :
>                                  element.getAttribute(attr) === value

Actually, that's not true. getAttribute returns the empty string if the attribute is missing. :/

Comment 50

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/23355/#review20805

> Actually, that's not true. getAttribute returns the empty string if the attribute is missing. :/

Ugh, because we follow a different version of the DOM spec in XUL than we do in HTML. Awesome! I'll probably want to look over this again before landing then.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Bug 1209689 - Flesh out BrowserTestUtils.waitForAttribute's capabilities a bit more. r?Gijs

This adds the ability for waitForAttribute to check for when an attribute
is removed. It can also check for the desired state before attaching
the MutationObserver, in the event that the attribute was changed
before waitForAttribute was called.

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

Bug 1209689 - Only show about:tabcrashed for the selected tab. r?Mossop

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Comment 56

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682582 [details]
MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert

Comment 59

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Comment 60

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop

Comment 61

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

Bug 1209689 - Only show about:tabcrashed for the selected tab. r?Mossop

Comment 62

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Comment 63

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Comment 64

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682582 [details]
MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Comment 65

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22503/diff/7-8/

Comment 66

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23355/diff/6-7/

Comment 67

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21915/diff/8-9/

Comment 68

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24087/diff/5-6/

Comment 69

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24089/diff/5-6/

Comment 70

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24091/diff/5-6/

Comment 71

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682582 [details]
MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24093/diff/5-6/

Comment 72

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Instead of introducing hackery with BrowserTestUtils.waitForAttribute, I just modified this test to not wait for the attribute (since this test would fail if the tab does not get restored anyhow).

Comment 73

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

https://reviewboard.mozilla.org/r/24087/#review22093

::: browser/base/content/tabbrowser.xml:4424
(Diff revision 6)
> +            SessionStore.reviveCrashedTab(tab);

what's gonna be the effect of this for people who have browser.sessionstore.restore_on_demand = false?  Will a crash of the content process immediatelly restore all tabs in the background while only loading about:tabcrashed in the foreground tab?

Comment 74

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

https://reviewboard.mozilla.org/r/24091/#review22095

Comment 75

[Dave Townsend [:mossop]]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

https://reviewboard.mozilla.org/r/22503/#review22227

Comment 76

[Dave Townsend [:mossop]]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

https://reviewboard.mozilla.org/r/23355/#review22231

Comment 77

[Dave Townsend [:mossop]]

Comment on attachment 8682582 [details]
MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

https://reviewboard.mozilla.org/r/24093/#review22233

Comment 79

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/24087/#review22093

> what's gonna be the effect of this for people who have browser.sessionstore.restore_on_demand = false?  Will a crash of the content process immediatelly restore all tabs in the background while only loading about:tabcrashed in the foreground tab?

Yes, that's what's going to happen (but thanks for reminding me, since it looks like with that pref disabled, things break horribly since we attempt to restore tabs immediately before ContentParent has had a chance to fully shut down).

I'm working on fixing that now - but do you have concerns about us immediately restoring like this on a crash (with the exception of the selected tab at about:tabcrashed)?

Comment 80

[Dave Townsend [:mossop]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #79)
> I'm working on fixing that now - but do you have concerns about us
> immediately restoring like this on a crash (with the exception of the
> selected tab at about:tabcrashed)?

If a page you aren't looking at caused the crash and if it is reproducible you'll get into a crash loop

Comment 81

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dave Townsend [:mossop] from comment #80)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #79)
> > I'm working on fixing that now - but do you have concerns about us
> > immediately restoring like this on a crash (with the exception of the
> > selected tab at about:tabcrashed)?
> 
> If a page you aren't looking at caused the crash and if it is reproducible
> you'll get into a crash loop

Good point. :/

Do you recommend we restore on demand in the crashing case then?

Comment 82

[:Felipe Gomes (needinfo for replies!)]

That sounds a reasonable option to me. Let's see what Mossop says.

Comment 83

[Mike Conley (:mconley) (:⚙️)]

Another idea would be to keep a record of the most recent time that a tab was revived. If a tab crashed within some threshold of it having been revived, THEN we show about:tabcrashed in it - kinda like how we show about:sessionrestore if the whole browser crashes within some threshold since another crash.

Comment 84

[Dave Townsend [:mossop]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #81)
> (In reply to Dave Townsend [:mossop] from comment #80)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #79)
> > > I'm working on fixing that now - but do you have concerns about us
> > > immediately restoring like this on a crash (with the exception of the
> > > selected tab at about:tabcrashed)?
> > 
> > If a page you aren't looking at caused the crash and if it is reproducible
> > you'll get into a crash loop
> 
> Good point. :/
> 
> Do you recommend we restore on demand in the crashing case then?

Sounds reasonable to me

Comment 85

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22503/diff/8-9/

Comment 86

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23355/diff/7-8/

Comment 87

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21915/diff/9-10/

Comment 88

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24087/diff/6-7/

Comment 89

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24089/diff/6-7/

Comment 90

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24091/diff/6-7/

Comment 91

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682582 [details]
MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24093/diff/6-7/

Comment 92

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?felipe

Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?Mossop

Comment 93

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1209689 - Test forcing revived background tabs to restore on demand. r?felipe

Bug 1209689 - Test forcing revived background tabs to restore on demand. r?Mossop

Comment 94

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/24087/#review22093

> Yes, that's what's going to happen (but thanks for reminding me, since it looks like with that pref disabled, things break horribly since we attempt to restore tabs immediately before ContentParent has had a chance to fully shut down).
> 
> I'm working on fixing that now - but do you have concerns about us immediately restoring like this on a crash (with the exception of the selected tab at about:tabcrashed)?

We now force background tabs that are revived to restore on demand, regardless of the restore_on_demand pref. See the last set of patches in this series.

Comment 95

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

https://reviewboard.mozilla.org/r/24087/#review22473

Comment 96

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

https://reviewboard.mozilla.org/r/24089/#review22475

Comment 97

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8685783 [details]
MozReview Request: Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?felipe

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

Comment 98

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8685784 [details]
MozReview Request: Bug 1209689 - Test forcing revived background tabs to restore on demand. r?felipe

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

Comment 99

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8685783 [details]
MozReview Request: Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?felipe

https://reviewboard.mozilla.org/r/24905/#review22853

::: browser/components/sessionstore/SessionStore.jsm:3033
(Diff revision 1)
> -    } else {
> +    } else if (!forceOnDemand) {

Ok, thanks for the explanation on IRC about this part. I see that it makes sense not to add the tab to the queue here, but it's unclear how that should interact with the onTabShow/onTabHide events from SessionStore.jsm.

If a tab was not added to the queue, the call to hiddenToVisible/visibleToHidden will throw. I was thinking we could replace the error with a warning (or just ignore that condition entirely), but I don't know exactly what the expected behavior would be.


But I guess that, regardless of restore on demand true or false, a crashed tab should just never be part of the restore queue. In that case this is be fine, with the removal of those `throw new Error` mentioned.


Ok, I think I have one good test to do to check this:
- crash the content process with two tab groups
- switch groups
- make sure that the tab that just became the foreground tab now gets restored

Comment 100

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675915 [details]
MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22503/diff/9-10/

Comment 101

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8679052 [details]
MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23355/diff/8-9/

Comment 102

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21915/diff/10-11/

Comment 103

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682579 [details]
MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24087/diff/7-8/

Comment 104

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24089/diff/7-8/

Comment 105

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24091/diff/7-8/

Comment 106

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8682582 [details]
MozReview Request: Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24093/diff/7-8/

Comment 107

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8685783 [details]
MozReview Request: Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?felipe

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

Comment 108

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8685784 [details]
MozReview Request: Bug 1209689 - Test forcing revived background tabs to restore on demand. r?felipe

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

Comment 109

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/24905/#review22853

> Ok, thanks for the explanation on IRC about this part. I see that it makes sense not to add the tab to the queue here, but it's unclear how that should interact with the onTabShow/onTabHide events from SessionStore.jsm.
> 
> If a tab was not added to the queue, the call to hiddenToVisible/visibleToHidden will throw. I was thinking we could replace the error with a warning (or just ignore that condition entirely), but I don't know exactly what the expected behavior would be.
> 
> 
> But I guess that, regardless of restore on demand true or false, a crashed tab should just never be part of the restore queue. In that case this is be fine, with the removal of those `throw new Error` mentioned.
> 
> 
> Ok, I think I have one good test to do to check this:
> - crash the content process with two tab groups
> - switch groups
> - make sure that the tab that just became the foreground tab now gets restored

Hey - so, re-examining how TabRestoreQueue works, I've made it so that we continue to add the tab to the queue, we just don't call restoreNextTab(). Seems kinda obvious in retrospect, but for some reason that was hard to think of before. :)

Would you still want a test-case for the above?

Comment 110

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8685783 [details]
MozReview Request: Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?felipe

https://reviewboard.mozilla.org/r/24905/#review23021

r+ assuming everything works fine. I didn't mean writing a test for that scenario, just manually testing that it behaves well.  With the new patch I think it's still worth doing that manual testing. The risk now with the newer version is if there's any behavior that can trigger a cascade of restoring the crashed tabs if restore_on_demand=false  (by looking at the code, I'm thinking that a tab group switch will cause this)

Comment 111

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8685784 [details]
MozReview Request: Bug 1209689 - Test forcing revived background tabs to restore on demand. r?felipe

https://reviewboard.mozilla.org/r/24907/#review23023

Comment 112

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Felipe Gomes from comment #110)
> Comment on attachment 8685783 [details]
> MozReview Request: Bug 1209689 - Force tab restoration on demand when
> crashed tabs are revived. r?felipe
> 
> https://reviewboard.mozilla.org/r/24905/#review23021
> 
> The risk now with the
> newer version is if there's any behavior that can trigger a cascade of
> restoring the crashed tabs if restore_on_demand=false  (by looking at the
> code, I'm thinking that a tab group switch will cause this)

Hrm, yes, that _is_ what's occurring in that scenario. :/ But do we really care that much, given that panorama is going away?

Comment 113

[Mike Conley (:mconley) (:⚙️)]

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

Comment 114

[:Felipe Gomes (needinfo for replies!)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #112)
> (In reply to :Felipe Gomes from comment #110)
> > Comment on attachment 8685783 [details]
> > MozReview Request: Bug 1209689 - Force tab restoration on demand when
> > crashed tabs are revived. r?felipe
> > 
> > https://reviewboard.mozilla.org/r/24905/#review23021
> > 
> > The risk now with the
> > newer version is if there's any behavior that can trigger a cascade of
> > restoring the crashed tabs if restore_on_demand=false  (by looking at the
> > code, I'm thinking that a tab group switch will cause this)
> 
> Hrm, yes, that _is_ what's occurring in that scenario. :/ But do we really
> care that much, given that panorama is going away?

Probably not enough to drag this bug further. I'm not sure exactly what are the plans for the Panorama removal, but I heard that some of the APIs might remain. If the hidden tabs API remain, then this probably need to be fixed.  Let's figure this out in a follow-up.

I think though that the first version of the patch (which didn't add to the list), + removing the Errors thrown hiddenToVisible/visibleToHidden, would be better (and not cause this prob).  But I'll let you choose

Comment 115

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/24905/#review22853

> Hey - so, re-examining how TabRestoreQueue works, I've made it so that we continue to add the tab to the queue, we just don't call restoreNextTab(). Seems kinda obvious in retrospect, but for some reason that was hard to think of before. :)
> 
> Would you still want a test-case for the above?

Alright, removing the throw's and then I'll file a follow-up. Thanks!

Comment 116

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d5c2bb1d594

Comment 117

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/fx-team/rev/243353ae9c0883275bacd5faea33e4dd9909daef
Bug 1209689 - Tabs that haven't yet been restored should not crash. r=Mossop

https://hg.mozilla.org/integration/fx-team/rev/e9147c785bb2ab3c64e70fa3469d00edb91d0c4b
Bug 1209689 - Update browser_replace_load for async tab restoration. r=Mossop

https://hg.mozilla.org/integration/fx-team/rev/e099a63d77b46dd9fc115210ca432b1c66ac1bc1
Bug 1209689 - Regression test. r=Mossop

https://hg.mozilla.org/integration/fx-team/rev/88f0ef22ab78502203405e8da70d61e247f76dc2
Bug 1209689 - Only show about:tabcrashed for the selected tab. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/6ee79ecb5b03dd205e5c3de00bdf0ab0f0128326
Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/1c2698493944a81d07fddb07d3d21e82804671c7
Bug 1209689 - Fix browser_async_flushes.js. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/b8dd7691d17899d792b07c5a197d67d49c3d0c00
Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r=Mossop

https://hg.mozilla.org/integration/fx-team/rev/ba8e082be3988dca3afea760f40f13bd05d1f2e7
Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/924d421d766ad07c2e76a1a299ec3e05b7ad0b4f
Bug 1209689 - Test forcing revived background tabs to restore on demand. r=felipe

Comment 118

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/243353ae9c08
https://hg.mozilla.org/mozilla-central/rev/e9147c785bb2
https://hg.mozilla.org/mozilla-central/rev/e099a63d77b4
https://hg.mozilla.org/mozilla-central/rev/88f0ef22ab78
https://hg.mozilla.org/mozilla-central/rev/6ee79ecb5b03
https://hg.mozilla.org/mozilla-central/rev/1c2698493944
https://hg.mozilla.org/mozilla-central/rev/b8dd7691d178
https://hg.mozilla.org/mozilla-central/rev/ba8e082be398
https://hg.mozilla.org/mozilla-central/rev/924d421d766a

Comment 119

[The 8472]

Is it possible that this bug (or something related) changed the way session restore works? Specifically lazy-loaded background tabs, i.e. about:blank windows, seem to go into the parent process now instead of the child process.

That obviously shifts memory footprint from the child to the parent and has a negative performance impact for me (many background tabs).

Comment 120

[Mike Conley (:mconley) (:⚙️)]

(In reply to The 8472 from comment #119)
> Is it possible that this bug (or something related) changed the way session
> restore works? Specifically lazy-loaded background tabs, i.e. about:blank
> windows, seem to go into the parent process now instead of the child process.

Yes. This patch changes things so that unrestored background tabs create non-remote browsers that run in the parent process. This is to avoid having the unrestored background tabs go into the crashed state when their process dies.

> 
> That obviously shifts memory footprint from the child to the parent and has
> a negative performance impact for me (many background tabs).

It also caused a talos regression that put us on parity with single-process Firefox (as we'd been kicking the talos tests butt before). This doesn't block shipping e10s, but it's something we want to address soon - see bug 1227275.

Comment 121

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1209689.html