1373055 - When background tabs crash, unload them instead of flipping their remoteness and sending them to about:blank

Status: REOPENED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

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

When background tabs crash, we currently flip their remoteness to false, and send them to about:blank.

Now that we have lazy-tabs abilities, we should unload these background tabs instead. That should be strictly cheaper.

Once bug 1284886 lands, this should be pretty straight-forward.

Comment 1

[Lorena Diaconescu]

I would like to work on this bug. Could you please point me in the right direction to start and assign me to this bug? Thanks!

Comment 2

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

Hi Lorena! Thanks for getting in touch! :)

Sure - I'll assign you to the bug now. Thanks!

Do you have a build of Firefox all ready to go?

Comment 3

[Lorena Diaconescu]

I have the Firefox build set up. Could you tell me what files I should look at for this bug? 

Thank you!

Comment 4

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

For sure.

We're going to be working with the code that handles tabs crashing. That's kind of spread out in a few places, but here are the important pieces:

browser/base/content/tabbrowser.xml - lines 6021-6048:  http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/base/content/tabbrowser.xml#6021-6048

  This event handler is defined in each browser window, and handles the oop-browser-crashed event,
  which is fired for each browser tab that crashes.

  Specifically, this line: http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/base/content/tabbrowser.xml#6041

  This is "flipping the remoteness" of the browser, which is an expensive operation, and what I'd rather we do instead
  is unload the tab.

  So the first step here is to replace that updateBrowserRemoteness bit to be more like:

  this.discardBrowser(browser);


The next part is inside that reviveCrashedTab bit that the handler is calling. That's in our SessionStore code here:

browser/components/sessionstore/SessionStore.jsm - lines 2834-2870:  http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/components/sessionstore/SessionStore.jsm#2834-2870

  We need to modify this function to deal with the fact that the browser is now "discarded", and
  hasn't had its remoteness flipped.

  That means getting rid of this check: http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/components/sessionstore/SessionStore.jsm#2852-2857

  Instead, we should probably ensure that there's no linked browser for the browser that we're reviving. We check that
  with:

  browser.linkedPanel

  ^-- if that's false-y, then there's no linked browser, and that's good. So I'd modify that conditional to throw
  if browser.linkedPanel is defined, like:

  // Modify this sanity check comment to reflect this change
  if (browser.linkedPanel) {
    // throw in here
  }

  Finally, I think we need to remove this line: http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/components/sessionstore/SessionStore.jsm#2864

  because loading a URI causes the browser to be inserted, and we want to avoid that.


I _think_ those are all of the required functional changes. There might be a few that I've missed, but these are the ones I can see right off the bat.

It's possible you'll need to modify some tests too, but we'll get there.

Once you've written that, we can try testing the fix manually to see how Firefox behaves with tabs crashing. That will mean that we need to intentionally crash some content processes, and techniques for doing that vary from OS to OS. Which OS do you have?

Let me know if you have any questions about anything I just wrote! Happy to answer anything.

Comment 5

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

Hey Lorena, just checking in - let me know if you have any questions about any of this stuff. Also feel free to e-mail me at mconley at mozilla dot com. You can also drop into the #fx-team IRC channel if you'd like to chat in real-time. A list of IRC clients and instructions on how to connect to irc.mozilla.org are here: https://wiki.mozilla.org/IRC

I'm mconley in IRC.

Comment 6

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

Any updates here, Lorena? Anything I can help with?

Comment 7

[Arjun Krishna Babu]

This is something I'm interested in. Is Lorena Diaconescu still working on this?

Also, can somebody tell me the method to make tabs crash? I'm on Debian OS. 

Thanks.

Comment 8

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

Hi Arjun,

I haven't heard from Lorena in some time, so I presume they're not working on it.

I'll assign you.

The best way to crash tabs on Debian is probably to send a SIGABRT to the content process.

So choose a tab to crash, and hover the tab title with your mouse until the tooltip comes up. The process ID for its content process should appear after the page title in the tooltip.

Then, in your terminal:

kill -SIGABRT [process ID]

Should kill the process and cause it to crash.

Does that help?

Comment 9

[Arjun Krishna Babu]

Yes, that helps.

Here's what I've done so far:

I have NOT made changes to any code. 

Opened two tabs, visited two different URLS.
Made one of the URLS crash as per the instructions on comment #8.

The tab then says "Gah. Your tab just crashed.", which I suppose is the expected behavior. 

So, what does it really mean when you say "flip their remoteness"? I understand based on comments above that it is an expensive procedure. 

So what exactly do you want me to do? Follow the instructions in comment #4?

Comment 10

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

Hey Arjun,

So far so good!

The case we're trying to deal with is when background tabs crash. Up until now, what we've done is replaced those crashed background tabs with empty about:blank tabs that are hosted in the parent process until the user selects it. When the user selects that background tab, we might show about:tabcrashed or we might just restore the tab - it depends on whether or not the user is configured to automatically send crash reports or not.

At any rate, it's that first about:blank load that we want to avoid. Since writing that code, a new mechanism for lazily creating browsers has been introduced, and we should use that instead. Essentially, it allows us to "discard" crashed background browser tabs until they're selected, rather than creating new ones and parking them at about:blank.

I hope that kinda clears up what we're trying to do here.

Reading over comment 4 again, those instructions still sound sane, so yes, I'd try to follow them. The code they refer to is several months out of date, so some things might have shifted around, but I can try to point you in the right direction if things have moved in ways that are confusing.

Let me know if you have any questions on how to proceed. Thanks!

Comment 11

[Arjun Krishna Babu]

I made the changes you mentioned in comment 4, rebuilt, and ran the browser.

I opened two URLs in two table, and made one of them crash (as per instructions in comment 8).

The behavior after crash is different. I'll try my best to explain without confusing you. 

1. If I'm on the tab when it crashes, the "Gah. Your tab just crashed." page appears (which I assume is "about:tabcrashed" page). From there, I can go to a new URL, close that tab etc. 

2. If I'm on a different tab when *another* tab crashes, and I click the crashed tab, it shows a blank white page. However, the URL shown on the crashed tab would then be equal to whichever tab I was on when the crash happened. 
If I then switch to another tab, and back to the crashed tab, the URL would be equal to whatever URL it was before the crash happened.

I'm unable to visit a new URL from the crashed tab; it just won't happen. 
I'm able to close the crashed tab.

3. If I'm on a different tab when *another* tab crashes, and the crashed tab had the YouTube homepage on it ("https://www.youtube.com/"), then the behavior is strange:
- If I type in a new URL in the crashed tab and try to go there, the new URL opens in the tab I was previously on (ie., the tab I was on before I switched over to the crashed tab).
- If I close the crashed tab, the tab seems to go into the "background", as though you aren't on that tab. But the crashed tab (which I closed, remember) is still visible. And I'm unable to access the tab; clicking it won't switch me over there. I press the tab's close button and nothing happens. 
All of this behavior only happens when the crashed tab was displaying YouTube just before crash.

That was probably confusing. 

By the way, should I remove this line: https://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2894

Comment 12

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

(In reply to Arjun Krishna Babu from comment #11)
> 
> That was probably confusing. 
> 

I think I followed. :)

Can you show me your patch so far? It doesn't have to be done, but I want to see what you're working with, since that might help me explain what's going on.

Comment 13

[Arjun Krishna Babu]

Attachment: bug1373055: The changes done so far

Basically just the diff output; not sure if this is the proper procedure to generate the patch (it was folks at IRC that advised me to do this).

Comment 14

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

Ah, I think I see what's going wrong: discardBrowser has this code at the top:

https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/base/content/tabbrowser.xml#2508-2515

and the last thing we check to see if we can discard is if the browser has a beforeunload handler set. That goes through this code in the out-of-process browser case:

https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/toolkit/content/widgets/remote-browser.xml#293-297

where we attempt to access the hasBeforeUnload property of the browser's tabParent.

However, when a browser crashes, the tabParent is destroyed, so the tabParent is null, and this is throwing. So discardBrowser is failing, and so the browser gets into a really funky state - the one you describe in comment 11 (which I can easily reproduce).

So I think the solution here is to have the out-of-process browser's permitUnload include the case where the tabParent has gone away:

https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/toolkit/content/widgets/remote-browser.xml#295-297

^-- so in there, I think it should be something like:

if (!tabParent || !tabParent.hasBeforeUnload) {
  // ...
}

and then I think that'll get us what we want.

Comment 15

[Arjun Krishna Babu]

I made the change you recommended, and now things seem to be working good! 

All of that funky behavior of comment 11 is gone.

So, what do I do now? (I'm new to Mozilla, mercurial etc).

Comment 16

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

(In reply to Arjun Krishna Babu from comment #15)
> I made the change you recommended, and now things seem to be working good! 
> 
> All of that funky behavior of comment 11 is gone.
> 

Great!

> So, what do I do now? (I'm new to Mozilla, mercurial etc).

First off, welcome aboard. :)

Second off, you have two choices:

1) You can use Mercurial and MozReview to post the patch. There is some set-up required in order to do this - http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html . It's a little intense, but it's a one-time cost. I can walk you through it wherever you get stuck.

2) You can use hg diff to produce a diff like you did before, and attach the file to this bug. Then we use Bugzilla to review that patch. That's probably the simpler approach in this case - though it means you don't get an opportunity to flex / learn some Mercurial skills.

I can review in either case. Up to you - let me know what you need.

Comment 17

[Arjun Krishna Babu]

I'm here to learn! I'll go with the first option!

Comment 18

[Arjun Krishna Babu]

Attachment: Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank;

When background tabs crash, they were being replaced by about:blank tab
hosted in parent process.

Now that there is a mechanism for lazily creating browsers, discarding the
crashed background browser until they are selected is a cheaper operation
(compared to creating about:blank).

Procedure:
1. Stop flipping remoteness of the browser, and instead discard the tab.
2. Ensure there is no linked browser (we discarded it).
3. Stop loading about:blank in reviveCrashedTab() function.
4. Have out-of-process browser's permitUnload include the case where the
tabParent has gone away.

Review commit: https://reviewboard.mozilla.org/r/206820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206820/

Comment 19

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

Thanks! I'm traveling at the moment, but will have a review ready for you early next week.

Comment 20

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

Comment on attachment 8935967 [details]
Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank;

https://reviewboard.mozilla.org/r/206820/#review214202

Thanks so much! This looks great. I've pushed your patch to try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2867000e77941422a42a5a2818432757197b592e

While we're waiting for that to come back, it might be worth modifying a few tests to check for this. There's a pre-existing test that we can probably use:

browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js

Right after these lines: https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js#38-39

Can you do a check that there's no linkedPanel on the crashed tab? Something like, `ok(!newTab2.linkedPanel, "Background tab should have been discarded");`

::: commit-message-8c84a:10
(Diff revision 1)
> +Procedure:
> +1. Stop flipping remoteness of the browser, and instead discard the tab.
> +2. Ensure there is no linked browser (we discarded it).
> +3. Stop loading about:blank in reviveCrashedTab() function.
> +4. Have out-of-process browser's permitUnload include the case where the
> +tabParent has gone away.

This commit message is great - but I suspect we can drop this part.

Comment 21

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

Hey Arjun,

So the try push indicates a few failing tests - specifically, the following tests are failing:

browser/components/sessionstore/test/browser_background_tab_crash.js
browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js

You can run those locally via:

./mach mochitest <path to test>

For example:

./mach mochitest browser/components/sessionstore/test/browser_background_tab_crash.js

You can also run the tests and launch the JS debugger to try to walk through where things are going wrong:

./mach mochitest browser/components/sessionstore/test/browser_background_tab_crash.js --jsdebugger

So we need to figure out why these tests are failing. Is something actually broken, or does the test itself just need its expectations updated? That's what we need to sort out.

Let me know if you need further guidance on how to proceed, and I'll give you everything I can.

Comment 22

[Arjun Krishna Babu]

I'm kinda stuck.

In browser_background_tab_crash.js, line 99 is probably incorrect under the new circumstances because we stopped explicitly setting the remoteness of the browser to false (didn't we?). https://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_background_tab_crash.js#99

However, line 99 is probably not the (only) thing that makes the tests fail; in fact, it doesn't make the tests fail anyway. Apparently, the failures are caused by some sort of timeout which in turn seem to be caused by the await operators; there are 3 of them right before line 99: https://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_background_tab_crash.js#92-94

The tests in browser_revive_crashed_bg_tabs.js also experiences the aforementioned timeouts. Curious to know what would happen, I commented out the await operator in line 42 of this file. https://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js#42 

Surprisingly, all tests passed on the subsequent run. Of course, I do realize that's probably not the correct way to do things.

Speaking of which, keep in mind I do not have a clear idea of how await, promises etc work. In fact, reading through these source codes made me realize how little JavaScript I know (I'm fairly adept at figuring things out from the documentation, however).

Also, consecutive runs of the same tests (without any changes to the source code) occasionally produce different results. It seems to be dependent on whether or not I click the Nightly that pops up when the tests are being run. However, I'm unable to reproduce this behavior.

I'm pretty-much stuck at this point. Help.

Comment 23

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

Thanks Arjun. I'll be attempting to sort through what you're hitting today during The Joy of Coding, which airs in about an hour if you want to follow along!

https://www.reddit.com/r/WatchPeopleCode/comments/7l28xv/live_weekly_1pm_et_on_wednesdays_watch_a_mozilla/

Comment 24

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

Hey Arjun,

So I spent some time looking at this today, and there's a little bit more work to do which I hadn't realized when I was first speccing out this bug.

It turns out my understanding of how discarding browsers works was slightly flawed - I didn't realize that discarding a browser also puts it into this state where the tab will be restored as soon as the browser element is accessed and inserted into the browser window DOM.

Our plan can still work here, but it's going to require the following changes:

1) After we discardBrowser inside the oop-browser-crashed handler, _do not_ call SessionStore.reviveCrashedTab. There's no point in doing that - discardBrowser is doing all of the heavy lifting for us by putting the browser tab into this super-special state where it's ready to reload as soon as its selected.

2) We had code that detected when you selected a crashed background tab, and chose whether or not to send you to the about:tabcrashed page. That's here:

https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/SessionStore.jsm#2123-2125

With the new discarding mechanism, this code will run too late when we select a background tab that had crashed, and we'll end up starting to restore the tab and _then_ sending it to about:tabcrashed, which is a bit of a waste. What would probably be better is if we determine whether or not the send the tab to about:tabcrashed during that restoration point.

We should modify SessionStore.onTabBrowserInserted in the following way:

Inside this block: https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/SessionStore.jsm#1896-1899

Before choosing to restore the tab, we should see whether or not we should be showing about:tabcrashed instead. So perhaps write in something like:

let selectedBrowser = aWindow.gBrowser.selectedBrowser;
if (browser === selectedBrowser && TabCrashHandler.willShowCrashedTab(browser)) {
  this.enterCrashedState(browser);
} else {
  // .. restore the tab like we were before
}

What the above will do is check to see if when inserting one of those lazy browsers, it's the currently selected browser. If so, it'll ask the TabCrashHandler stuff whether or not it should show about:tabcrashed, and if so, it'll send it there.

This also means we can get rid of this code here:

https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/SessionStore.jsm#2114-2125

and we should just restoreTabContent.

3) Get rid of the linkedPanel check inside of reviveCrashedTab. I thought that'd be a good idea at the time, but it turns out there are some cases where we'll enter reviveCrashedTab when we _do_ have a linkedPanel. For example, when we click on "Restore Tab" inside a crashed tab. This will revive the tab and restore the state, and the linkedPanel will exist throughtout.

4) We have to modify the two failing tests to account for the fact that background tabs aren't going to flip remoteness nor restore their state automatically when they crash.

I'm going to break this down by test.

# browser_background_tab_crash.js

We have to start by removing these two awaits here:

https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/test/browser_background_tab_crash.js#93-94

and their definitions here: https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/test/browser_background_tab_crash.js#84-90

because we're no longer flipping remoteness nor restoring those tabs on crash anymore.

We should also probably add a check that the linkedPanel doesn't exist as the first check in this loop:

https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/test/browser_background_tab_crash.js#98-102

something like:

Assert.ok(!tab.linkedPanel, "The tab should be initially discarded.");

right at the top of those Asserts is fine.

We should also ensure that the tab browser IS STILL remote, instead of no longer being remote (since we're not doing the remoteness flipping anymore), so just flip the sign on this check: https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/test/browser_background_tab_crash.js#99

We also have to make sure that we listen for AboutTabCrashedReady event (which fires when about:tabcrashed is loaded) at the right time:

https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/test/browser_background_tab_crash.js#128-131

^-- this is doing it too soon now, because about:tabcrashed will load as soon as we attempt to access the linkedBrowser at the point where we're trying to add the event handler! Luckily, AboutTabCrashedReady is an event that bubbles, so instead of waiting for the event on the browser, wait for it on the window, like this:

      BrowserTestUtils.waitForEvent(window,
                                    "AboutTabCrashedReady",
                                    false, null, true);

# browser_revive_crashed_bg_tabs.js

For browser_revive_crashed_bg_tabs.js, we don't need to wait for the SSWindowStateReady event anymore because the background tabs shouldn't restore automatically, so I think you can get rid of this: https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js#35 and where it awaits for windowReady.



I think if you can address the above, the tests will pass.

I know this was a lot. I may have accidentally forgotten to explain something, or omitted some detail and this will cause you confusion. Please let me know if there's anything unclear here, and I'll attempt to explain and clear things up!

Thanks Arjun, and sorry there are some extra hoops here.

Comment 25

[Arjun Krishna Babu]

Attachment: bug1373055-diff.patch

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #24)

I made the changes.

Tests at browser_revive_crashed_bg_tabs.js passes.

Tests at browser_background_tab_crash.js still fails.

It's probably because I screwed up somwhere, but I can't seem to figure out where. 

I've attached the hg diff output.

> so instead of waiting for the event on the browser, wait for
> it on the window, like this:
> 
>       BrowserTestUtils.waitForEvent(window,
>                                     "AboutTabCrashedReady",
>                                     false, null, true);

Another question: are you sure it's "window"?

Comment 26

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

Comment on attachment 8938587 [details] [diff] [review]
bug1373055-diff.patch

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

Hey Arjun,

Sorry again for the delay - winter holidays got in the way. I should be more responsive going forward now.

I think I see why the test is still failing. See below. Thanks!

::: browser/components/sessionstore/SessionStore.jsm
@@ +1890,4 @@
>  
>      // Only restore if browser has been lazy.
>      if (aTab.__SS_lazyData && !browser.__SS_restoreState && TabStateCache.get(browser)) {
> +      let selectedBrowser = aWindow.gBrowser.selectedBrowser;

I think I was wrong about this comparison, and I believe this is partially why the test is failing. At insertion time, the switch hasn't actually occurred yet, so this condition should be impossible.

So I think we should just drop the browser === selectedBrowser comparison. Sorry about misleading you there!

::: browser/components/sessionstore/test/browser_background_tab_crash.js
@@ +117,4 @@
>  
>      // Selecting the first tab should now send it to the tab crashed page.
>      let tabCrashedPagePromise =
> +      BrowserTestUtils.waitForContentEvent(window,

Yeah, we want to target "window" here, but we want to use BrowserTestUtils.waitForEvent instead.

Comment 27

[Arjun Krishna Babu]

Comment on attachment 8935967 [details]
Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206820/diff/1-2/

Comment 28

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

Comment on attachment 8935967 [details]
Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank;

https://reviewboard.mozilla.org/r/206820/#review216200

This looks great, thanks Arjun! Two very minor nits, and I think (assuming a green try run), I think we can get this landed.

::: browser/components/sessionstore/SessionStore.jsm:1893
(Diff revision 2)
>        this._lastKnownFrameLoader.set(browser.permanentKey, browser.frameLoader);
>      }
>  
>      // Only restore if browser has been lazy.
>      if (aTab.__SS_lazyData && !browser.__SS_restoreState && TabStateCache.get(browser)) {
> +      // let selectedBrowser = aWindow.gBrowser.selectedBrowser;

Please remove this dead code.

::: browser/components/sessionstore/test/browser_background_tab_crash.js:120
(Diff revision 2)
> -      BrowserTestUtils.waitForContentEvent(tab1.linkedBrowser,
> +      BrowserTestUtils.waitForEvent(window,
>                                             "AboutTabCrashedReady",
>                                             false, null, true);

Just need to adjust the alignment here.

Comment 29

[Arjun Krishna Babu]

Comment on attachment 8935967 [details]
Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206820/diff/2-3/

Comment 30

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

Thanks Arjun! I've pushed your patch to try to run the tests in automation.

Also, for future reference, you should mark the "Open issues" in MozReview as "Fixed" as you fix them. It's really a checklist for you to keep track of what you've addressed - but open issues also prevent the patch from landing.

I've taken the liberty of marking those issues as "Fixed" for you.

Here's the try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ec2d3fc9d0a34529da1617ce68333e8715c77f8

Comment 31

[Arjun Krishna Babu]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #30)

> Also, for future reference, you should mark the "Open issues" in MozReview
> as "Fixed" as you fix them. It's really a checklist for you to keep track of
> what you've addressed - but open issues also prevent the patch from landing.

Aw! I didn't know!

 
> I've taken the liberty of marking those issues as "Fixed" for you.

Thank you :)

Comment 32

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

(In reply to Arjun Krishna Babu from comment #31)
> 
> Aw! I didn't know!
> 

No worries - these things are pretty non-obvious if you've never done then before.

> Thank you :)

No problem! Thanks for the patch!

Comment 33

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

Hm, so that last try push has a new test failure in it:

browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js | uncaught exception
  browser.frameLoader is null at setTabState@chrome://browser/content/tabbrowser.xml:4384:19
  unloadNonRequiredTabs@chrome://browser/content/tabbrowser.xml:4803:19
  onUnloadTimeout@chrome://browser/content/tabbrowser.xml:4779:15
  suppressDisplayPortAndQueueUnload/this.unloadTimer<@chrome://browser/content/tabbrowser.xml:5073:54

I think this is because the asynchronous tab switcher is expecting crashed browsers to flip remoteness, and they're not doing it anymore. We're going to have to teach it to deal with the new world of unloaded crashed tabs instead.

Setting a needinfo? on myself to lay out some instructions on how to do that once I've had a chance to investigate further.

Comment 34

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

Okay, I think I know how to proceed here.

Arjun, there's one last piece of the puzzle - the infrastructure that does the switching of tabs needs to be aware that crashed tabs don't flip remoteness anymore - they become discarded.

The code in question is a rather complex piece of machinery, but the actual work should be a pretty straight-forward operation.

The async tab switcher can be found here: https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/browser/base/content/tabbrowser.xml#4312

(See the comment block above it if you're curious about how it works.)

Here's what you'll need to do:

1) Add an event handler for the TabBrowserDiscarded event, which gets dispatched through the <xul:tab> that gets discarded on crashing. You should do that here: https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/browser/base/content/tabbrowser.xml#4477

2) Remove the event handler when the async tab switcher shuts down here: https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/browser/base/content/tabbrowser.xml#4521

3) Here, in the generic event handler, add a case for the TabBrowserDiscarded event: https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/browser/base/content/tabbrowser.xml#5196 . When that case occurs, have it call a new function called something like onTabDiscarded. Pass the event.target to that function - the event target will be the <xul:tab>.

4) Add the new function onTabDiscarded, taking the single tab argument. Above here seems like a logical place: https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/browser/base/content/tabbrowser.xml#4966

5) Inside that function, have it log state, and set the tab's state to STATE_UNLOADED without doing further actions, something like this:

this.logState(`onTabDiscarded(${tab._tPos})`);
this.setTabStateNoAction(tab, this.STATE_UNLOADED);

and that should be all that's necessary to get the test to pass.

Here's an explanation of what's going on:

The async tab switcher's main responsibility is to tell background tabs to render and upload layers to the compositor to be viewed by the user, and when they're ready, to flip the <xul:deck> to display those layers. Due to the nature of multi-process Firefox, this entire operation is asynchronous. The async tab switcher manages and monitors the layers-rendering state for each tab. It roughly maps like this:

STATE_UNLOADED:  No layers are being rendered.
STATE_LOADING: Layers have been requested but haven't been received by the compositor yet
STATE_LOADED: Layers have been received by the compositor and the tab can be shown
STATE_UNLOADING: We've asked the compositor to drop the layers because the user has tabbed away, and we're waiting to hear from the compositor that this has occurred.

The test is failing because, presumably, some tab was in the STATE_LOADED state before it crashed, and the "cleanup" function that moves tabs into the STATE_UNLOADING state wasn't prepared to find a discarded browser for that tab when calling setTabState.

So what your change will do is notice when tabs are discarded, and immediately move them to STATE_UNLOADED without any further action needed.

Comment 35

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

Comment on attachment 8935967 [details]
Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank;

https://reviewboard.mozilla.org/r/206820/#review217180

All of this looks great, but we'll need that change to tabbrowser.xml to fix the remaining test failure before the patch can be merged. Let me know if you have any questions, thanks!

Comment 36

[Arjun Krishna Babu]

Hi. I'm extremely sorry for my delayed response. I've been traveling. 

I'll try to get things done over the weekend :)

Comment 37

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

(In reply to Arjun Krishna Babu from comment #36)
> Hi. I'm extremely sorry for my delayed response. I've been traveling. 
> 
> I'll try to get things done over the weekend :)

No apology necessary. :) But thank you!

Comment 38

[Arjun Krishna Babu]

Attachment: bug1373055-tabbrowser.patch

I think I followed.

Can you take a look at the diff output I have attached, and let me know if it looks good?

Comment 39

[Arjun Krishna Babu]

Attachment: bug1373055-tabbrowser.patch

Oops; the previous attachment had a minor mistake. 

Could you take a look at this new one?

Comment 40

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad69b8b2f68d784f5e59a7aac6176b27147f9bb4

Comment 41

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

(In reply to Arjun Krishna Babu from comment #39)
> Created attachment 8942587 [details] [diff] [review]
> bug1373055-tabbrowser.patch
> 
> Oops; the previous attachment had a minor mistake. 
> 
> Could you take a look at this new one?

Thanks Arjun! This looks good - I've pushed it to try for testing (comment 40).

Comment 42

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

Hey Arjun, the try push looks good to me! Would you mind folding your changes in, and I'll go ahead and r+ this!

Comment 43

[Arjun Krishna Babu]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #42)
> Hey Arjun, the try push looks good to me! Would you mind folding your
> changes in, and I'll go ahead and r+ this!

The try push looks good? I see a failing test from the link you gave in comment 40. Am I missing something here?

And I've got a couple more questions:

1. Wasn't I supposed to make a commit and put a mozreview request?
2. What exactly does "folding your changes in" mean?

Comment 44

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

(In reply to Arjun Krishna Babu from comment #43)
> The try push looks good? I see a failing test from the link you gave in
> comment 40. Am I missing something here?

Yes, a test failed, but unfortunately we sometimes get what are called "intermittent failures", where a test will fail for unrelated reasons non-deterministically. These are separate bugs that are not usually related to your work (unless your patches tend to make an intermittent orange become more frequent, which doesn't appear to be the case here).

If you click on the test failure job in Treeherder, you'll see it's:

"browser/base/content/test/urlbar/browser_urlbarRaceWithTabs.js | New tab still has about:newtab"

and below that, there's an associated intermittent orange bug filed for it.

I also noticed this failure, so I retriggered the bc7 job for you just to be sure. If you click on the +17 next to the failure, you'll see all of the jobs unfold, including the bc7's that were green after my retriggers.

So I think that test failure is unrelated, and we can safely ignore it.

> 
> And I've got a couple more questions:
> 
> 1. Wasn't I supposed to make a commit and put a mozreview request?

Yes please! And that's related to (2) - basically, I'm hoping you can apply the changes you made in attachment 8942587 [details] [diff] [review] on top of the MozReview patch.

You can do that via histedit: https://www.mercurial-scm.org/wiki/HisteditExtension

Enable histedit by adding this to the .hg/hgrc file under mozilla-central:

[extensions]
histedit =

Then, commit your attachment 8942587 [details] [diff] [review] as a new commit on top of your old commit. Then do:

hg histedit -r <SHA of the original commit>

this will bring up an editor (hopefully one you're comfortable with!) that will let you merge the changes in the two commits into a single commit.

In the newer commit, replace the "pick" next to the SHA, with the word "roll". This will merge the two commits without changing the commit message of the first message. (Using "fold" would merge the commit messages, which we don't actually want to do).

Then you should be able to push that up to MozReview as you did before, and your changes should be reflected there.

It's a bit complex, I realize, and I apologize. :/ Let me know if you have any questions. If this is too bothersome, I can always merge them for you and land them.

Comment 45

[Arjun Krishna Babu]

Comment on attachment 8935967 [details]
Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206820/diff/3-4/

Comment 46

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

Comment on attachment 8935967 [details]
Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank;

https://reviewboard.mozilla.org/r/206820/#review218646

This looks great to me, thanks!

I'm going to wait until after the next uplift to land this. I'll leave the needinfo on myself. Thanks for your work, Arjun!

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5eba8dcac2bb
Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank; r=mconley

Comment 48

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5eba8dcac2bb

Comment 49

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/94057f351ba9
Backed out changeset 5eba8dcac2bb for causing bug 1430466

Comment 50

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out changeset for causing browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js to frequently fail (bug 1430466). Please fix the issue and submit an updated patch. Thank you.

https://hg.mozilla.org/mozilla-central/rev/94057f351ba94b31e1b5a56fefdfe9d8e43d0e5d

See bug 1430466.

Push to first failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=windows7%20debug%20M-e10s%28bc1%29&tochange=f13e81b2bad0146a126275742a44a22f55f7ead9&fromchange=5eba8dcac2bb944425c4088f29b497a2b95e93a2
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=157756325&repo=autoland

17:50:15     INFO -  711 INFO TEST-START | browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js
17:50:15     INFO -  GECKO(5572) | ++DOCSHELL 0E202800 == 10 [pid = 5876] [id = {d6095765-de63-417e-b118-c45853ce0c8e}]
17:50:15     INFO -  GECKO(5572) | ++DOMWINDOW == 52 (0D6C3820) [pid = 5876] [serial = 245] [outer = 00000000]
17:50:15     INFO -  GECKO(5572) | ++DOMWINDOW == 53 (0E24B400) [pid = 5876] [serial = 246] [outer = 0D6C3820]
17:50:15     INFO -  GECKO(5572) | --DOMWINDOW == 8 (064923A0) [pid = 5276] [serial = 288] [outer = 00000000] [url = about:blank]
17:50:15     INFO -  GECKO(5572) | ++DOMWINDOW == 54 (0E247400) [pid = 5876] [serial = 247] [outer = 0D6C3820]
17:50:16     INFO -  GECKO(5572) | ++DOCSHELL 0E445400 == 11 [pid = 5876] [id = {fe44851d-7b31-4b08-b838-1ff3e7bd3e9c}]
17:50:16     INFO -  GECKO(5572) | ++DOMWINDOW == 55 (0E411EE0) [pid = 5876] [serial = 248] [outer = 00000000]
17:50:16     INFO -  GECKO(5572) | ++DOMWINDOW == 56 (0E449000) [pid = 5876] [serial = 249] [outer = 0E411EE0]
17:50:16     INFO -  GECKO(5572) | ++DOMWINDOW == 57 (0E446000) [pid = 5876] [serial = 250] [outer = 0E411EE0]
17:50:16     INFO -  GECKO(5572) | Et tu, Brute?
17:50:16     INFO -  GECKO(5572) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x15008F,name=PBrowser::Msg_UpdateNativeWindowHandle) Channel error: cannot send/recv
17:50:16     INFO -  GECKO(5572) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x150082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
17:50:16     INFO -  GECKO(5572) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x15008F,name=PBrowser::Msg_UpdateNativeWindowHandle) Channel error: cannot send/recv
17:50:16     INFO -  GECKO(5572) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x150082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
17:50:16     INFO -  GECKO(5572) | Assertion failure
17:50:16     INFO -  GECKO(5572) | assert@chrome://browser/content/tabbrowser.xml:4699:46
17:50:16     INFO -  GECKO(5572) | finish@chrome://browser/content/tabbrowser.xml:4550:15
17:50:16     INFO -  GECKO(5572) | postActions@chrome://browser/content/tabbrowser.xml:4854:17
17:50:16     INFO -  GECKO(5572) | handleEvent@chrome://browser/content/tabbrowser.xml:5226:15
17:50:16     INFO -  GECKO(5572) | discardBrowser@chrome://browser/content/tabbrowser.xml:2580:13
17:50:16     INFO -  GECKO(5572) | onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:6233:13
17:50:16     INFO -  TEST-INFO | started process screenshot
17:50:16     INFO -  TEST-INFO | screenshot: exit 0
17:50:16     INFO -  Buffered messages logged at 17:50:15
17:50:16     INFO -  712 INFO Entering test bound setup
17:50:16     INFO -  713 INFO Leaving test bound setup
17:50:16     INFO -  714 INFO Entering test bound test_revive_bg_tabs_on_demand
17:50:16     INFO -  715 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "data:text/html,<html><body>A%20regular,%20everyday,%20normal%20page." line: 0}]
17:50:16     INFO -  Buffered messages logged at 17:50:16
17:50:16     INFO -  716 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "data:text/html,<html><body>Another%20regular,%20everyday,%20normal%20page." line: 0}]
17:50:16     INFO -  Buffered messages finished
17:50:16    ERROR -  717 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_revive_crashed_bg_tabs.js | uncaught exception - Error: Assertion failure at assert@chrome://browser/content/tabbrowser.xml:4703:25
17:50:16     INFO -  finish@chrome://browser/content/tabbrowser.xml:4550:15
17:50:16     INFO -  postActions@chrome://browser/content/tabbrowser.xml:4854:17
17:50:16     INFO -  handleEvent@chrome://browser/content/tabbrowser.xml:5226:15
17:50:16     INFO -  discardBrowser@chrome://browser/content/tabbrowser.xml:2580:13
17:50:16     INFO -  onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:6233:13
17:50:16     INFO -  Stack trace:
17:50:16     INFO -  chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1645
17:50:16     INFO -  chrome://browser/content/tabbrowser.xml:discardBrowser:2580
17:50:16     INFO -  chrome://browser/content/tabbrowser.xml:onxbloop-browser-crashed:6233
17:50:16     INFO -  GECKO(5572) | JavaScript error: chrome://browser/content/tabbrowser.xml, line 4703: Error: Assertion failure
17:50:16     INFO -  GECKO(5572) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x15008F,name=PBrowser::Msg_UpdateNativeWindowHandle) Channel error: cannot send/recv
17:50:16     INFO -  GECKO(5572) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x150082,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
17:50:16     INFO -  GECKO(5572) | ++DOCSHELL 0E67E400 == 22 [pid = 5572] [id = {0c40eece-a1df-41d7-a27c-6179327424cb}]
17:50:16     INFO -  GECKO(5572) | ++DOMWINDOW == 96 (009F1700) [pid = 5572] [serial = 1320] [outer = 00000000]
17:50:16     INFO -  GECKO(5572) | ++DOMWINDOW == 97 (138DF800) [pid = 5572] [serial = 1321] [outer = 009F1700]
17:50:16     INFO -  718 INFO Console message: [JavaScript Error: "remote browser crashed while on data:text/html,<html><body>A%20regular,%20everyday,%20normal%20page.
17:50:16     INFO -  " {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 8}]
17:50:16     INFO -  e10s_init/<@chrome://mochikit/content/mochitest-e10s-utils.js:8:5
17:50:16     INFO -  EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@resource://gre/modules/RemoteAddonsParent.jsm:673:5
17:50:16     INFO -  interposeProperty/desc.value@jar:file:///Z:/task_1516642575/build/application/firefox/omni.ja!/components/multiprocessShims.js:157:52
17:50:16     INFO -  e10s_init@chrome://mochikit/content/mochitest-e10s-utils.js:6:3
17:50:16     INFO -  testInit@chrome://mochikit/content/browser-test.js:102:5
17:50:16     INFO -  setTimeout handler*@chrome://mochikit/content/browser-test.js:26:3
17:50:16     INFO -  719 INFO Console message: [JavaScript Error: "remote browser crashed while on data:text/html,<html><body>A%20regular,%20everyday,%20normal%20page.
17:50:16     INFO -  " {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 8}]
17:50:16     INFO -  e10s_init/<@chrome://mochikit/content/mochitest-e10s-utils.js:8:5
17:50:16     INFO -  EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@resource://gre/modules/RemoteAddonsParent.jsm:673:5
17:50:16     INFO -  interposeProperty/desc.value@jar:file:///Z:/task_1516642575/build/application/firefox/omni.ja!/components/multiprocessShims.js:157:52
17:50:16     INFO -  e10s_init@chrome://mochikit/content/mochitest-e10s-utils.js:6:3
17:50:16     INFO -  testInit@chrome://mochikit/content/browser-test.js:102:5
17:50:16     INFO -  setTimeout handler*@chrome://mochikit/content/browser-test.js:26:3
17:50:16     INFO -  720 INFO Console message: [JavaScript Error: "Error: Assertion failure" {file: "chrome://browser/content/tabbrowser.xml" line: 4703}]
17:50:16     INFO -  assert@chrome://browser/content/tabbrowser.xml:4703:25
17:50:16     INFO -  finish@chrome://browser/content/tabbrowser.xml:4550:15
17:50:16     INFO -  postActions@chrome://browser/content/tabbrowser.xml:4854:17
17:50:16     INFO -  handleEvent@chrome://browser/content/tabbrowser.xml:5226:15
17:50:16     INFO -  discardBrowser@chrome://browser/content/tabbrowser.xml:2580:13
17:50:16     INFO -  onxbloop-browser-crashed@chrome://browser/content/tabbrowser.xml:6233:13
17:50:16     INFO -  721 INFO Console message: [JavaScript Error: "remote browser crashed while on data:text/html,<html><body>A%20regular,%20everyday,%20normal%20page.
17:50:16     INFO -  " {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 8}]
17:50:16     INFO -  e10s_init/<@chrome://mochikit/content/mochitest-e10s-utils.js:8:5
17:50:16     INFO -  EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@resource://gre/modules/RemoteAddonsParent.jsm:673:5
17:50:16     INFO -  interposeProperty/desc.value@jar:file:///Z:/task_1516642575/build/application/firefox/omni.ja!/components/multiprocessShims.js:157:52
17:50:16     INFO -  e10s_init@chrome://mochikit/content/mochitest-e10s-utils.js:6:3
17:50:16     INFO -  testInit@chrome://mochikit/content/browser-test.js:102:5
17:50:16     INFO -  setTimeout handler*@chrome://mochikit/content/browser-test.js:26:3
17:50:16     INFO -  GECKO(5572) | ++DOMWINDOW == 98 (0F6EC400) [pid = 5572] [serial = 1322] [outer = 009F1700]

Comment 51

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

Okay, the issue here seems to be that sometimes in preActions, after a crashed browser has been discarded during a tab switch, we'll accidentally _recreate_ those discarded browsers.

We should make sure that the async tab switcher is not accidentally recreating these things.

From a quick search through the async tab switcher code, we use linkedBrowser a bunch, but in most of those cases, I think it's a safe assumption that the browser should exist (and it seems to make sense to create it if it doesn't).

There's one place that that's not true, and that's here:

https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/base/content/tabbrowser.xml#4744-4773

Before attempting to update its internal state, and the display of tabs, this function scans tabs for browsers that no longer exist, and makes sure we clear out state that's no longer valid. For discarded tabs, that'll accidentally re-create browsers that we shouldn't.

So I think we can avoid this entirely by replacing all of the linkedBrowser properties in here[1] with linkedPanel, which is only truth-y if the browser hasn't been discarded (and won't accidentally create a browser if one's not already there).

Arjun, do you have the cycles for one last modification here?

[1]: https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/base/content/tabbrowser.xml#4744-4773

Comment 52

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

Note that if you want to update the patch, you're going to need to re-open the review request at https://reviewboard.mozilla.org/r/206818/.

Comment 53

[Arjun Krishna Babu]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #51)
> Arjun, do you have the cycles for one last modification here?
> 
> [1]:
> https://searchfox.org/mozilla-central/rev/
> e3cba77cee3ff1be38313abe9c804d13c51bd95b/browser/base/content/tabbrowser.
> xml#4744-4773
What exactly do you mean by "cycles"?

(In reply to Mike Conley (:mconley) (:⚙️) from comment #52)
> Note that if you want to update the patch, you're going to need to re-open
> the review request at https://reviewboard.mozilla.org/r/206818/.
I will update the patch, but it might take a few days. I'll try to get it before the end of the weekend; is that fine?

Comment 54

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

(In reply to Arjun Krishna Babu from comment #53)
> What exactly do you mean by "cycles"?

Sorry, I slipped in some jargon there - my apologies! I should have written:

"Arjun, do you have the time for one last modification here?"

> I will update the patch, but it might take a few days. I'll try to get it
> before the end of the weekend; is that fine?

That's fine, thanks!

Comment 55

[Arjun Krishna Babu]

Attachment: bug1373055_updated.patch

My apologies for the inexcusable delay; I'm a full-time grad student in US, and that kept me busy.

Anyway, I made the changes you recommended in comment 51.

Does it look good?

Comment 56

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

> My apologies for the inexcusable delay; I'm a full-time grad student in US,
> and that kept me busy.
> 

Hey Arjun,

Totally understandable, and thanks for coming back with a patch! I'll look at it now.

Comment 57

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

Folded patches pushed to try for testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cf1253adf4d0f335d5f3dd53c6393c4f02a5162

Comment 58

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1524e32b04bfb0140d2e3ae1d8b972790cf6fa
Bug 1373055 - Unload crashed background tabs instead of flipping their remoteness and sending them to about:blank; r=mconley

Comment 59

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

Try looks good, so I've landed this on inbound for you (I didn't want to have to make you roll the two patches together yourself - you've been through enough already. :) )

Comment 60

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ba1524e32b04

Comment 61

[Andreea Pavel [:apavel]]

Backed out for causing a frequency uptick of https://bugzilla.mozilla.org/show_bug.cgi?id=1430466
https://hg.mozilla.org/mozilla-central/rev/b9d1e753f014

Comment 62

[Shivam Singhal [ :championshuttler ]]

Hi :mconley, I would like to work on it, can you help me getting started?

Thanks

Comment 63

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

Hi championshuttler,

I actually think this bug is probably not the best one to work on right now - I'm going to remove the good-first-bug and mentor tags. Unfortunately, this seemed to hit a roadblock with an intermittent test failure that we couldn't figure out. :/

I encourage you to find another juicy bug on https://codetribute.mozilla.org/ to hack on - perhaps bug 1116280?

Comment 64

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.