Closed Bug 1080055 Opened 6 years ago Closed 6 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)

x86_64
Linux
defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.1
Tracking Status
e10s m3+ ---

People

(Reporter: catlee, Assigned: mossop)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ 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.
Do you have a specific example that demonstrates this? I just tried on reddit and every link I clicked correctly opened a new tab.
tracking-e10s: --- → ?
Component: General → Tabbed Browser
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.
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?
And today it's working just fine :\
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
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 → ---
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
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.
Blocks: 961867, 999239
Status: REOPENED → ASSIGNED
Iteration: --- → 35.3
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Attached patch patch (obsolete) — Splinter Review
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 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 {
Iteration: 35.3 → 36.1
m3 if we can get it in in the next week, knock this down to m4 if it bogs down.
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. ;)
(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 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+
Right, silly builds with the crashreporter disabled...
Pushed with a minor change reviewed by mconley over IRC to fix the ASAN bustage.

https://hg.mozilla.org/integration/fx-team/rev/ba0bb4f26680
Attached patch patchSplinter Review
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 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+
Attachment #8503459 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/01bc61be7ad2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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.