Closed Bug 879752 Opened 12 years ago Closed 11 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

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

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox23 --- unaffected
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed
firefox31 --- wontfix
firefox32 --- fixed
firefox-esr24 --- wontfix
firefox-esr31 --- fixed

People

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

References

(
URL
)

Details

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

Attachments

(4 files, 8 obsolete files)

Skip patch
1.46 KB, patch
davehunt
: review+
davehunt
: checkin+
Details | Diff | Splinter Review
logs without waitFor
16.86 KB, text/plain
Details
patch_v1.0
1.54 KB, patch
AndreeaMatei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
unskip.patch
806 bytes, patch
whimboo
: review+
Details | Diff | Splinter Review
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
No longer depends on: 791670
status-firefox24: --- → affected
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
Attached patch Skip patch — — Splinter 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.
Attached 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.
(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.
Attached 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.
status-firefox23: --- → unaffected
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
Attached 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.
Attached 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)
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)
Attached file logs without waitFor —
(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.
Attached 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.
status-firefox25: --- → affected
(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
Attached 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)
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)
status-firefox24: affected → disabled
status-firefox25: affected → disabled
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)
Depends on: 887258
Blocks: 874344
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.
Should we push this back since we are now using 1.5.22 again?
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
Closed: 12 years ago
status-firefox24: disabled → fixed
status-firefox25: disabled → fixed
status-firefox26: disabled → fixed
Resolution: --- → FIXED
Attachment #797820 - Flags: checkin+
Whiteboard: [mozmill-test-failure][sprint2013-39][mozmill-test-skipped] → [mozmill-test-failure][sprint2013-39]
This test is still disabled on release, esr31 and esr24!
Status: RESOLVED → REOPENED
status-firefox31: --- → wontfix
status-firefox32: --- → disabled
status-firefox-esr24: --- → disabled
status-firefox-esr31: --- → disabled
Resolution: FIXED → ---
Attached patch unskip.patch — — Splinter 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)
And I wouldn't worry about esr24 here.
status-firefox-esr24: disabled → wontfix
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+
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
Closed: 12 years ago → 11 years ago
status-firefox32: disabled → fixed
status-firefox-esr31: disabled → fixed
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: