Closed Bug 1287723 Opened 6 years ago Closed 6 years ago

Intermittent failure for debug builds (0.002s is too short) | test_screenshot.py Chrome.test_secondary_windows | AssertionError: None != 'png' | after NoSuchWindowException: No such content frame; perhaps the listener was not registered?

Categories

(Testing :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox48 wontfix, firefox49 fixed, firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1281915 +++

This is a follow-up bug for bug 1281915 which is still not fixed for me for debug builds. The used sleep of 0.002s is too less and still causes test failures. 

As a general rule sleep() calls are evil so I will see that we can get rid of it completely.

This failure is permanent for my try push of the patch on bug 1257476 so marking it blocking.
The problem here seems to be that we are not waiting for the window to be fully loaded but want to operate on it immediately. This can certainly be a permafail for debug builds as I can see locally. Waiting for the loaded state should hopefully fix it.
The problem here is with switch_to_window() and that it doesn't check if the chrome window (not sure about tabs yet but those might not be affected because we check the page load state?) has been completed loading, or at least readyState is not `loading`. I feel that `switchToWindow()` in driver.js should really wait for that state before switching to the specified chrome window.

Andreas, what do you think about it?
Flags: needinfo?(ato)
I talked with David. So for this bug we want to put a workaround in the test itself, which is similar to what we do in Firefox Puppeteer - which is looping until the window has been loaded.

I will file a follow-up bug later to get the underlying issue in Marionette server fixed. Andreas, I will ask the same ni? over there again. So please wait with your answer until then. Thanks.
Flags: needinfo?(ato)
Summary: Near permafailing on beta: test_screenshot.py Chrome.test_secondary_windows | AssertionError: None != 'png' | after NoSuchWindowException: NoSuchWindowException: No such content frame; perhaps the listener was not registered? → Intermittent failure for debug builds (0.002s is too short) | test_screenshot.py Chrome.test_secondary_windows | AssertionError: None != 'png' | after NoSuchWindowException: No such content frame; perhaps the listener was not registered?
Depends on: 1288339
Attachment #8773197 - Flags: review?(dburns) → review+
Comment on attachment 8773197 [details]
Bug 1287723 - Fix test_screenshot.py for handling secondary chrome windows.

https://reviewboard.mozilla.org/r/65922/#review62860
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d93b95e192e9
Fix test_screenshot.py to properly wait for new chrome window to be loaded. r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/d93b95e192e9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
This is a test-only change which fixes a race-condition mainly for debug builds. It would be great to have it backported to aurora and beta.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Removing backport request given that this might not have been the right fix. I will investigate more on Monday, also if a backout might be necessary.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
backed out on request from whimboo
Status: RESOLVED → REOPENED
Flags: needinfo?(hskupin)
Resolution: FIXED → ---
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/7c669d5d63ef
Backed out changeset d93b95e192e9 on request from whimboo
The reason for this backout simply is that the patch did not fix the real underlying problem. After doing some more investigation I do not see the correlation of this test as created with the changes made on bug 1140542. In this bug Andreas made changes so that Marionette is able now to create screenshots from chrome windows which do not have a top-level window element. But the test is actually covering the case that a newly opened chrome window has an invalid XUL URL. I see a fundamental difference here given that such an erroneous chrome window is handled differently. 

I would propose that we get this test fixed so we actually test secondary chrome windows which have e.g. a <dialog> or any other arbitrary node as top-level element. The node type should actually not matter because we get our info from `win.document.documentElement` now.

As a follow-up bug we should ensure to correctly handle the initial loading of erroneous chrome windows. Maybe this is already covered by bug 1288339 where I will investigate it in the next weeks.
Flags: needinfo?(hskupin)
Comment on attachment 8773197 [details]
Bug 1287723 - Fix test_screenshot.py for handling secondary chrome windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65922/diff/1-2/
Attachment #8773197 - Attachment description: Bug 1287723 - Fix test_screenshot.py to properly wait for new chrome window to be loaded. → Bug 1287723 - Fix test_screenshot.py for handling secondary chrome windows.
Comment on attachment 8773197 [details]
Bug 1287723 - Fix test_screenshot.py for handling secondary chrome windows.

I have to re-request review given that changes made as described above.
Attachment #8773197 - Flags: review+ → review?(dburns)
Attachment #8773197 - Flags: review?(dburns) → review+
Comment on attachment 8773197 [details]
Bug 1287723 - Fix test_screenshot.py for handling secondary chrome windows.

https://reviewboard.mozilla.org/r/65922/#review63944
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a4de3609f4e
Fix test_screenshot.py for handling secondary chrome windows. r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/4a4de3609f4e
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
This patch works great. No more failures are visible. It would be great if we can get this test-only patch backported to mozilla-aurora. For 48.0 it's too late given that it is already on the mozilla-release branch, and that fix is not that important to get it fixed on that branch. Thanks.
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.