Closed Bug 1209689 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
e10s m8+ ---
firefox45 --- fixed

People

(Reporter: jimm, Assigned: mconley)

References

(Depends on 1 open bug, )

Details

Attachments

(11 files)

44.25 KB, image/jpeg
Details
40 bytes, text/x-review-board-request
mossop
: review+
Details
40 bytes, text/x-review-board-request
mossop
: review+
Details
40 bytes, text/x-review-board-request
mossop
: review+
Details
40 bytes, text/x-review-board-request
Felipe
: review+
Details
40 bytes, text/x-review-board-request
Felipe
: review+
Details
40 bytes, text/x-review-board-request
Felipe
: review+
Details
40 bytes, text/x-review-board-request
mossop
: review+
Details
40 bytes, text/x-review-board-request
Felipe
: review+
Details
40 bytes, text/x-review-board-request
Felipe
: review+
Details
24.20 KB, application/zip
Details
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.
Attached image crashed tabs
Version: 37 Branch → 44 Branch
Flags: needinfo?(philipp)
Keywords: uiwanted
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.
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
(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!
Flags: needinfo?(philipp)
(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!
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
Hey mike, how much work do you think this would be to implement? Wondering if we could slip this into m8.
Flags: needinfo?(mconley)
Keywords: uiwanted
Blocks: 1191440
Blocks: 1204774
(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.
Flags: needinfo?(mconley)
Bug 1209689 - Regression test. r?Mossop
Attachment #8673356 - Flags: review?(dtownsend)
No fix yet, but here's a test.
per a discussion with brad, functionality work should be in m8. renoming so we can find an owner.
I can take this.
Assignee: nobody → mconley
And putting into M8 as per comment 11.
Attachment #8673356 - Flags: review?(dtownsend) → review+
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 on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop
Attachment #8673356 - Attachment description: MozReview Request: Bug 1209689 - Regression test. r?Mossop → MozReview Request: Bug 1209689 - Regression test. r=Mossop
Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert
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.
Attachment #8679052 - Flags: review?(dtownsend)
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
Attachment #8675915 - Flags: review?(dtownsend)
Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop
Attachment #8679052 - Flags: review?(dtownsend) → review+
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
Attachment #8675915 - Flags: review?(dtownsend)
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
(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.
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?
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.
(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.
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?
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
Depends on: 1220929
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 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 on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop
Bug 1209689 - Only show about:tabcrashed for the selected tab. r?Mossop
Bug 1209689 - Ensure browsers are properly removed from SessionStore's crashedBrowser set. r?Mossop
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 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 on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop
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 on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe
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 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 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 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 on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop
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 on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe
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 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
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. :/
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 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 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 on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop
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 on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe
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 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 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 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
Attachment #8679052 - Attachment description: MozReview Request: Bug 1209689 - Flesh out BrowserTestUtils.waitForAttribute's capabilities a bit more. r?Gijs → MozReview Request: Bug 1209689 - Update browser_replace_load for async tab restoration. r?Mossop
Comment on attachment 8673356 [details]
MozReview Request: Bug 1209689 - Regression test. r=Mossop

Bug 1209689 - Regression test. r=Mossop
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 on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

Bug 1209689 - browser_crashedTabs.js fixes. r=felipe
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 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
Attachment #8675915 - Attachment description: MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?ttaubert → MozReview Request: Bug 1209689 - Tabs that haven't yet been restored should not crash. r?Mossop
Attachment #8675915 - Flags: review?(dtownsend)
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 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 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/
Attachment #8682579 - Attachment description: MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?Mossop → MozReview Request: Bug 1209689 - Only show about:tabcrashed for the selected tab. r?felipe
Attachment #8682579 - Flags: review?(felipc)
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 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/
Attachment #8682580 - Flags: review?(felipc)
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/
Attachment #8682581 - Flags: review?(felipc)
Attachment #8682582 - Flags: review?(dtownsend)
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 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).
Attachment #8679052 - Flags: review+ → review?(dtownsend)
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?
Attachment #8682579 - Flags: review?(felipc)
Attachment #8682581 - Flags: review?(felipc) → review+
Comment on attachment 8682581 [details]
MozReview Request: Bug 1209689 - Fix browser_async_flushes.js. r=felipe

https://reviewboard.mozilla.org/r/24091/#review22095
Attachment #8675915 - Flags: review?(dtownsend) → review+
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 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
Attachment #8679052 - Flags: review?(dtownsend) → review+
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
Attachment #8682582 - Flags: review?(dtownsend) → review+
Duplicate of this bug: 1185190
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)?
Flags: needinfo?(felipc)
(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
(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?
That sounds a reasonable option to me. Let's see what Mossop says.
Flags: needinfo?(felipc)
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.
(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 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 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 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/
Attachment #8682579 - Flags: review?(felipc)
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 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 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 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/
Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?Mossop
Attachment #8685783 - Flags: review?(dtownsend)
Bug 1209689 - Test forcing revived background tabs to restore on demand. r?Mossop
Attachment #8685784 - Flags: review?(dtownsend)
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 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
Attachment #8682579 - Flags: review?(felipc) → review+
Comment on attachment 8682580 [details]
MozReview Request: Bug 1209689 - browser_crashedTabs.js fixes. r=felipe

https://reviewboard.mozilla.org/r/24089/#review22475
Attachment #8682580 - Flags: review?(felipc) → review+
Attachment #8685783 - Flags: review?(dtownsend) → review?(felipc)
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 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/
Attachment #8685784 - Flags: review?(dtownsend) → review?(felipc)
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
Attachment #8685783 - Flags: review?(felipc)
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 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 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 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 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 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 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/
Attachment #8685783 - Attachment description: MozReview Request: Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?Mossop → MozReview Request: Bug 1209689 - Force tab restoration on demand when crashed tabs are revived. r?felipe
Attachment #8685783 - Flags: review?(felipc)
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 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/
Attachment #8685784 - Attachment description: MozReview Request: Bug 1209689 - Test forcing revived background tabs to restore on demand. r?Mossop → MozReview Request: Bug 1209689 - Test forcing revived background tabs to restore on demand. r?felipe
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?
Attachment #8685783 - Flags: review?(felipc) → review+
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)
Attachment #8685784 - Flags: review?(felipc) → review+
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
(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?
Flags: needinfo?(felipc)
(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
Flags: needinfo?(felipc)
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!
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
Depends on: 1227275
Depends on: 1227630
Depends on: 1228518
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).
(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.
Depends on: 1256280
See Also: → 1256472
You need to log in before you can comment on or make changes to this bug.