Closed Bug 1241459 Opened 9 years ago Closed 8 years ago

[e10s] A crashed background tab does not give option to submit

Categories

(Firefox :: General, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
e10s + ---
firefox52 --- fixed

People

(Reporter: jonathan, Assigned: mconley)

References

Details

(Whiteboard: [e10s-multi:M1])

Attachments

(3 files)

This is minor edge case until multiple content processes but still can be created with one. STR: One tab remote in background Foreground new tab page (ie not remote) kill -6 the plugin-container (or whatever windows/mac equivalent is) Crash is created but no means to submit.
Attached image crashedTab.tiff (deleted) —
I'm using the crashme add-on, but should be same result. I've attached a clip of Checkbox for "Submit a crash report to help prevent more bad news" that was checked but there was no crash reporter dialog. If I unchecked the box then check it again, the crash reporter dialog correctly appears below that option. Then seeing Bug 1241430 - "Crash report already submitted" screen is not displayed
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hey Mike, What do we want to do in this case? We're looking at a non-remote tab, and the background tabs all crash, since bug 1209689 we only show about:tabcrashed on the currently selected tab (if that tab is remote). So we don't show about:tabcrashed. Which means we don't show a UI for submitting a crash report for the content process. Which means we might miss out on some crash data. I think this is a corner case, but one we'll likely hit more as we add more content processes over time. Any suggestions on how to deal with background tab crashes, and surfacing some kind of UI for the user to submit a crash report?
Flags: needinfo?(mmaslaney)
I need to talk to phsla about this.
Flags: needinfo?(jgriffiths)
clearing needinfo, will talk to Philipp next week. I think this a low-priority edge-case until we start using multiple content processes. Jim, can you put this into the multi-process bucket?
Flags: needinfo?(jgriffiths) → needinfo?(jmathies)
Blocks: e10s-multi
Flags: needinfo?(jmathies)
Priority: -- → P2
Flags: needinfo?(gkrizsanits)
Based on Bug 1290886 comment 2, we still need to fix this. Marking it M1.
Flags: needinfo?(gkrizsanits)
Whiteboard: [e10s-multi:M1]
In the current released version we have one single content process that hosts all the tabs in all open windows. When _the_ content process crashed the selected tab will indicate the crash and allow the user to submit the crash report. If the user reload the current tab it will also reload all the pinned tabs (except if they crash again then it will give that part up) and selecting other tabs will reload them one by one. With multiple content processes one content process typically will host multiple tabs but not all the tabs. So we have a few more cases. If the content process of the currently selected tab is crashed, we can keep the old behavior. If however a background tab crashes we should: - indicate the crash of the related pinned tabs - the first time one of the tab from that particular content process is selected we should show the dialog and enable the user to reload / send crashreport (reload should reload pinned tabs as well) - automatically reload the rest of the related tabs the next time they are selected I'm not a UX person so before I start implement anything I need a confirmation. Philipp, I think you are in charge for these sort of decisions... what do you think?
Flags: needinfo?(philipp)
Hey Mike, since you have worked on the e10s version of this and now you are working on platform UX I thought you could help us out here a bit. This is quite important since as it stands this is a blocker for both the sandboxing team to enable a dedicated content process for file urls and for the e10s-multi team to turn on 2 content processes on nightly. For both, this is the biggest remaining blocker and we would like to get this done as soon as possible. Is there a chance you could take ownership of this bug?
Flags: needinfo?(mconley)
Yep.
Assignee: nobody → mconley
Flags: needinfo?(mconley)
So there is a related bug that I think this bug can piggyback off of - specifically, bug 1287178. A quick crash-course(ha) on bug 1287178: We currently show users on Nightly and Aurora the "unsubmitted crash reports" notification bar soon after start-up if such reports exist. Ideally, we'd never show that bar, but for now, there are often silent shutdown crashes that occur if Firefox doesn't close in time. This is often why the notification appears. We need those reports to make shutdown faster, and we're currently asking the user for them (previously, we'd just never send them and they'd expire). This rich data is very useful for making Firefox more stable and for improving shutdown times. We've got approval to ask the user to opt-in for automatically sending those reports, so that the notifications for the last sessions shutdown crashes should go away, and we'll still get the crash data. A proposal from shorlander on how to address this is in bug 1287178 comment 8. Probably best to read that proposal before proceeding. So now how about this bug? With e10s-multi in the pipe, background tabs can crash while the foreground tab does not. We want those crash reports as well. For this, I suspect the notification bar at the bottom of the screen is an appropriate mechanism to notify the user and give them the choice to submit the data or not. Assuming I can get bug 1287178 finished, I think this bug can piggyback off of its mechanism. Specifically, if a background tab or tabs crash, we should: - Check to see if the crash reporter is enabled. If not, we put the crashed tab into the background, unrestored state instead of reloading it automatically. I’m not 100% sure about this - should we just reload the page? Do we risk crash loops? - Assuming the crash reporter is enabled, show the notification bar with a message about how some background tabs crashed, and how Firefox will reload them when you select them (assuming we’re not reloading them automatically). That notification bar will ask the user if they want to submit the crash report data. The choices are “Submit”, “Always Submit”, and the X button on the right to close the notification bar. - The “X” button is interpreted as “No”, and the crash reports are marked so that we’ll never offer to send them like this again (except via about:crashes). - “Submit” will submit the crash reports and close the notification. - “Always Submit” will set the same pref that’s used in bug 1287178, in that it’ll submit the report, and the notification should never show up again. This means that if the user has ever chosen “Always Submit” from the startup check for unsubmitted crash reports, they’ll never see these background tab crash report notifications, and vice versa. - The pref for always submitting will, as in bug 1287178, be controlled via a checkbox in about:preferences. So, questions for consideration: - Are we okay to use the notification bar at the bottom? Or does a separate mechanism somehow make more sense? - Should we be putting these crashed background tabs into the unrestored background state? Or should we do a full reload of them? Note that there could be any number of crashed background tabs, so we could be setting off a chain of dozens and dozens of full-page reloads (we could probably chain them so that they don’t happen simultaneously, but still, that’s a lot of network traffic).
Flags: needinfo?(bwinton)
Flags: needinfo?(mmaslaney)
So, I would say try to reload the crashed page, but if it fails twice, leave it in the unrestored state to avoid crash loops. A notification bar at the bottom seems fine to me. It's unobtrusive, and this is a lower priority kinda thing. The network traffic thing is a good point. I'm not sure if that changes my answer or not…
Flags: needinfo?(bwinton)
Hey y'all -- sorry, I'm obviously late to the party here. I'm not a huge fan of the notification bar (particularly for anything beyond Aurora), since it leaves the user without context (oftentimes telling them that something has crashed, even though they didn't notice the crash). We could do the following (in declining order of desirability) 1) Somehow get users to opt into always submitting crash reports. I know that there's a lot of policy involved in that, but we should aim for something like this in the long time. It improves the experience and it improves our knowledge of about crashes. 2) Always make sure that the crashed tab page is shown once after a crash. Consider the following scenario: - User is on tab A - Tabs B and C crash - User doesn't notice and browses happily in tab A - User switches to tab B - User sees crashed page (and might submit a report) - User switches to tab C - Tab C reloads automatically
Flags: needinfo?(philipp)
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #11) > Hey y'all -- sorry, I'm obviously late to the party here. > > I'm not a huge fan of the notification bar (particularly for anything beyond > Aurora), since it leaves the user without context (oftentimes telling them > that something has crashed, even though they didn't notice the crash). > > We could do the following (in declining order of desirability) > > 1) Somehow get users to opt into always submitting crash reports. I know > that there's a lot of policy involved in that, but we should aim for > something like this in the long time. It improves the experience and it > improves our knowledge of about crashes. > Thanks phlsa. In bug 1287178, we got clearance to do a "Submit Always" opt-in on crash reports, so I'm adding that there. My original plan was to show a similar notification bar, with the same Submit Always option, in the case of a background tab crash. Does that jive with your Desirable #1?
Flags: needinfo?(philipp)
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #12) > In bug 1287178, we got clearance to do a "Submit Always" opt-in on crash > reports, so I'm adding that there. My original plan was to show a similar > notification bar, with the same Submit Always option, in the case of a > background tab crash. Does that jive with your Desirable #1? Am I reading you correctly that the notification bar would only show up if the crashed tab is a background tab? If so, can we make it so that it does only show up on the crashed background tab(s)? Mainly, I'm trying to limit the number of notification bars we are showing as much as possible, since they have been growing wildly for too long (and we have pointers that they aren't really that effective either).
Flags: needinfo?(philipp)
Also, could we include the »Always submit« option into the tab crashed page instead of a notification bar and then use the second option described in comment 11? That would make it more contextually relevant
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #14) > Also, could we include the »Always submit« option into the tab crashed page > instead of a notification bar and then use the second option described in > comment 11? > That would make it more contextually relevant Hm, okay, let me try to summarize, and you tell me if what I'm describing matches what's in your head: A user has 3 tabs open, A, B and C. They're browsing on A, and all is well. Tabs B and C are using the same content process, and suddenly, that content process crashes. The user is still browsing on Tab A, however, and does not notice. No notification is presented. The user eventually switches to one of the crashed tabs. Maybe it's B. Maybe it's C. It doesn't matter - the result is the same: the user sees the about:tabcrashed page. If the user switches to the other tab that has crashed, that tab auto-reloads - but the one at about:tabcrashed will continue to show that page until the user either restores it or closes it. Regarding "Always Send" - we might want to run that part past legal / privacy. They were okay with us allowing an opt-in for users to submit backlogged crash reports soon after start-up... but I think it's slightly different to automatically submit crash reports if the user's content process crashes. It's something I can double check with legal / privacy, anyhow. Assuming legal / privacy signs off, how would the tab crashed pages work in the future if Always Send had been clicked?
Flags: needinfo?(philipp)
Sorry, I realize that I mixed a few things up -- shouldn't rush out comments like this right before calling it a day ;) (In reply to Mike Conley (:mconley) - (needinfo me!) from comment #15) > Hm, okay, let me try to summarize, and you tell me if what I'm describing > matches what's in your head: > > A user has 3 tabs open, A, B and C. They're browsing on A, and all is well. > > Tabs B and C are using the same content process, and suddenly, that content > process crashes. > > The user is still browsing on Tab A, however, and does not notice. No > notification is presented. > > The user eventually switches to one of the crashed tabs. Maybe it's B. Maybe > it's C. It doesn't matter - the result is the same: the user sees the > about:tabcrashed page. If the user switches to the other tab that has > crashed, that tab auto-reloads - but the one at about:tabcrashed will > continue to show that page until the user either restores it or closes it. Exactly! Does that make sense to you? > Regarding "Always Send" - we might want to run that part past legal / > privacy. They were okay with us allowing an opt-in for users to submit > backlogged crash reports soon after start-up... but I think it's slightly > different to automatically submit crash reports if the user's content > process crashes. It's something I can double check with legal / privacy, > anyhow. > > Assuming legal / privacy signs off, how would the tab crashed pages work in > the future if Always Send had been clicked? Yeah, this might be a longer shot. If we actually *can* allow users to opt into automatically sending reports when something crashes (or pool them and send them at the next restart), we could stop showing crash pages altogether in most cases. The obvious exception would be crash loops as Blake mentioned above.
Flags: needinfo?(philipp)
Hey Mike, I've seen the prompt to submit crashes from background tabs. Is this fixed now through another bug?
Flags: needinfo?(mconley)
(In reply to Blake Kaplan (:mrbkap) from comment #17) > Hey Mike, I've seen the prompt to submit crashes from background tabs. Is > this fixed now through another bug? Not exactly. The notification is for backlogged crash reports, and while under the hood, they might use the same submission mechanism and rules, I'm still hoping to achieve phlsa's vision of the behaviour that he laid out in comment 15 and comment 16.
Flags: needinfo?(mconley)
Comment on attachment 8796688 [details] Bug 1241459 - Background tab crashes should only show about:tabcrashed for first selected tab. https://reviewboard.mozilla.org/r/82446/#review81062 Hoping that felipe can review the changes I've made to ContentCrashHandlers.jsm, and mikedeboer is cool with the changes I'm making to SessionStore.
Attachment #8796688 - Flags: review?(mdeboer)
Attachment #8796688 - Flags: review?(felipc)
Comment on attachment 8796688 [details] Bug 1241459 - Background tab crashes should only show about:tabcrashed for first selected tab. https://reviewboard.mozilla.org/r/82446/#review81064 Found some corner cases. :/ Fixed and automated tests coming up.
Comment on attachment 8796688 [details] Bug 1241459 - Background tab crashes should only show about:tabcrashed for first selected tab. https://reviewboard.mozilla.org/r/82446/#review82162 r=me for the sessionstore bits.
Attachment #8796688 - Flags: review?(mdeboer) → review+
Comment on attachment 8797739 [details] Bug 1241459 - Add regression tests for background tab crashing behaviour. https://reviewboard.mozilla.org/r/83374/#review82548 ::: browser/components/sessionstore/test/browser_background_tab_crash.js:148 (Diff revision 2) > + * Tests that if a content process crashes taking down only > + * background tabs, and the user is configured to send backlogged > + * crash reports automatically, that the tab crashed page is not > + * shown. > + */ > +add_task(function* test_background_crash_simple() { Bah, just realized I need to rename this function. ::: browser/components/sessionstore/test/browser_background_tab_crash.js:181 (Diff revision 2) > + * and we visit 1, 1 should go to the tab crashed page. If we then have > + * two new background tabs (3, 4) crash, visiting B should still restore. > + * Visiting 4 should show us the tab crashed page, and then visiting 3 > + * should restore. > + */ > +add_task(function* test_background_crash_simple() { And this one.
Comment on attachment 8797739 [details] Bug 1241459 - Add regression tests for background tab crashing behaviour. https://reviewboard.mozilla.org/r/83374/#review82550 ::: browser/components/sessionstore/test/browser_background_tab_crash.js:177 (Diff revision 2) > + * Tests that if there are two background tab crashes in a row, that > + * the two sets of background crashes don't interfere with one another. > + * > + * Specifically, if we start with two background tabs (1, 2) which crash, > + * and we visit 1, 1 should go to the tab crashed page. If we then have > + * two new background tabs (3, 4) crash, visiting B should still restore. Gah, and here, I need to swap out "B" for "2".
Comment on attachment 8797739 [details] Bug 1241459 - Add regression tests for background tab crashing behaviour. https://reviewboard.mozilla.org/r/83374/#review82552 ::: browser/components/sessionstore/test/browser_background_tab_crash.js:222 (Diff revision 2) > + yield BrowserTestUtils.switchTab(gBrowser, tab3); > + yield tabRestored; > + }); > + }); > + > + yield SpecialPowers.popPrefEnv(); And need to remove this.
Comment on attachment 8796688 [details] Bug 1241459 - Background tab crashes should only show about:tabcrashed for first selected tab. https://reviewboard.mozilla.org/r/82446/#review83266
Attachment #8796688 - Flags: review?(felipc) → review+
Comment on attachment 8797739 [details] Bug 1241459 - Add regression tests for background tab crashing behaviour. https://reviewboard.mozilla.org/r/83374/#review83268
Attachment #8797739 - Flags: review?(felipc) → review+
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/01c97f84d227 Background tab crashes should only show about:tabcrashed for first selected tab. r=Felipe,mikedeboer https://hg.mozilla.org/integration/autoland/rev/814c9082ada8 Add regression tests for background tab crashing behaviour. r=Felipe
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Depends on: 1337351
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: