Closed
Bug 1080055
Opened 10 years ago
Closed 10 years ago
[e10s] After a content process crash links to external sites from pinned tabs are opened in the same tab instead of opening a new tab
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
Tracking | Status | |
---|---|---|
e10s | m3+ | --- |
People
(Reporter: catlee, Assigned: mossop)
References
Details
Attachments
(1 file, 1 obsolete file)
10.10 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1059962 +++ I have reddit open in a pinned tab. In non-e10s if I click on a story that's not on reddit.com, the link is opened in a new tab. In e10s if I click on the same link, it's opened in my pinned tab instead. This was fixed in bug 961867, but seems to have regressed in the latest nightly.
Assignee | ||
Comment 1•10 years ago
|
||
Do you have a specific example that demonstrates this? I just tried on reddit and every link I clicked correctly opened a new tab.
Assignee | ||
Updated•10 years ago
|
tracking-e10s:
--- → ?
Component: General → Tabbed Browser
Reporter | ||
Comment 2•10 years ago
|
||
E.g. I have http://www.reddit.com/r/Homebrewing/ opened in a pinned tab. The top-most story right now is a link to imgur - http://i.imgur.com/jsL7IZT.jpg. When I click on the story link, the location of my pinned tab changes rather than opening a new tab.
Assignee | ||
Comment 3•10 years ago
|
||
Seems to work for me. Have you tried disabling your extensions? Are you running with e10s enabled globally or just by opening a new e10s window?
Reporter | ||
Comment 4•10 years ago
|
||
And today it's working just fine :\
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 5•10 years ago
|
||
huh, spoke too soon. many hours later in the same session it started happening again. in the meanwhile I've had tabs crash many times and have had to reload them, could that be related?
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee | ||
Comment 6•10 years ago
|
||
Bingo!
Assignee: nobody → dtownsend+bugmail
Summary: [e10s] in pinned tab, links to external sites are opened in the same tab instead of opening a new tab → [e10s] After a content process crash links to external sites from pinned tabs are opened in the same tab instead of opening a new tab
Assignee | ||
Comment 7•10 years ago
|
||
The problem here is that the docshell's isAppTab state isn't being set when we switch from remote to non-remote browsers and vice-versa.
Assignee | ||
Comment 8•10 years ago
|
||
I settled on sending the isAppTab state whenever we receive Browser:Init (for browsers in the child process) or just sending it directly for in-process browsers from updateBrowserRemoteness. Mike, you'll probably have to tweak how this test initiates the restore with your changes in bug 1065785, unless that lands first.
Attachment #8503459 -
Flags: review?(mconley)
Comment 9•10 years ago
|
||
Comment on attachment 8503459 [details] [diff] [review] patch >--- a/browser/base/content/tabbrowser.xml >+++ b/browser/base/content/tabbrowser.xml >@@ -1471,20 +1471,23 @@ > // Tearing down the browser gives a new permanentKey but we want to > // keep the old one. Re-set it explicitly after unbinding from DOM. > aBrowser.permanentKey = permanentKey; > parent.appendChild(aBrowser); > > // Restore the progress listener. > aBrowser.webProgress.addProgressListener(filter, Ci.nsIWebProgress.NOTIFY_ALL); > >- if (aShouldBeRemote) >+ if (aShouldBeRemote) { > tab.setAttribute("remote", "true"); >- else >+ } >+ else { prevailing style is } else {
Updated•10 years ago
|
Iteration: 35.3 → 36.1
Comment 10•10 years ago
|
||
m3 if we can get it in in the next week, knock this down to m4 if it bogs down.
Comment 11•10 years ago
|
||
Comment on attachment 8503459 [details] [diff] [review] patch Review of attachment 8503459 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +3090,5 @@ > this.selectedTab = tab; > window.focus(); > break; > } > + case "Browser:Init": { It looks like we're setting up the listener for Browser:Init iff the browser is remote. If that's the case, and we're only ever sending the Browser:AppTab message for remote browsers, wouldn't this make more sense in remote-browser.xml's pre-existing Browser:Init handler function? ::: browser/base/content/test/general/browser_restore_isAppTab.js @@ +2,5 @@ > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +const DUMMY = "http://example.com/browser/browser/base/content/test/general/dummy_page.html"; > + > +Components.utils.import("resource://gre/modules/Services.jsm"); I don't think you need to import this - I think it's just inherited by way of the test framework. @@ +5,5 @@ > + > +Components.utils.import("resource://gre/modules/Services.jsm"); > + > +function getMinidumpDirectory() { > + var dir = Services.dirsvc.get('ProfD', Ci.nsIFile); let, not var @@ +24,5 @@ > + info("This is a normal termination and isn't the one we are looking for..."); > + return; > + } > + > + var dumpID; let, not var @@ +31,5 @@ > + ok(dumpID, "dumpID is present and not an empty string"); > + } > + > + if (dumpID) { > + var minidumpDirectory = getMinidumpDirectory(); let, not var @@ +48,5 @@ > + Services.obs.removeObserver(CrashObserver, 'ipc:content-shutdown'); > +}); > + > +function frameScript() { > + addMessageListener("Test:GetIsAppTab", function({ data: uri }) { This argument isn't being used. @@ +53,5 @@ > + sendAsyncMessage("Test:IsAppTab", { isAppTab: docShell.isAppTab }); > + }); > + > + addMessageListener("Test:Crash", function() { > + dump("Crashing\n"); dump! @@ +80,5 @@ > + > +// Restarts the child process by crashing it then reloading the tab > +let restart = Task.async(function*(browser) { > + // If the tab isn't remote this would crash the main process so skip it > + if (browser.getAttribute("remote") != "true") browser.isRemoteBrowser @@ +86,5 @@ > + > + browser.messageManager.sendAsyncMessage("Test:Crash"); > + yield promiseWaitForEvent(browser, "AboutTabCrashedLoad", false, true); > + > + TabCrashReporter.reloadCrashedTab(browser); Thanks for bitrotting bug 1065785. ;)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11) > Comment on attachment 8503459 [details] [diff] [review] > patch > > Review of attachment 8503459 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/tabbrowser.xml > @@ +3090,5 @@ > > this.selectedTab = tab; > > window.focus(); > > break; > > } > > + case "Browser:Init": { > > It looks like we're setting up the listener for Browser:Init iff the browser > is remote. If that's the case, and we're only ever sending the > Browser:AppTab message for remote browsers, wouldn't this make more sense in > remote-browser.xml's pre-existing Browser:Init handler function? Unfortunately remote-browser.xml is in toolkit which can't depend on tabbrowser over in browser which holds the pinned state. > @@ +86,5 @@ > > + > > + browser.messageManager.sendAsyncMessage("Test:Crash"); > > + yield promiseWaitForEvent(browser, "AboutTabCrashedLoad", false, true); > > + > > + TabCrashReporter.reloadCrashedTab(browser); > > Thanks for bitrotting bug 1065785. ;) Anytime!
Comment 13•10 years ago
|
||
Comment on attachment 8503459 [details] [diff] [review] patch Review of attachment 8503459 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, you're absolutely right about remote-browser.xml. It's kinda sad how we have to duplicate ourselves so much to account for remote and non-remote browsers. :/ Beyond the nits I found in my last review, I think this looks good. Thanks Mossop!
Attachment #8503459 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8a4b1c63f433
This was failing on Linux ASAN, so I backed it out in https://hg.mozilla.org/integration/fx-team/rev/089a2b8ecd00 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=922924&repo=fx-team
Assignee | ||
Comment 16•10 years ago
|
||
Right, silly builds with the crashreporter disabled...
Assignee | ||
Comment 17•10 years ago
|
||
Pushed with a minor change reviewed by mconley over IRC to fix the ASAN bustage. https://hg.mozilla.org/integration/fx-team/rev/ba0bb4f26680
And it's still busted on ASAN bc1, so backed out again in https://hg.mozilla.org/integration/fx-team/rev/92c87e95915e https://treeherder.mozilla.org/ui/logviewer.html#?job_id=934736&repo=fx-team
Flags: needinfo?(dtownsend+bugmail)
Assignee | ||
Comment 19•10 years ago
|
||
Fixes the ASAN bustage for sure this time (did a try run and everything!) and fixes the bitrot from bug 1065785.
Flags: needinfo?(dtownsend+bugmail)
Attachment #8508320 -
Flags: review?(mconley)
Comment 20•10 years ago
|
||
Comment on attachment 8508320 [details] [diff] [review] patch Review of attachment 8508320 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks Mossop! ::: browser/base/content/test/general/browser_restore_isAppTab.js @@ +89,5 @@ > + > + let tab = gBrowser.getTabForBrowser(browser); > + SessionStore.reviveCrashedTab(tab); > + > + yield promiseTabLoaded(gBrowser.getTabForBrowser(browser)); Isn't this just "tab" again?
Attachment #8508320 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Third times the charm? https://hg.mozilla.org/integration/fx-team/rev/01bc61be7ad2
Assignee | ||
Updated•10 years ago
|
Attachment #8503459 -
Attachment is obsolete: true
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/01bc61be7ad2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment 23•10 years ago
|
||
Reproduced the initial issue on a Nightly build before the fix, verified on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.9.5 using latest Nightly.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•