Closed
Bug 961360
Opened 10 years ago
Closed 10 years ago
[e10s] "Tab crashed" page's "Try again" button does not reload multiple tabs
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 32
People
(Reporter: cpeterson, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
1.44 KB,
patch
|
Felipe
:
feedback+
|
Details | Diff | Splinter Review |
3.31 KB,
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
After my e10s tabs crash (usually from bug 910962), clicking the "Try again" button on the "Tab crashed" page does not reload any tabs. This seems to happen when I have multiple tabs open because "Try again" has worked for me when I only had one "Tab crashed" page.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 1•10 years ago
|
||
So what you want is for "Try again" to reload every tab from a process that crashed? From what I remember this is what my original patch did, but was changed for some reason. And is there some other issue where pressing "try again" doesn't actually work? Just to describe what currently happens. Every time we crash we save the previously loaded URL in the DOM of the about:tabcrashed page. When you click that "try again" button we basically just go to that href in the current tab. Than there is the code that handles the crash report submission, which is also triggered by the same button. Right now we don't have any UI to ask the user about that submission. There is some small complexity, because multiple tabs map to the same content-process with one crash report. Iterating through every crashed tab's DOM to reload it, is maybe not that awesome, so we probably want to save the URLs somewhere on the browser element? Felipe you were involved in this last time from what I remember. Do you have any thoughts?
Flags: needinfo?(felipc)
Comment 2•10 years ago
|
||
The URL will already be stored in each browser as browser.currentURI (because displayLoadError doesn't change currentURI). So what we need to do is just iterate on tabbrowser through every tab that was remote and do something similar to what BrowserReloadWithFlags does to update the browser's remoteness again. The sucky part is doing this for every open window instead of just the current one. I can't think of a super clean way to do it besides sending an observer notification. So onAboutTabCrashed could be the function that sends the notification and each tabbrowser observes it.
Flags: needinfo?(felipc)
Reporter | ||
Comment 3•10 years ago
|
||
Would this problem still exist if (when?) each tab has its own sandbox process? If so, maybe we don't really need to implement this feature after all. :)
Status: NEW → ASSIGNED
I still think we should do this. It will be a while before we have multiple content processes, and even then they'll often be shared between tabs.
Reporter | ||
Updated•10 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Comment 5•10 years ago
|
||
Simplest approach that I could think of. Lightly tested.
Attachment #8424208 -
Flags: review?(felipc)
Assignee | ||
Comment 6•10 years ago
|
||
Sorry, wrong patch.
Attachment #8424208 -
Attachment is obsolete: true
Attachment #8424208 -
Flags: review?(felipc)
Attachment #8424217 -
Flags: review?(felipc)
Comment 7•10 years ago
|
||
Comment on attachment 8424217 [details] [diff] [review] v1 Review of attachment 8424217 [details] [diff] [review]: ----------------------------------------------------------------- To make it reload tabs from all windows, I think we can move this a method from TabCrashReporter, e.g. reloadCrashedTabs. That function could iterate through navigator:browser windows using Services.wm and from there do the same thing as done on this patch. Maybe check if it's a gMultiProcessBrowser window before. ::: browser/base/content/browser.js @@ +2416,5 @@ > + let doc = browser.contentDocument; > + if (!doc.documentURI.startsWith("about:tabcrashed")) > + continue; > + > + let url = doc.getElementById("tryAgain").getAttribute("url"); I think that browser.currentURI should have the correct URL, which leaves me wondering why we end up in this direction of setting the url in the button.. Maybe we didn't think of that before. Can you check and see if this can be simplified?
Attachment #8424217 -
Flags: review?(felipc) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8426499 [details] [diff] [review] v2 There is one weird bug where I don't really know what is going on: I open two e10s windows, with one tab each. Pressing "Try again" in the first window/tab works and reloads the tab in the other window as well. Pressing the button in the other window/tab does nothing.
Attachment #8426499 -
Attachment description: reload-all → v2
Attachment #8426499 -
Attachment is patch: true
Attachment #8426499 -
Flags: review?(felipc)
Updated•10 years ago
|
Attachment #8426499 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd1ea2dc580
https://hg.mozilla.org/mozilla-central/rev/5bd1ea2dc580
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
You need to log in
before you can comment on or make changes to this bug.
Description
•