616152 - Timeout failure while waiting for default homepage in testNewWindow.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

MODULE: testNewWindow.js
TEST: testNewWindow
ERROR: waitForEval: Timeout exceeded for 'subject.targetURL.value == subject.currentURL' 
BRANCH: default
PLATFORM: Linux/Windows
APPLICATION: Firefox 4.0b8pre (2.0b8pre, en-US, 20101202030316)

Recent reports:

Linux Ubuntu 10.4 - http://mozmill-release.brasstacks.mozilla.com/#/general/report/2c37a7c1942414e420c43bdb342f9a30

Windows NT 5.1.2600 - http://mozmill-release.brasstacks.mozilla.com/#/general/report/2c37a7c1942414e420c43bdb342fca99

Windows NT 6.1.7600 - http://mozmill-release.brasstacks.mozilla.com/#/general/report/2c37a7c1942414e420c43bdb342fa8e8

Location of error - http://hg.mozilla.org/qa/mozmill-tests/file/487ee592b42f/firefox/testTabbedBrowsing/testNewWindow.js#l87

Comment 1

[Aaron Train [:aaronmt]]

The problem is that the test is failing when it handles the wrong window. The current sleep solution is demonstrating that it is not reliable [1]. Is there a reliable way to wait for the new window?

[1] http://hg.mozilla.org/qa/mozmill-tests/file/e885ee9e1007/firefox/testTabbedBrowsing/testNewWindow.js#l72

Comment 2

[u279076]

I'm wondering if there is something we could throw into a waitFor()?

In fact, maybe we should have a controller.closeWindow() function?  The function would not only close the window, but do a check to ensure the window is closed.

Comment 3

[Aaron Train [:aaronmt]]

In particular, this is in regards to a newly opened window, but yes, I vouch for the logic to be handled in utils::handleWindow preferably.

Comment 4

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default) [checked-in]

Removed the sleep, and changed the window handler to use the default window title which in turn references the most recently opened window. Works like a charm.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 497584 [details] [diff] [review]
Patch v1 - (default) [checked-in]

>+  controller2 = utils.handleWindow("type", "",
>                                    checkDefaultHomepage, false);

Please move the parameters up, so we only have one single line. Otherwise r=me.

Comment 6

[Aaron Train [:aaronmt]]

Fixed and landed 

default - http://hg.mozilla.org/qa/mozmill-tests/rev/1a547e8e7200
1.9.2 - http://hg.mozilla.org/qa/mozmill-tests/rev/c79ed8e8f01d
1.9.1 - http://hg.mozilla.org/qa/mozmill-tests/rev/4cc6a1e62b84

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is not fixed, and still happens all the time across branches:

http://mozmill-release.brasstacks.mozilla.com/#/general/failure?branch=All&platform=Windows%20NT&from=2010-12-20&to=2010-12-27&test=firefox%2FtestTabbedBrowsing%2FtestNewWindow.js&func=testNewWindow

Comment 8

[Aaron Train [:aaronmt]]

From testing locally and on the machines I have been unable to reproduce the issue. I was suspecting the issue is similarily akin to the order of testing from script investigation by Anthony.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

How did you run the tests? With Mozmill directly or with the testrun_general.py script? The latter one should be the first step to try.

Comment 10

[Aaron Train [:aaronmt]]

(In reply to comment #9)
> How did you run the tests? With Mozmill directly or with the testrun_general.py
> script? The latter one should be the first step to try.

Both actually. I tested on XP/Vista/7 and Ubuntu, running mozmill directly and testrun_general.py, in the logs they show this as a pass. Is the configuration different in the daily testrun?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, there is no difference. But if those failures persist, I would recommend to update the test so we add the value of both elements to the report (get - expected). That way it could give us a hint where to start.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Any progress on this failure? Can we please completely disable the test for the time being, so we don't have those errors?

Comment 13

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default) - [Enable skip test]

Have not been able to reproduce at all on the machines running testrun_general across platforms and branches, the test itself and the test collection module.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 501648 [details] [diff] [review]
Patch v1 - (default) - [Enable skip test]

Lets wait for the more detailed test failure.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alright, the issue we are facing here is clear now:

Current URL should be identical to the target URL - got about:home, expected http://www.mozilla.org/projects/firefox/prerelease.html 

Aaron, do you want to continue and fix this test?

Comment 16

[Aaron Train [:aaronmt]]

Dave and I took some investigation this morning both locally and remotely but have so far come up dry in terms of being able to reproduce. I'm not entirely sure where/what to look at next would be.

Comment 17

[u279076]

(In reply to comment #16)
> Dave and I took some investigation this morning both locally and remotely but
> have so far come up dry in terms of being able to reproduce. I'm not entirely
> sure where/what to look at next would be.

I've recently discovered that some of our failures are only happening on qa-horus.  For example, testPasswordNotSaved only fails in the daily testrun on qa-horus; it does not fail on the daily testrun from qa-set nor locally.  Can you see if this is one such failure?

Comment 18

[Aaron Train [:aaronmt]]

> I've recently discovered that some of our failures are only happening on
> qa-horus.  For example, testPasswordNotSaved only fails in the daily testrun on
> qa-horus; it does not fail on the daily testrun from qa-set nor locally.  Can
> you see if this is one such failure?

Been testing on qa-horus included already. My only other idea, is if possible to watch the daily test-runs in view

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I have taken a look at this and it is somehow suspicious. The default homepage on 4.0 is definitely 'about:home' but expect to see "http://www.mozilla.org/projects/firefox/prerelease.html". After searching a while I have found that this URL is loaded when you open the Release Notes via the help menu. For pre-releases we don't have release notes so we forward people to this address.

That means that when the new window gets opened 'about:home' is somehow redirected to the above URL.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, I believe I know what's going on here. We loosely handle the newly opened window in the handleWindow function. The reason is that we actually don't really have great support for multiple windows of the same type. Not by Mozmill itself nor in our shared modules. As you can see when opening the browser the above mentioned page is loaded as the initial page in the first tab. That's the only time this page really gets loaded. We are loading "about:blank" into that tab, but - I have no idea why - that could fail and the pre-release page is still open. Now we are opening the new window, but we could get the wrong window on slower machines. We don't really check that the window is different from controller.window.

I propose that we fix that flaw in the test and check if that helps. Patch upcoming.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aaron, I hope you are ok when I steal this bug from you.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1 (handle window correctly)

This patch updates the test to make sure that we are using the correct window.

Comment 23

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 502685 [details] [diff] [review]
Patch v1 (handle window correctly)

Looks OK, so I r+'d. A little commentary below, though.

>+    // Make sure that we work on the correct window
>+    var windows = mozmill.utils.getWindows("navigator:browser");
>+    windows.forEach(function (window) {
>+      if (window !== controller.window)
>+        controller2 = new mozmill.controller.MozMillController(window);
>+    });
> 
>-  controller2 = utils.handleWindow("type", "", checkDefaultHomepage, false);
>+    return !!controller2;

However, it may make sense to add a break; at the end of the if block. I know there are only supposed to be two windows, but if there were a cleanup issue prior you might mistakenly have more. 

The way it is now will iterate through all of them, leaving controller2 assigned the last. A break would leave controller2 assigned the first. I'm not sure how the windows are stored, and thus which one is the most recent. 

I'm also honestly a little torn as to whether we should aim to make the test always work (pick most recent), or to intentionally break on a previous cleanup error (pick least recent). I lean towards the latter. I'd be interested to know your viewpoint on that.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #23)
> However, it may make sense to add a break; at the end of the if block. I know
> there are only supposed to be two windows, but if there were a cleanup issue
> prior you might mistakenly have more. 

That absolutely makes sense! Bad sadly forEach doesn't support break, so you will have to throw an exception or we can simply make use of the default for loop. I have changed the code to the latter path.

> The way it is now will iterate through all of them, leaving controller2
> assigned the last. A break would leave controller2 assigned the first. I'm not
> sure how the windows are stored, and thus which one is the most recent. 

I would say those are ordered by z position. Means the top-most window should appear first. So breaking the loop early is the right way to go.

> I'm also honestly a little torn as to whether we should aim to make the test
> always work (pick most recent), or to intentionally break on a previous cleanup
> error (pick least recent). I lean towards the latter. I'd be interested to know
> your viewpoint on that.

That will not work because of slowness on some systems. As an alternative we could call getMostRecentWindow and wait until the controller's window is not the current window. But once a page gets opened behind the current window, our code would be broken. So I wouldn't use this way in general.

Thanks Geo! I will check-in the current code, so we can check if it fixes our problems with this test. I'm not 100% sure it will, but lets see.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2 (handle window correctly) [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/4a0b6d2db468 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/3c29a812f294 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/2bce7e631257 (1.9.1)

I will leave this bug open so we can check todays results and if we need to fix even more.

Comment 26

[Aaron Train [:aaronmt]]

(In reply to comment #21)
> Aaron, I hope you are ok when I steal this bug from you.

Glad you figured it out, let's see results soon.

Comment 27

[Aaron Train [:aaronmt]]

It seems like it would make sense to express disapproval (deprecate) of using handleWindow() for multiple windows as a result of this bug on the documentation at, https://developer.mozilla.org/en/Mozmill_Tests/Shared_Modules/UtilsAPI, but it isn't there in the first place. If this is desired, I can add it?

Comment 28

[Aaron Train [:aaronmt]]

Today's results show a failure

Examples :

3.5.17pre - http://mozmill-release.brasstacks.mozilla.com/#/general/report/1d2bdcae68748c40bbb80427179aff63

4.0b10pre - http://mozmill-release.brasstacks.mozilla.com/#/general/report/1d2bdcae68748c40bbb80427179a6273

3.6.14pre - http://mozmill-release.brasstacks.mozilla.com/#/general/report/1d2bdcae68748c40bbb804271799dc81

Comment 29

[Aaron Train [:aaronmt]]

If we now have reason to believe that bug 580438 is causing this, shall we disable this test using one of the provided patches above until the dependancy is fixed?

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, this test fails only when the testrun gets executed via our testrun_all.py or testrun_daily.py scripts. Not sure why yet in detail but I suspect bug 580438.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, we are blocked on bug 580438 here. As it looks like the additional --jsbridge=24242 option at the command line opens the browser with exactly a single tab. Keep in mind that we normally have two tabs open for the first start! We somehow stop to show the initial two pages, but those are getting opened exactly by this test in a new window. The first tab of this new window has the prerelease page open and it's causing this test to fail.

Opening another window solves the problem. Lets see if I can find a workaround until bug 580438 has been fixed.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So it is really caused by bug 580438. I have already attached a possible patch even when it is a workaround. For now I would just disable the test on Windows and Linux for all branches.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See bug 625614 for the underlying core issue.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch - temporarily disable test on Windows and Linux

Disable the test for now until Mozmill 1.5.2 has been released.

Comment 35

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 503697 [details] [diff] [review]
Patch - temporarily disable test on Windows and Linux

Kinda bummed that the only real way to do a platform-specific skip is exit early like this, since it means the test results will still show it passing.

Code looks fine, r+.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch - temporarily disable test on Windows and Linux (v2 - default)

No, it's not. We can indeed skip it.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 503706 [details] [diff] [review]
Patch - temporarily disable test on Windows and Linux (v2 - default)

I really want to see this test passing in todays testrun. Switching review over to Aaron, so we can make sure it will be landed in-time, even for older branches.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch - temporarily disable test on Windows and Linux (v2 - 1.9.1)

Simple backport to 1.9.1

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/a3728a0f64fc (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/36a2f060f298 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/179c9d5e1a48 (1.9.1)

Comment 40

[Aaron Train [:aaronmt]]

The above changes can now be reverted. Can we revert those changesets?

Comment 41

[Aaron Train [:aaronmt]]

Henrik? Can those be backed out, or do new patches have to be made?

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Now that we are stable with Mozmill 1.5.2 I have backed-out all temporary landings:

http://hg.mozilla.org/qa/mozmill-tests/rev/fec1bce1e6db (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/62aa4da55d30 (2.0)
http://hg.mozilla.org/qa/mozmill-tests/rev/80f941aceff3 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/6bdf309df890 (1.9.1)

This is fixed now! Yay.