Closed Bug 1211637 Opened 9 years ago Closed 8 years ago

Intermittent browser_bug585558.js | Fourth tab marked last-visible-tab! - Got false, expected true

Categories

(Firefox :: Tabbed Browser, defect, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: KWierso, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1211637 - if there's no docShell (on a local tab) or permitUnload times out (on a remote tab), close the tab,
(deleted), text/x-review-board-request
billm
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details 
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Gijs, do you have cycles to look into this by chance?
status-firefox48: --- → wontfix
status-firefox49: --- → wontfix
status-firefox50: --- → affected
status-firefox51: --- → affected
Flags: needinfo?(gijskruitbosch+bugs)
I can't reproduce this locally on a linux VM, even with run-until-failure with 100 repeats on a debug artifact build.

I'm currently running the same thing on an rr record chaos mode cycle with a self-compiled debug build in my VM, but I'm not super optimistic I'll be more lucky there.

From the screenshot, I think the problem is that somehow the tab we're removing with removeTab() in that test isn't actually getting removed. I don't know why that isn't happening. To find out, if I can't reproduce, I'd need to add logging to tabbrowser.xml itself which means I can't just land said logging because it'd be app logging rather than test logging.

20 times a week (rather than 20 a day) also doesn't seem as frequent as some of the top oranges - Ryan, am I missing something? Is this nevertheless in the OF top hits or something?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ryanvm)
The rr thing got me a trace but trying to use it crashes rr, so that's not a lot of help.

I'd still like to understand if this is somehow more frequent than I think it is or whatever before I poke at this more, as it'll change what the best angle of attack is, as it were...
so rr debugging wasn't very helpful, but rr chaos mode + lots of printfs got me there in the end.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(ryanvm)
Attachment #8791644 - Flags: review?(gkrizsanits)
So what's happening is that we time out while waiting to call permitUnload() on a tab. I think we should just let the tab close in that case. If we're waiting that long, it's likely the content process isn't being very responsive, and the last thing we want is for the user to click [x] on a tab and for it to do nothing.

Note that the window closing code already works this way ( http://searchfox.org/mozilla-central/rev/515bca4fca5e2e8233ed30e6b6d48fcab8497dbf/browser/base/content/browser.js#6167-6173 )
Comment on attachment 8791644 [details]
Bug 1211637 - if there's no docShell (on a local tab) or permitUnload times out (on a remote tab), close the tab,

https://reviewboard.mozilla.org/r/79016/#review77610

So I've ended up just using a different timeout for some tests where this can happen and causes the test to fail: see bug 1294388. The default timeout is 5 seconds, which is a lot of time for this to ever happen on a release build in practice (especially without the user keep clicking on close again, which triggers a different closing mechanism). On the other hand I think this is the right thing to do, I just never had the time to read all the related code to figure out the possible consequences. Bill, knows this area a lot better than me so I'm sure he can make the right call here, but thanks for keeping me in the loop.
Attachment #8791644 - Flags: review?(gkrizsanits)
It seems like a lot of tests failing because of this issue (when we have more than one content processes), so I might add the increased timeout globally for tests or if this patch is acceptable as it is, then it might be a better alternative (faster test runs).
Comment on attachment 8791644 [details]
Bug 1211637 - if there's no docShell (on a local tab) or permitUnload times out (on a remote tab), close the tab,

https://reviewboard.mozilla.org/r/79016/#review78258
Attachment #8791644 - Flags: review?(wmccloskey) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/413dd09c1e88
if there's no docShell (on a local tab) or permitUnload times out (on a remote tab), close the tab, r=billm
https://hg.mozilla.org/mozilla-central/rev/413dd09c1e88
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Looks like it's working :). Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8791644 [details]
Bug 1211637 - if there's no docShell (on a local tab) or permitUnload times out (on a remote tab), close the tab,

Approval Request Comment
[Feature/regressing bug #]: making tab-closing e10s-compatible.
[User impact if declined]: intermittent orange; potentially failing to close tabs if the request to do so to the content process times out;
[Describe test coverage new/current, TreeHerder]: beforeunload's behaviour has automated test coverage - indeed, that is why we had an intermittent...
[Risks and why]: reasonably low. The code changes are very simple and in that sense carry no risk. The patch is well-understood. This patch does (further) open up the possibility that we might more frequently not send an onbeforeunload event to the content tab if the user tries to close it, if the content process somehow is not responding in a timely fashion. Given that this is already possible (if you click the close button a second time while the first time's request is still pending) and given that we're slowly weaning the web off beforeunload, this seems like the correct thing to do anyway.
[String/UUID change made/needed]: nope.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8791644 - Flags: approval-mozilla-beta?
Attachment #8791644 - Flags: approval-mozilla-aurora?
Comment on attachment 8791644 [details]
Bug 1211637 - if there's no docShell (on a local tab) or permitUnload times out (on a remote tab), close the tab,

Fixes an intermittent test failure, Aurora51+, Beta50+
Attachment #8791644 - Flags: approval-mozilla-beta?
Attachment #8791644 - Flags: approval-mozilla-beta+
Attachment #8791644 - Flags: approval-mozilla-aurora?
Attachment #8791644 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e4269c82e54f
status-firefox51: affected → fixed
Flags: in-testsuite+
https://hg.mozilla.org/releases/mozilla-beta/rev/c3e2971240ea
status-firefox50: affected → fixed
Depends on: 1312279
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: