963358 - [e10s] Crashed tab's "Try again" button does not always reload tabs

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Chris Peterson [:cpeterson]]

This is an intermittent problem.

Comment 1

[Tom S]

I think I noticed this while working on bug 961360 as well.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

The intermittency of this problem might be what Tom described on bug 961360 comment 9, i.e. it works for the first window but not for subsequent ones.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I wasn't able to reproduce this, so closing WFM for now. We can re-open if someone sees it again.

Comment 5

[Anne (:annevk)]

I just had this problem. But I can't make these tabs crash again... Happened for twitter.com, transip.nl, newsblur.com. For some sites it worked after a couple of attempts (fetch.spec.whatwg.org).

Comment 6

[Anne (:annevk)]

Note also that bug 961360 definitely did not appear fixed. Had to press it for each tab.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

I think this bug never got fixed, but was just hidden by other problems from bug 961360. I thought that bug 1057663 would have helped but apparently not. However, I expect this to be fixed by 1065785. Let's keep this open to verify that it gets fixed.

(In reply to Anne (:annevk) from comment #6)
> Note also that bug 961360 definitely did not appear fixed. Had to press it
> for each tab.

Bug 961360 was reversed by bug 1068210.

Comment 8

[Chris Peterson [:cpeterson]]

I still see this problem.

Comment 9

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

I also see this still. It's easy for me to reproduce on my Macbook with my tab crasher add-on (https://bugzilla.mozilla.org/attachment.cgi?id=8499038).

My STR:

1) Install Tab Crasher
2) Open two tabs, and browse to a few pages in one
3) Open a second tab and browser to a few pages in it
4) Hit the Tab Crash button in the toolbar to bring all of the tabs down
5) Immediately hit the "Try again" button in the tab you're in.

ER:

Should restore the tab you selected.

AR:

Nothing. browser.js doesn't even see the click event, apparently.

Note that if you switch to the other tab and click Try Again in it, it often works.

Comment 10

[martin]

I see this too.
BuildID
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141022030202 CSet: ae4d9b4ff2ee

Comment 11

[martin]

Further testing reveals that I can reproduce this only for tabs that have flash or silverlight OOP content, other tabs reload just fine.
(Silverlight and Flash currently crashes for me due to https://bugzilla.mozilla.org/show_bug.cgi?id=1087410 )

Steps to reproduce:
Open some Tabs
Browse to http://bubblemark.com/silverlight2.html or http://kurier.at/video/film-at/schedls-filmschau-maze-runner/91521810
click to activate -> crash (due to 1087410) 
Clicking Reload Tab in the tab which hosts content with plugins will do nothing.
Clicking Reload Tab in a Tab without active plugins will work.

Expected Result:
Reload Tab should work after all crashes.

Hope this helps in nailing this issue down.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Nice find! I think everyone has seen this bug before but on simple tests no one can reproduce. Given the reliable STR now we should probably re-triage it.

worth checking if Bill has seen this independently with his recent work on plugins.

Comment 13

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

I wasn't able to reproduce with Martin's steps (I didn't crash when activating those plugins, since bug 1087410 is fixed), but I was able to reproduce with this:

1) Visit http://www.cbc.ca/
2) As the page is half-way through loading, crash the tab with my tab crasher add-on: https://bugzilla.mozilla.org/attachment.cgi?id=8499038
3) Hit "Try again" to try to revive the tab

ER:

Tab should revive

AR:

Tab does not revive, and I still see a floating status bar on the bottom saying that it's still loading content from cbc.ca


Workaround: Type a URL into the URL bar and press enter. This will revive the tab automatically. You can then hit "back" to go back to where you were when you crashed.

Comment 15

[martin]

Using tab Crasher as in comment 13 I can still reproduce on the following 2 pages with flash
http://kurier.at/video/film-at/schedls-filmschau-maze-runner/91521810
http://www.cbc.ca/

Works (i.e. Tab revives) for the following pages with silverlight
http://bubblemark.com/silverlight2.html
http://www2.netflix.com/WiPlayer?movieid=70219878&trkid=13462100&tctx=-99%2C-99%2C3159c717-c478-40fd-933b-bd1f6d83d7c2-9771939

BuildID: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141114030206 CSet: 7f0d92595432

Comment 16

[martin]

More Testing: This is reproducible (i.e. tabs do not revive) even if Flash is set to be activated only on click and NOT activated on the page loaded.
Tested with 
http://kurier.at/video/film-at/schedls-filmschau-maze-runner/91521810
http://www.cbc.ca/
Same BuildID as commend 15

Comment 17

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

I can't staaaaand this bug. So I did some digging.

We attach a click event listener in browser.js called BrowserOnClick to handle clicks on certain things, like the Try Again button, and tiles in about:newtab.

That click event listener is added in browser.js's TabProgressListener::onStateChange, under certain conditions. I've determined that in the tabs where Try Again does not work, the reason is because this event listener has not been set.

Comment 18

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

Attachment: [e10s] Crashed tab page's Try Again button doesn't always work. r=?

Instead of attaching / listening for click events in BrowserOnClick for the
Try Again button, we instead bubble up an event when the Try Again button
is clicked. That event has a listener set up for it that is established during
delayedStartup, so it has a very high probability of being set. This bypasses
some error cases where we hadn't successfully attached BrowserOnClick in the past.

Comment 19

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

Comment on attachment 8527276 [details] [diff] [review]
[e10s] Crashed tab page's Try Again button doesn't always work. r=?

I think this'll make clicking Try Again far more successful far more often.

Comment 20

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

(Sorry for stealing, but this was really bugging me)

Comment 21

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

Comment on attachment 8527276 [details] [diff] [review]
[e10s] Crashed tab page's Try Again button doesn't always work. r=?

I forgot to add a check that the sender of the event is about:tabcrashed.

Comment 22

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

Attachment: [e10s] Crashed tab page's Try Again button doesn't always work. r=?

Instead of attaching / listening for click events in BrowserOnClick for the
Try Again button, we instead bubble up an event when the Try Again button
is clicked. That event has a listener set up for it that is established during
delayedStartup, so it has a very high probability of being set. This bypasses
some error cases where we hadn't successfully attached BrowserOnClick in the past.

Comment 23

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

Comment on attachment 8527754 [details] [diff] [review]
[e10s] Crashed tab page's Try Again button doesn't always work. r=?

Added about:tabcrashed check (though it seems that arbitrary content firing an event named AboutTabCrashedTryAgain wouldn't normally get handled in browser.js anyway).

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8527754 [details] [diff] [review]
[e10s] Crashed tab page's Try Again button doesn't always work. r=?

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

Thanks!

Comment 25

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

Thanks for the fast review, billm:

remote:   https://hg.mozilla.org/integration/fx-team/rev/16a220a579a6

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/16a220a579a6

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8527754 [details] [diff] [review]
[e10s] Crashed tab page's Try Again button doesn't always work. r=?

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

::: browser/base/content/browser.js
@@ +1159,5 @@
> +      if (event.detail.sendCrashReport) {
> +        TabCrashReporter.submitCrashReport(browser);
> +      }
> +#endif
> +      let tab = gBrowser.getTabForBrowser(browser);

It's too bad we couldn't use gBrowser._getTabForContentWindow, since gBrowser.getBrowserForDocument gets the tab and then returns tab.linkedBrowser. So this patch actually gets the tab, then through that gets the linked browser, then uses the browser in conjunction with the tab<->browser map to get the tab. Quite round-a-bout :-/

Do you think _getTabForContentWindow should be made public?

Comment 29

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27)
> Comment on attachment 8527754 [details] [diff] [review]
> [e10s] Crashed tab page's Try Again button doesn't always work. r=?
> 
> Review of attachment 8527754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +1159,5 @@
> > +      if (event.detail.sendCrashReport) {
> > +        TabCrashReporter.submitCrashReport(browser);
> > +      }
> > +#endif
> > +      let tab = gBrowser.getTabForBrowser(browser);
> 
> It's too bad we couldn't use gBrowser._getTabForContentWindow, since
> gBrowser.getBrowserForDocument gets the tab and then returns
> tab.linkedBrowser. So this patch actually gets the tab, then through that
> gets the linked browser, then uses the browser in conjunction with the
> tab<->browser map to get the tab. Quite round-a-bout :-/
> 
> Do you think _getTabForContentWindow should be made public?

I agree, that's pretty circuitous under the hood.

I'd be fine exposing and using _getTabForContentWindow so long as we document in tabbrowser.xml how expensive iterating a series of CPOWs can be, as well as documenting here in browser.js that we avoid the CPOW look-ups since an about:tabcrashed browser is not going to be remote.

I filed bug 1105450 as a follow-up to look into this.

Comment 30

[Ada [:adalucinet]]

Reproduced with Nightly 36.0a1 from 2014-11-10 with str from comment 13: nothing happens when ‘Try again’ button is clicked.
Verified as fixed on latest Nightly (Build ID: 20150210030231) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5 with the same str; since a new style for e10s crash pages was added in bug 1087618, verified that the crashed tab is properly restored by using ‘Restore This Tab’ and ‘Restore All Crashed Tabs’ options and crash reports are submitted accordingly.