Intermittent timeout in browser_CaptivePortalWatcher.js on all platforms

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: past, Assigned: nhnt11)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Firefox 50
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 unaffected, firefox50 affected, firefox52 fixed)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 attachments)

The patch from bug 989193 landed with the test disabled on Linux. MattN writes in bug 989193 comment 43:

"I think the problem is related to hasFocus not being true on Linux when the xul-... notification arrives."

We should fix the problem and remove the skip-if in browser.ini.
Priority: -- → P3
Whiteboard: [fxprivacy]
There were some failures on Win8 64 bit after landing, so I'll disable it there too.
Summary: Enable test browser_CaptivePortalWatcher.js on Linux → Enable test browser_CaptivePortalWatcher.js on Linux and Windows

Comment 2

3 years ago
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/0cdadfe25480
Disable browser_CaptivePortalWatcher.js on Windows for intermittent failures . r=bustage

Comment 3

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0cdadfe25480
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 4

3 years ago
Assigning myself
Assignee: nobody → nhnt11
(Assignee)

Comment 7

3 years ago
Comment on attachment 8766972 [details]
Bug 1279491 - Fix timing issue when waiting for captive portal tab in new window in browser_CaptivePortalWatcher.js.

This is just an idea, try push found here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47c0bd64ad51

Removing r? that MozReview automatically added, sorry for the bugmail.
Attachment #8766972 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Attachment #8766973 - Flags: review?(MattN+bmo)
(Assignee)

Comment 8

3 years ago
FWIW, when we receive the xul-window-visible notification, Services.ww.activeWindow reflects the newly focused window even if its document hasn't received focus yet, at least on OS X. I've actually been seeing document.hasFocus() not being true intermittently on OS X as well, which is why I hope that this might help. If it doesn't, I'll be firing up a VM again and having another go at this soon.
(Assignee)

Comment 9

3 years ago
The test failed in Linux and Linux x64 opt in that try push. It's a different failure than what MattN saw in bug 989193, I'm looking into it.
(Assignee)

Comment 10

3 years ago
Comment on attachment 8766972 [details]
Bug 1279491 - Fix timing issue when waiting for captive portal tab in new window in browser_CaptivePortalWatcher.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61676/diff/1-2/
Attachment #8766972 - Flags: review?(MattN+bmo)
Attachment #8766973 - Flags: review?(MattN+bmo)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8766973 [details]
Bug 1279491 - Enable test browser_CaptivePortalWatcher.js on all platforms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61678/diff/1-2/
(Assignee)

Comment 12

3 years ago
Took another crack at this today in a VM. It seems like when we close a window, there's a timing issue with xul-window-visible being fired from the original window after the next test has already started. Please see the comment I wrote in the test code for more details.

Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6f4a453f103
Comment on attachment 8766973 [details]
Bug 1279491 - Enable test browser_CaptivePortalWatcher.js on all platforms.

https://reviewboard.mozilla.org/r/61678/#review59940
Attachment #8766973 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8766972 [details]
Bug 1279491 - Fix timing issue when waiting for captive portal tab in new window in browser_CaptivePortalWatcher.js.

https://reviewboard.mozilla.org/r/61676/#review59942

::: browser/modules/CaptivePortalWatcher.jsm:89
(Diff revision 2)
>      // immediately in that window so they can login before continuing to browse.
> -    if (!win || !win.document.hasFocus()) {
> +    if (!win || Services.ww.activeWindow !=  win) {

Nit: extra space after `!=`
Attachment #8766972 - Flags: review?(MattN+bmo) → review+
I'll fix the nit and land ASAP to see if this helps with some recent test failures on OS X as well.

Comment 16

3 years ago
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/8136281a8b86
CaptivePortalWatcher: use Services.ww.activeWindow to check window focus instead of document.hasFocus(). r=MattN
https://hg.mozilla.org/integration/fx-team/rev/b0ff95a9eb9d
Enable test browser_CaptivePortalWatcher.js on Linux and Windows. r=MattN
backed out for making https://treeherder.mozilla.org/logviewer.html#?job_id=10801102&repo=fx-team perma failure
Flags: needinfo?(nhnt11)

Comment 18

3 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/6e336698908c
Backed out changeset b0ff95a9eb9d 
https://hg.mozilla.org/integration/fx-team/rev/f10815c69250
Backed out changeset 8136281a8b86 for perma failures in own test
(Assignee)

Comment 19

3 years ago
I have pinpointed the intermittent failures to be a timing issue with the way the test waits for a captive portal tab to be opened, for cases where a portal was detected with no browser window already open. I.e., we wait first for a new window, and then a new tab.

The code yielded on BrowserTestUtils.openNewBrowserWindow(), which resolves after delayed startup. The issue is that at this point of time, there may or may not already be a captive portal tab open, depending on timing. The existing code assumes that there is no tab, and waits for a new tab. If the tab was already open, we never stop waiting, so the test times out.

I have a patch that I will be uploading shortly that first checks if there's already a tab/notification, and waits only if there isn't. I got a green try push with this - I retriggered the test plenty of times on Linux x64 debug and opt.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffc6263225d6

This should likely solve all of the intermittent failures around this test.
Flags: needinfo?(nhnt11)
(Assignee)

Comment 20

3 years ago
BTW, I arrived at this diagnosis after realizing that all the timeouts were happening at the beginning of test_detectedWithNoBrowserWindow_Open or test_detectedWithNoBrowserWindow_Redirect. These functions both call openWindowAndWaitForPortalTabAndNotification, and no assertion is ever reached, which means that either we wait forever for a new browser window, or a new tab - the former of which is obviously extremely unlikely.
(Assignee)

Comment 21

3 years ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #20)
> [...] no assertion is ever
> reached, which means that either we wait forever for a new browser window,
> or a new tab - the former of which is obviously extremely unlikely.

As evidenced here: https://dxr.mozilla.org/mozilla-central/rev/3f4c3a3cabaf94958834d3a8935adfb4a887942d/browser/modules/test/browser_CaptivePortalWatcher.js#28
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1287714
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Summary: Enable test browser_CaptivePortalWatcher.js on Linux and Windows → Intermittent timeout in browser_CaptivePortalWatcher.js on all platforms
(Assignee)

Comment 27

3 years ago
mozreview-review
Comment on attachment 8766972 [details]
Bug 1279491 - Fix timing issue when waiting for captive portal tab in new window in browser_CaptivePortalWatcher.js.

https://reviewboard.mozilla.org/r/61676/#review88298

::: browser/modules/CaptivePortalWatcher.jsm:100
(Diff revision 4)
>      let win = RecentWindow.getMostRecentBrowserWindow();
>      // If there's no browser window or none have focus, open and show the
>      // tab when we regain focus. This is so that if a different application was
>      // focused, when the user (re-)focuses a browser window, we open the tab
>      // immediately in that window so they can login before continuing to browse.
> -    if (!win || !win.document.hasFocus()) {
> +    if (!win || win != Services.ww.activeWindow) {

I'm doing this in this patch to make it consistent the way we check whether the window is currently focused (we use activeWindow in _delayedAddCaptivePortalTab below)
Comment on attachment 8766972 [details]
Bug 1279491 - Fix timing issue when waiting for captive portal tab in new window in browser_CaptivePortalWatcher.js.

https://reviewboard.mozilla.org/r/61676/#review88308

LGTM
Comment on attachment 8766973 [details]
Bug 1279491 - Enable test browser_CaptivePortalWatcher.js on all platforms.

https://reviewboard.mozilla.org/r/61678/#review88310

Comment 30

3 years ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2c9b7429061
Fix timing issue when waiting for captive portal tab in new window in browser_CaptivePortalWatcher.js. r=MattN
https://hg.mozilla.org/integration/autoland/rev/428724f364fc
Enable test browser_CaptivePortalWatcher.js on all platforms. r=MattN

Comment 31

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2c9b7429061
https://hg.mozilla.org/mozilla-central/rev/428724f364fc
Status: NEW → RESOLVED
Last Resolved: 3 years ago3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.