1287723 - 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?

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

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

+++ 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.

Comment 1

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

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.

Comment 2

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

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?

Comment 3

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

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.

Comment 4

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

Attachment: Bug 1287723 - Fix test_screenshot.py for handling secondary chrome windows.

Review commit: https://reviewboard.mozilla.org/r/65922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65922/

Comment 5

[David Burns :automatedtester]

Comment on attachment 8773197 [details]
Bug 1287723 - Fix test_screenshot.py for handling secondary chrome windows.

https://reviewboard.mozilla.org/r/65922/#review62860

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d93b95e192e9
Fix test_screenshot.py to properly wait for new chrome window to be loaded. r=automatedtester

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d93b95e192e9

Comment 8

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

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.

Comment 9

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

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.

Comment 10

[Carsten Book [:Tomcat]]

backed out on request from whimboo

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/7c669d5d63ef
Backed out changeset d93b95e192e9 on request from whimboo

Comment 12

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

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.

Comment 13

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

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/

Comment 14

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

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.

Comment 15

[David Burns :automatedtester]

Comment on attachment 8773197 [details]
Bug 1287723 - Fix test_screenshot.py for handling secondary chrome windows.

https://reviewboard.mozilla.org/r/65922/#review63944

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a4de3609f4e
Fix test_screenshot.py for handling secondary chrome windows. r=automatedtester

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a4de3609f4e

Comment 18

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

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3fc2b0d8fb83

Comment 20

[BMO Automation]

Moving bug to Testing::Marionette Client and Harness component per bug 1815831.