Test failure "controller.waitForPageLoad(): Timeout waiting for page loaded" with mozmill 1.5 in testNewTab.js

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mario.garbi, Assigned: cosmin-malutan)

Tracking

({regression})

Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24 fixed, firefox25 fixed, firefox26 fixed, firefox31 wontfix, firefox32 fixed, firefox-esr24 wontfix, firefox-esr31 fixed)

Details

(Whiteboard: [mozmill-test-failure][sprint2013-39], URL)

Attachments

(4 attachments, 8 obsolete attachments)

(Reporter)

Description

6 years ago
We have noticed this failing on each run today and it might be a regression due to Bug 791670 landing.
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=8f9ba85eb61c

Sample report:
http://mozmill-ci.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916bf9446
(Reporter)

Updated

6 years ago
No longer depends on: 791670
(Reporter)

Updated

6 years ago
Hardware: x86 → All
I'm not exactly sure how controller.waitForPageLoad() works but since docShells are swapped there sometimes will be no load event. What you can do is:

https://hg.mozilla.org/mozilla-central/rev/bbac98ae1fea#l2.12

So basically if document.readyState === "complete" then the newtab page has been swapped in and was already preloaded. if readyState !== "complete" you can wait for a "load" event.
I cannot reproduce this locally with the latest Nightly build from today. Are you able to?
Keywords: regression
(Reporter)

Comment 3

6 years ago
Posted patch Skip patchSplinter Review
Uploaded a skip patch until we can come with a fix so we stop the failure reports.

We were able to reproduce it locally indeed.
Attachment #758572 - Flags: review?(hskupin)
Attachment #758572 - Flags: review?(dave.hunt)
Attachment #758572 - Flags: review?(andreea.matei)
Comment on attachment 758572 [details] [diff] [review]
Skip patch

Review of attachment 758572 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Mario, skip patch landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/ce39598543c1
Attachment #758572 - Flags: review?(hskupin)
Attachment #758572 - Flags: review?(dave.hunt)
Attachment #758572 - Flags: review?(andreea.matei)
Attachment #758572 - Flags: review+
Attachment #758572 - Flags: checkin+
(In reply to mario garbi from comment #3)
> We were able to reproduce it locally indeed.

Please show debug output. I still can't reproduce this problem.

Comment 6

6 years ago
Posted file logOutput (obsolete) —
(In reply to Henrik Skupin (:whimboo) from comment #5)
> Please show debug output. I still can't reproduce this problem.
I have attached the output for the failure. We reproduced the issue on Linux 12.04 x86 with the latest nightly build.
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Do you also see this with latest Mozmill 2.0 tip? It should be easier to debug with all the dump() statements commented in from windows.js.

Comment 8

6 years ago
(In reply to Henrik Skupin (:whimboo) from comment #7)
> Do you also see this with latest Mozmill 2.0 tip? It should be easier to
> debug with all the dump() statements commented in from windows.js.
I cannot reproduce it with mozmill 2.0. 

With 1.5 I can reproduce it on both Linux 12.04 and MAC OS 10.7.5, but it doesn't reproduce on both OS with 2.0
Ok, good to hear that! Looks like we are pretty solid in 2.0 now. In case for 1.5 please comment in all dump statements from /resource/modules/init.js, and provide the appropriate output. There is no need to run with --show-all. Just do a --show-errors.
Posted file log output for mozmill 1.5 (obsolete) —
These are the logs for running with mozmill 1.5 on Linux 12.04 x86 with dump statements from resource/modules/init.js.
Comment on attachment 758572 [details] [diff] [review]
Skip patch

This patch should really have gotten a better commit message. It says nothing about skipping the test! :(
And also please use our template when filing new failures. It seems like the whiteboard tag is always missed those days.
Priority: -- → P1
Whiteboard: [mozmill-test-failure]
Comment on attachment 758999 [details]
log output for mozmill 1.5

This log output is not useful given that it does not contain any debug output from the window map dump() calls.
Attachment #758999 - Attachment is obsolete: true
Posted file logs with window map dumps (obsolete) —
I commented in the dumps from controller.js, too.
I still miss a dump() call commented in. I have to see for which window id the waitForPageLoad method waits. Otherwise I can't really identify that.
Here's a similar dump from 1.5 with XP32 SP3 just for comparison. I don't see a window id there either unfortunately.
In that case add one inside of isLoaded() which gets periodically called until the page has been loaded.
Posted file logs (obsolete) —
Added a dump inside the isLoaded() method in controller.js to show the ID of the window.
Attachment #759107 - Attachment is obsolete: true
This is scary. We are waiting for a content window which we haven't registered yet. That means it is clear why it is failing, but not how this could happen. I'm still unable to reproduce this.

Andreea and Andrei, is one of you able to see that? It would help quite a lot to get this fixed.
Flags: needinfo?(andrei.eftimie)
Flags: needinfo?(andreea.matei)

Comment 20

6 years ago
I can reproduce this on mozmill 1.5
As a sidenote, this does not happen on mozmill 2.0
Flags: needinfo?(andrei.eftimie)
Daniela, please check the test itself and the comment in line 74. As you can see this referenced bug is fixed meanwhile. So we should have removed that line.
Flags: needinfo?(andreea.matei)
(In reply to Henrik Skupin (:whimboo) from comment #21)
> Daniela, please check the test itself and the comment in line 74. As you can
> see this referenced bug is fixed meanwhile. So we should have removed that
> line.
I have removed the waitForPageLoad in line 74, but we still get some errors when comparing controller.tabs.activeTab.location.href with newTabURL. Error log is attached.

This can be fixed by using an expect.waitFor instead of expect.equal at line http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testTabbedBrowsing/testNewTab.js#l80

Please tell me if this fix would be ok.
Attachment #758625 - Attachment is obsolete: true
Attachment #759178 - Attachment is obsolete: true
Attachment #759638 - Attachment is obsolete: true
No, we cannot simply change this. Given the output you can see that wo/ the waitForPageLoad() we end up testing about:blank but not about:newtab. So whatever has been changed or not, we can't do that change and should most likely drop the comment and making it less confusing.
Posted patch patch v1.0 - remove comment (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #23)
> No, we cannot simply change this. Given the output you can see that wo/ the
> waitForPageLoad() we end up testing about:blank but not about:newtab. So
> whatever has been changed or not, we can't do that change and should most
> likely drop the comment and making it less confusing.
I have removed the comment, but the test is still skipped because it will only work with mozmill 2.0. 

For 1.5:
- We could patch mozmill so that we handle windows and wait for loading events as we do in 2.0, but this would be a massive change and I am not sure it's worth it.
- We can also add a check in the test to verify the document.readyState before waitForPageLoad, but this would be a workaround only if we would like to unskip it.

Otherwise, we need to wait for 2.0 to unskip it.
Attachment #760926 - Flags: feedback?(hskupin)
Comment on attachment 760926 [details] [diff] [review]
patch v1.0 - remove comment

Review of attachment 760926 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Daniela Petrovici from comment #24)
> For 1.5:
> - We could patch mozmill so that we handle windows and wait for loading
> events as we do in 2.0, but this would be a massive change and I am not sure
> it's worth it.

Not sure I understand that comment. Also in 1.5 we are waiting until a tab has been loaded and flag it as loaded. Why you are talking about the handling of windows? This test is only about a new tab.
Attachment #760926 - Flags: feedback?(hskupin)
Based on the discussion in Ask an expert meeting yesterday, we have talked about adding in mozmill the preference: browser.newtab.preload and setting it to false. This test is passing with this preference:
http://mozmill-crowd.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1e0b1eb1

The other bugs (bug 880135 and bug 885221) are not fixed by adding the same preference. So, if it is only for this test that the preference helps, do you still want me to add it in mozmill 1.5 preferences or just in this test?
No, in that case we don't want to change it. Please mark all about:newtab bugs where mozmill 2.0 is involved as dependent on bug 885221. So what changes to do you see when we for now go back to about:blank for any cases?
(In reply to Henrik Skupin (:whimboo) from comment #27)
> No, in that case we don't want to change it. Please mark all about:newtab
> bugs where mozmill 2.0 is involved as dependent on bug 885221. 

This issue reproduces only for mozmill 1.5. I updated the subject.

> So what changes to do you see when we for now go back to about:blank for any cases?
I did not try that since the issue seems to be fixed on today's build. I have also tried to find the regression range and this is what I found:
- with the build from 14th of June, the issue reproduces
http://mozmill-crowd.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1e1afc4d

- with the build from 15th of June, the issue does not reproduce
http://mozmill-crowd.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1e1cc6b3

The changeset is this: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=3d16d59c9317 and contains only one change - fix to bug 809306.

NOTE: I will also like to test this on the remote machines to make sure that the issue is fixed now.
Summary: Test failure "controller.waitForPageLoad(): Timeout waiting for page loaded" in testNewTab.js → Test failure "controller.waitForPageLoad(): Timeout waiting for page loaded" with mozmill 1.5 in testNewTab.js
Tested on the remote machine and this issue did not reproduce in 8 runs:
http://mozmill-crowd.blargon7.com/#/functional/reports?branch=All&platform=Mac&from=2013-06-25&to=2013-06-25

The change here: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=3d16d59c9317 fixed the issue.

So, I think we can unskip this test now.
Ok, lets try then.
(In reply to Henrik Skupin (:whimboo) [away 06/28 - 07/07] from comment #30)
> Ok, lets try then.
It still reproduces with aurora branch and 24.0a2 build for MAC:
http://mozmill-crowd.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1e96e028

Please do not unskip it on either branch. 

I would like to test with Nightly 25.0a1 to see if it really works, but the builds are not up yet.
Backed-out on default for Firefox 25.0:
http://hg.mozilla.org/qa/mozmill-tests/rev/63f84b4a0e16

Lets see how it sticks.
(In reply to Henrik Skupin (:whimboo) [away 06/28 - 07/07] from comment #32)
> Backed-out on default for Firefox 25.0:
> http://hg.mozilla.org/qa/mozmill-tests/rev/63f84b4a0e16
> 
> Lets see how it sticks.
The issue still reproduces with Nightly build 25.0a1:
http://mozmill-ci.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1ea47816

I think we should unskip the test again

Also, I and Andreea tested on Linux 12.04 x86 and x64 and the issue does not reproduce with the build from 21st of June, but it's reproducing with the build from 22nd of June (and all builds afterwards). This is the pushlog, but I am not sure why it caused this issue to happen again:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=cea75ce9a559
Because bug 791670 was re-landed on June 21st... I already mentioned what you can do to fix that in comment #1.
Blocks: 791670
Posted patch patch v2.0 (obsolete) — Splinter Review
Based on previous comment, the changes in bug 791670 caused this issue. 

So, I have changed the preference for preloading to false in the test, not mozmill or the tabs.js library since this is the only one affected by setting the preference to true.
Attachment #760926 - Attachment is obsolete: true
Attachment #767210 - Flags: feedback?(hskupin)

Updated

6 years ago
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][sprint2013-39]
Comment on attachment 767210 [details] [diff] [review]
patch v2.0

Review of attachment 767210 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with the pref. But if this passes in Mozmill 2.0 with our new waitForPageLoad logic, we should only disable it for Mozmill 1.5. Also I would do that globally as best in mozmill 1.5 where we still have to release a new minor release soon.
Attachment #767210 - Flags: feedback?(hskupin) → feedback+
Reskipped it on default until we fix this so we can avoid known failures. On Aurora is already skipped due to the merge.

http://hg.mozilla.org/qa/mozmill-tests/rev/a8d5132f791c (default)
Whiteboard: [mozmill-test-failure][sprint2013-39] → [mozmill-test-failure][sprint2013-39][mozmill-test-skipped]
Comment on attachment 767720 [details] [diff] [review]
patch v1.0 for mozmill 1.5

This is not the right bug to request review for changes in Mozmill. File a separate bug as usual.
Attachment #767720 - Attachment is obsolete: true
Attachment #767720 - Flags: review?(hskupin)

Updated

6 years ago
Depends on: 887258
(Reporter)

Updated

6 years ago
Blocks: 874344
(Assignee)

Updated

6 years ago
Assignee: daniela.p98911 → cosmin.malutan
Comment on attachment 797820 [details] [diff] [review]
patch_v1.0

Review of attachment 797820 [details] [diff] [review]:
-----------------------------------------------------------------

Enabled on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/999b7403857e
Attachment #797820 - Flags: review?(andrei.eftimie)
Attachment #797820 - Flags: review?(andreea.matei)
Attachment #797820 - Flags: review+
Thanks Andreea, 
the patch applies cleanly also on aurora and beta.
(Reporter)

Comment 45

6 years ago
Should we push this back since we are now using 1.5.22 again?

Comment 46

6 years ago
Yes, lets unskip this test.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/59ee46cad0a9 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/5d95d0406961 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/417495b0ca2f (mozilla-beta)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Attachment #797820 - Flags: checkin+
Whiteboard: [mozmill-test-failure][sprint2013-39][mozmill-test-skipped] → [mozmill-test-failure][sprint2013-39]

Comment 47

5 years ago
This test is still disabled on release, esr31 and esr24!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 48

5 years ago
Posted patch unskip.patchSplinter Review
The test itself runs fine. I think it was missed to unskip it when this bug was initially closed.

This unskips the test for release and esr31.
Attachment #8481251 - Flags: review?(andreea.matei)

Comment 49

5 years ago
And I wouldn't worry about esr24 here.
Comment on attachment 8481251 [details] [diff] [review]
unskip.patch

Review of attachment 8481251 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch. Patch looks fine. Lets get it unskipped.
Attachment #8481251 - Flags: review?(andreea.matei) → review+

Comment 51

5 years ago
https://hg.mozilla.org/qa/mozmill-tests/rev/12dff51ed46a (mozilla-release)
https://hg.mozilla.org/qa/mozmill-tests/rev/ff1477c8587f (mozilla-esr31)
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.