Closed Bug 1371509 Opened 8 years ago Closed 7 years ago

Intermittent uriloader/exthandler/tests/mochitest/browser_auto_close_window.js | got the right windowContext - Got about:blank, expected https://example.com/browser/uriloader/exthandler/tests/mochitest/download_page.html

Categories

(Firefox :: File Handling, defect)

Product:
Firefox
Component:
File Handling
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 65
Tracking Flags:
Tracking Status
firefox64 --- fixed
firefox65 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: Gijs)

References

Details

(Keywords: intermittent-failure)

Attachments

(3 files)

Disable browser_auto_close_window.js for frequent failures
718 bytes, patch
jmaher
: review+
Details | Diff | Splinter Review
Bug 1371509 - Make the test browser_auto_close_window.js wait for the correct URL before continuing. r=Gijs
46 bytes, text/x-phabricator-request
Details | Review
Bug 1371509 - fix issues with comparing window URIs in browser_auto_close_window.js, r?felipe
47 bytes, text/x-phabricator-request
Details | Review
If this test relies on e10s behavior, could be disabled on e10s? If yes, I created this patch. Thanks
Assignee: nobody → ccoroiu
Attachment #9018811 - Flags: review?(jmaher)
Comment on attachment 9018811 [details] [diff] [review] Disable browser_auto_close_window.js for frequent failures Review of attachment 9018811 [details] [diff] [review]: ----------------------------------------------------------------- thanks, in this case we only run browser-chrome tests on e10s, so we can safely disable it.
Attachment #9018811 - Flags: review?(jmaher) → review+
Keywords: checkin-needed, leave-open
Whiteboard: [stockwell disable-recommended] → [stockwell disabled]
Pulsebot didn't comment when this landed for some reason. https://hg.mozilla.org/integration/mozilla-inbound/rev/52baa1bbbc79
Keywords: checkin-needed
Why was no engineer asked to actually look at why this was failing? The frequency of the failures here suggests we actually broke something, and right at the end of soft freeze, too. :-( Felipe, any idea what's happened here?
Flags: needinfo?(felipc)
(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #66) > thanks, in this case we only run browser-chrome tests on e10s, so we can > safely disable it. To be clear, the test was skipped on *non-e10s*. It was supposed to be running on e10s, and not failing like this...
I can easily reproduce this test with mach test --run-until-failure It looks like there' s a transient about:blank that gets loaded, and the waitForNewTab function accepted it instead of waiting for the correct URL. I haven't investigated what caused it to spike, but it wouldn't be too hard to do. I wrote a simple patch that fixes it so that we can re-enable the test. It passed without problems with mach test --repeat=150
Assignee: ccoroiu → felipc
Status: NEW → ASSIGNED
Flags: needinfo?(felipc)
fwiw I'm trying to bisect it with hg bisect + artifact builds but I'm not sure how far that will take
Pushed by fgomes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/183d44a49259 Make the test browser_auto_close_window.js wait for the correct URL before continuing. r=Gijs
So bisecting this with inbound + artifacts were a bit harder than usual due to: - merges - use of artifact builds (which I think sometimes picks a nearby changeset) - the fact that this was test was already intermittent, possibly throwing the bisection in the wrong direction At the end the bisection ended in a nonsensical bug, but looking at the range a couple of steps back suspiciously pointed to Bug 1499234 as a possible reason for the increase in failures. I then backed out my fix here and also bug 1499234 and, while the failures didn't go away, they reduced. Reading more of this test I think there's some things that it does wrong, specially around now checking if the helper app dialog window is ready before continuing. I added a DOMContentLoaded wait there and that did reduce the problems too, but not as strongly as the other attempts. I think what happens is that that window is ready for the previous testcase purposes, but the full load only comes later, tripping up later testcases (e.g. the one that I fixed with the above changeset). All in all, it looks like that's not all that was needed to make this test be correct, but it's been sufficient for it to make it rarely fail for me locally.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [stockwell disabled] → [stockwell]
Fail reappeared on autoland. Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=207450094&repo=autoland&lineNumber=5149 Log snippet: 00:23:53 INFO - TEST-PASS | uriloader/exthandler/tests/mochitest/browser_auto_close_window.js | Showing the helper app dialog - 00:23:53 INFO - Buffered messages finished 00:23:53 INFO - TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/mochitest/browser_auto_close_window.js | got the right windowContext - Got about:blank, expected https://example.com/browser/uriloader/exthandler/tests/mochitest/download_page.html 00:23:53 INFO - Stack trace: 00:23:53 INFO - chrome://mochikit/content/browser-test.js:test_is:1295 00:23:53 INFO - chrome://mochitests/content/browser/uriloader/exthandler/tests/mochitest/browser_auto_close_window.js:testNewTab:73 00:23:53 INFO - chrome://mochitests/content/browser/uriloader/exthandler/tests/mochitest/browser_auto_close_window.js:target_blank/<:84 00:23:53 INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:111 00:23:53 INFO - chrome://mochitests/content/browser/uriloader/exthandler/tests/mochitest/browser_auto_close_window.js:target_blank:83 00:23:53 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093 00:23:53 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084 00:23:53 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:986 00:23:53 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795 00:23:53 INFO - TEST-PASS | uriloader/exthandler/tests/mochitest/browser_auto_close_window.js | tab was opened and closed -
Status: RESOLVED → REOPENED
Flags: needinfo?(felipc)
Resolution: FIXED → ---
So this test has always been intermittent, but it raised in frequency recently. My patch attempted to reduce the frequency to how it was before.. Let's wait and see how it plays out. If it stays high frequency I can take a second look at it.
Assignee: felipc → nobody
Flags: needinfo?(felipc)
This is back to the previous failure rate, so I don't think we need to disable this test anymore.
Whiteboard: [stockwell disable-recommended]
See Also: → 1503050
:jmaher, can we teach the bot not to do this if the frequency has actually reduced? Having to keep re-setting the whiteboard every week gets boring.
Flags: needinfo?(jmaher)
Whiteboard: [stockwell disable-recommended]
:gijs, this is still a high failure rate, but I agree not enough to force a disable. The rule is 150 failures in 21 days, we have bug 1503188 to adjust this to account for fixes. The rules there will not affect this bug given the continued high failure rate. Worse case scenario is this is manually adjusted for 3 weeks. If this was worked on, can we not fix it to be better, it seems to be an issue on OSX only.
Flags: needinfo?(jmaher)
Reading this test some more, I think this assertion: https://searchfox.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/browser_auto_close_window.js#73-74 is just bogus. We're intending to check the toplevel browser window we got passed is the right one, but we do so by checking the URL of the selected tab - which may be the new tab that's opened for the download specifically (and then gets closed), which might be about:blank, which causes the intermittents. Felipe, is it enough here just to check that windowContext == window ? The test is a bit strange anyway, because it feels like windowContext is meant to be a content window ref, but we're in the parent process so I don't understand how that could work.
Flags: needinfo?(felipc)
Aah this test is really strange, indeed :( what's even weirder is that targetURL is the URL of the original tab, not download.bin (which is what the `a#target_blank` points to)... so it seems to rely on luck that the previous tab is still selected, and not the new window that briefly opened and closed (*) We can see that in the case of test_new_window, it doesn't compare the windowContext with the windowOpened.. actually it compares to the original window. So it looks like that windowContext is meant to be a reference to the original browser window that triggered the dialog. This testNewTab takes a `browser` as a parameter to represent the originating tab where to run this test. So how about we check windowContext == browser.ownerGlobal? That is, AFAICT, the closest to what this was intended to check. If that passes, I'd also make the same change here: https://searchfox.org/mozilla-central/rev/efc0d9172cb6a5849c6c4fc0f19d7fd5a2da9643/uriloader/exthandler/tests/mochitest/browser_auto_close_window.js#104 It is equally wrong, but it's just less problematic because, since it's a new window, the previous tab on the previous window, (*), remains selected, so it doesn't intermittently failed.
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #86) > what's even weirder is that targetURL is the URL of the original tab, not > download.bin (which is what the `a#target_blank` points to)... so it seems > to rely on luck that the previous tab is still selected, and not the new > window that briefly opened and closed (*) s/not the new window/not the new tab/
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e51037702c9e fix issues with comparing window URIs in browser_auto_close_window.js, r=Felipe
https://hg.mozilla.org/mozilla-central/rev/e51037702c9e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: