1234957 - [e10s] It's possible to accidentally make web pages unresponsive and end up with browser consuming 25% CPU for nothing after closing unbeforeunload tab

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, defect)

Description

[arni2033]

Attachment: testcase 1 - It's possible to accidentally end up with browser consuming 25 CPU.htm

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20151223030323
STR:
1.A) Detach this tab in a new window
1.B) Open attached "testcase 1" in a new window 
2.   Middle-click the link 'data:text/html,<body onbeforeunload="return 1">' on the page from Step 1
     to open it in a new tab.
3.   Switch to that tab with "data:" url, click on content area
4.   Open new tab
5.   Switch to the tab from Step 1
6.   Middle-click the tab with "data:" url from Steps 2-3   [tab title will become bold]
7.   Click the tab with "data:" url from Steps 2-3   [you'll switch to that tab]
8.   Click Reload button in urlbar or middle-click the tab
9.   Middle-click the tab

Result:       
 After Step 7 only blank confirm dialog is visible. Also, the tab from Step 1 stays visually selected
 After Step 8 the normal onbeforeunload dialog appears
 After Step 9 two tabs are visually selected: the tab from Step 1 and New Tab from Step 4
              New Tab infinitely displays the loading spinner at the center of content area.
              Firefox consumes 25% of CPU and stays at this rate until I close the application
 If after Step 9 I open other tabs in the window from Step 1, they are initially unresponsive too.

Expectations: 
 (1) Normal onbeforeunload dialog   (2) Only one selected tab
 (3) Normal CPU usage               (4) No infinite loading spinners

This happens after bug 967873
> pushlog_url:   https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=961911623a6f2ec1d036c7b12a5117ebbeff45d8&tochange=8e0bc70119606b70d74f1aa19d84e697ac4793c7

Comment 1

[arni2033]

(!) Oh, I must mention that the first result ("After Step 7") also applies to single-process mode. It was regressed by bug 332195. Probably we need another bug describing this issue for both modes. I just thought that this one (1234957) should be reported anyway to show the importance of the fix

Comment 2

[arni2033]

Gijs, I saw you working on bug 1234937 and bug 1234936, but this one is probably more important thing about background alerts. I found out that onbeforeunload and normal alert in background are broken for data: and about: pages - see comment 1
This bug is reported for e10s, but it's specifically about consuming 25% CPU and lagging. While in non-e10s mode STR in comment 0 result only in broken alert dialog; I see no lags.
I haven't filed non-e10s bug yet. Please take this into account while working on those 2 bugs.

Comment 3

[:Gijs (he/him)]

I don't know enough about e10s to be of help with the CPU use stuff. I'll look at the empty dialog issue you filed as bug 1235709.

Comment 4

[arni2033]

This was fixed by 1233747, so please somebody set an appropriate status
(I can't choose between "fixed", "wfm" and "duplicate")

Comment 5

[:Gijs (he/him)]

I'm worried that a variant of this might still be reproducible if you use a new window rather than a new tab, and close the tab while a different window has focus.

Comment 6

[arni2033]

I don't see this issue with some combination of windows and alert/onbeforeunload dialogs.

Anyway, I wanted to ask:
In bug 1233747 you said "The exception from that code" - does that mean (A) that exception is the root of the problem (B) the same exception (and bug) may be caused by an add-on?
Is it a good idea to make e10s more reliable by fixing this case? Non-e10s survived it without any damage, while e10s hung all tabs in current window. If it's not a good idea, let's close this bug

Comment 7

[:Gijs (he/him)]

(In reply to arni2033 from comment #6)
> I don't see this issue with some combination of windows and
> alert/onbeforeunload dialogs.

OK, good.

> Anyway, I wanted to ask:
> In bug 1233747 you said "The exception from that code" - does that mean (A)
> that exception is the root of the problem

The fact that the exception was thrown and went uncaught caused issues, yes.

> (B) the same exception (and bug) may be caused by an add-on?

Well, not anymore, that's what the try...catch accomplishes.

I'm not entirely sure what you actually want to know in asking these questions, so it's hard to provide more details.

> Is it a good idea to make e10s more reliable by fixing this case?

I don't know the answer to that question, because I still don't know why the code as-written caused the e10s high CPU usage. Maybe Bill can comment?

Comment 8

[arni2033]

(In reply to :Gijs Kruitbosch from comment #7)
> I'm not entirely sure what you actually want to know in asking these questions
I'm just worried about development process, so I wanted to double-check that this bug is no longer possible. I don't have much time to read all that code, so I ask those who write it.
"It was a breakage in browser.js itself, therefore it can't be caused by an add-on" - sounds plausibly

Just in case, here's the screencast of comment 0:
> https://dl.dropboxusercontent.com/s/j5qcl905l3irj20/bug%201234957%20-%20browser%20consuming%2025%25%20CPU%20for%20nothing.webm?dl=0

Comment 9

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Hi Arni,

I have tested this issue on latest Aurora (46.0a2) build and on latest Nightly (47.0a1) build and I could not reproduce it. I followed your steps and at step #6 the tab with "data:" URL from steps #2-#3 focuses and an alert shows up. If I middle click the tab, it closes it and the next tab will be focused but no infinite loading spinners appear. CPU usage is under 3% the whole time. 

Firefox: 46.0a2, Build ID: 20160209004008
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Firefox: 47.0a1, Build ID: 20160208030244
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

Is it still reproducible for you in the latest builds? Or I can close this as Resolved - WFM?

Thanks,
Cosmin.

Comment 10

[:Gijs (he/him)]

At this point we should just close this as WFM.