1279491 - Intermittent timeout in browser_CaptivePortalWatcher.js on all platforms

Status: RESOLVED FIXED

(Firefox :: General, defect, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

There were some failures on Win8 64 bit after landing, so I'll disable it there too.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0cdadfe25480
Disable browser_CaptivePortalWatcher.js on Windows for intermittent failures . r=bustage

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cdadfe25480

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Assigning myself

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1279491 - Fix timing issue when waiting for captive portal tab in new window in browser_CaptivePortalWatcher.js.

Review commit: https://reviewboard.mozilla.org/r/61676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61676/

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1279491 - Enable test browser_CaptivePortalWatcher.js on all platforms.

Review commit: https://reviewboard.mozilla.org/r/61678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61678/

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

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/

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

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/

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

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 13

[Matthew N. [:MattN]]

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

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

Comment 14

[Matthew N. [:MattN]]

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 `!=`

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Carsten Book [:Tomcat]]

backed out for making https://treeherder.mozilla.org/logviewer.html#?job_id=10801102&repo=fx-team perma failure

Comment 18

[Pulsebot]

Backout by [e-mail]:
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

Comment 19

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 20

[(inactive) Nihanth Subramanya [:nhnt11]]

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.

Comment 21

[(inactive) Nihanth Subramanya [:nhnt11]]

(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

Comment 23

[(inactive) Nihanth Subramanya [:nhnt11]]

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/2-3/

Comment 24

[(inactive) Nihanth Subramanya [:nhnt11]]

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/2-3/

Comment 25

[(inactive) Nihanth Subramanya [:nhnt11]]

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/3-4/

Comment 26

[(inactive) Nihanth Subramanya [:nhnt11]]

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/3-4/

Comment 27

[(inactive) Nihanth Subramanya [:nhnt11]]

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 28

[Matthew N. [:MattN]]

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 29

[Matthew N. [:MattN]]

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

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2c9b7429061
https://hg.mozilla.org/mozilla-central/rev/428724f364fc