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)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 52
People
(Reporter: KWierso, Assigned: Gijs)
References
(Depends on 1 open bug)
Details
(Keywords: intermittent-failure)
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
billm
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 29•8 years ago
|
||
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 32•8 years ago
|
||
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)
Assignee | ||
Comment 33•8 years ago
|
||
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)
Assignee | ||
Comment 34•8 years ago
|
||
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...
Assignee | ||
Comment 35•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8791644 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 37•8 years ago
|
||
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 38•8 years ago
|
||
mozreview-review |
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)
Comment hidden (Intermittent Failures Robot) |
Comment 40•8 years ago
|
||
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 41•8 years ago
|
||
mozreview-review |
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+
Comment 42•8 years ago
|
||
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
Comment 43•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/413dd09c1e88
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 44•8 years ago
|
||
Looks like it's working :). Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 45•8 years ago
|
||
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+
Comment 47•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/e4269c82e54f
Flags: in-testsuite+
Comment 48•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c3e2971240ea
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•