860665 - controller.waitForPageLoad() fails when working with Private Browsing windows. Fails with "controller.waitForPageLoad(): Timeout waiting for page loaded"

Status: RESOLVED DUPLICATE of bug 851006

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Andrei Eftimie]

Affected tests:

> /testPrivateBrowsing/testAboutPrivateBrowsing.js

Comment 1

[cmtalbert]

We need investigation here to determine when we fail:
* When opening the window?
* After the window is opened and some time later when trying to access the window?

Setting for 2.1 since this is a specific kind of window.

Comment 2

[Andreea Matei [:AndreeaMatei]]

This fails after opening a new pb window, in that test is done through a callback function, clicking the button in "about:privatebrowsing" loaded in a normal window.
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testPrivateBrowsing/testAboutPrivateBrowsing.js#l48

In pb library we already have waitForPageLoad() and we use it again in the test.
If I remove it from the test, it passes. If I remove it from the lib and leave it in the test, it also passes (but other tests fail because they need it in the lib). Only when used twice we fail (tried this with other pb tests as well).

I think is related to bug 847991.

Comment 3

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

A minimized testcase would be great to have. It might help us to figure out if something in waitForPageLoad has been regressed.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Attachment: testcase

This is checking all ways to open a pb window with open() method from pb library.

I've commented the waitForPageLoad() lines, this way the testcase passes on Mozmill 2.0 (also on 1.5).

If you uncomment them, will fail on Mozmill 2.0, but still pass on 1.5.

NOTE: in 2.0, you need to run whole folder, if you only run the testcase it will fail with "could not find element ID: startPrivateBrowsing" - bug 860655

Comment 5

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 738477 [details] [diff] [review]
testcase

Removing requests as I've been using our library to make my point regarding the double waitForPageLoad() call. Will come up soon with another testcase.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Attachment: testcase

Updated to make it standalone.

Comment 7

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

Comment on attachment 739559 [details]
testcase

Running this testcase in Aurora on Linux everything works fine and I do not see any failure. Which platforms are affected? Seems like not all of them?

Comment 8

[Andreea Matei [:AndreeaMatei]]

Henrik, have you uncommented one of the waitForPageLoad() lines? I left them like that so we can test each way of opening pb with the correspondent line of waitForPageLoad().
Indeed I haven't tried before with aurora, but did now and is failing as well for Beta.

Comment 9

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

A testcase should show the issues without the need of modifications. I don't think that any of the different ways in triggering the window makes a difference for us here, given that we fail in the newly opened window. So please reduce it to a menu action.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Attachment: testcase

Updated to use menu action and fail as it is.

Comment 11

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

That's way better. So it's a regression from 1.5 or we missed to port a change from hotfix-1.5 to master. waitForPageLoad has to be stable so nominating again for 2.0.

Comment 12

[Andreea Matei [:AndreeaMatei]]

In 1.5 we have less checks:
https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L1329

In 2.0, due to this line we get false when trying to wait for a page to load when is already loaded, because we don't have "waitForPageLoad" in win:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L1030

There was a reason for adding this firstRun, because without it some of the local pages (mostly in restartTests) won't load: Unable to connect.

I feel there's no point in duplicating the code and waiting for a page to load several times in a row. Our test should have that line removed and if we happen to use it again like that in a future test, it should let us know to remove it cause it's already waiting in the library.

Comment 13

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

Ouch. Good catch Andreea. I'm not sure if it was necessary. I haven't seen it before either. Please check which changeset brought this into the code base on the master branch. It might be we get more information on that other bug.

Beside that I think we should block this on bug 760720. Once done we can revisit.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Hm, this is where it was introduced, in bug 686030:
https://github.com/mozilla/mozmill/commit/0b4c4deb646df5615e86dac1d1019329d6c757d2

In bug 686030 comment 11 is presented the same issue we have, when waitForPageLoad() it's called twice.

Comment 15

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

Comment on attachment 740774 [details]
testcase

The attached testcase is invalid because it uses two waitForPageLoad calls for the same page which we never reload.

Comment 16

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

This is now fixed!

Comment 17

[Andreea Matei [:AndreeaMatei]]

It's still reproducing in the latest version, on Linux and OS X I checked.
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e60cef2

Comment 18

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

Hm, might be similar to what Szabolcs has seen when waiting for the first browser window to be opened. I will investigate soon.

Comment 19

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

As already figured out earlier by the minimized testcase it's invalid for Mozmill 2.0 because we cannot call waitForPageLoad() twice on the same page after each other. That will timeout because the page has not been changed.

The reason why it is happening for Mozmill 2.0 is the landing of the patch in bug 851006. We have to check there how to proceed.