Closed Bug 1420864 Opened 7 years ago Closed 4 years ago

Not closing GFX sanity check window prevents Marionette from initialization

Categories

(Remote Protocol :: Marionette, defect, P3)

Product:
Remote Protocol
Component:
Marionette
Version:
56 Branch
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 1572687

People

(Reporter: whimboo, Unassigned)

References

(
URL
)

Details

We have had a couple of reports in the past that the GFX sanity check window doesn't close. The result of that is that Marionette does not initialize at all, and waits forever until the window is gone. One of the recent reports for Firefox 56 can be found here: https://github.com/mozilla/geckodriver/issues/1043 Here the current handling of observer notifications: When "profile-after-change" is received we start listening for "command-line-startup" and "sessionstore-windows-restored". "command-line-startup" is always getting fired and handled correctly. But when "sessionstore-windows-restored" is received we check all open chrome windows for instances of "sanityparent.html", and register for "domwindowclosed" notifications: https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/testing/marionette/components/marionette.js#233-254 Given that we delay the initialization of Marionette until the GFX window has been closed: https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/testing/marionette/components/marionette.js#218-225 Sadly we never reach that code, and hang now. A solution would be to use a timer to escape the wait after eg. 5s or so. I'm not sure which consequences that would have, so I would like to get some feedback from Matt first.
Flags: needinfo?(matt.woodrow)
I also wonder if there was a known problem in Firefox 56, which then got fixed for Firefox 57. Given that 57 works for the reporters but 56 not.
I don't remember, why do we want to wait for it to close? If we add a 5 second timeout, then when we hit, won't we then run into whatever made us want to wait in the first place? Can we just disable the sanity test entirely when we're doing test runs? It's not particularly useful there anyway. Or can we try debug why it's not closing, since it seems possible that it might get stuck open for real users (in normal usage) too.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #2) > I don't remember, why do we want to wait for it to close? If we add a 5 > second timeout, then when we hit, won't we then run into whatever made us > want to wait in the first place? Not waiting for the window returned us the window via the window enumerator, and shortly after it closed itself. Marionette was and still is not able to handle that - at least that early in the process. But I would prefer a warning and failure, compared to a complete hang. > Can we just disable the sanity test entirely when we're doing test runs? > It's not particularly useful there anyway. Let me ask back... Is there a way to disable it? Also I don't know anything about this window to say if the results from it would be necessary / needed by any kind of test in the future. What is it actually doing? > Or can we try debug why it's not closing, since it seems possible that it > might get stuck open for real users (in normal usage) too. We can totally do that, but it would depend on the responsiveness on the users who reported this issue. Given that we have two of them right now, I will do my best to figure out why it is happening.
Flags: needinfo?(matt.woodrow)
(In reply to Henrik Skupin (:whimboo) from comment #3) > > Or can we try debug why it's not closing, since it seems possible that it > > might get stuck open for real users (in normal usage) too. > > We can totally do that, but it would depend on the responsiveness on the > users who reported this issue. Given that we have two of them right now, I > will do my best to figure out why it is happening. Well, I wonder if that is still necessary given that it was a problem in 56 but not in 57. Would you still be interested which bug fixed it? I assume you don't have an idea yet.
(In reply to Henrik Skupin (:whimboo) from comment #3) > > > Can we just disable the sanity test entirely when we're doing test runs? > > It's not particularly useful there anyway. > > Let me ask back... Is there a way to disable it? Not currently, but we could add one. > > Also I don't know anything about this window to say if the results from it > would be necessary / needed by any kind of test in the future. What is it > actually doing? It's testing graphics features to make sure they draw to the screen correctly. It's supposed to catch broken GPU drivers in the wild on weird configurations, and downgrade the users to a known-good software setup. I don't think that adds much value for a test environment. > > Well, I wonder if that is still necessary given that it was a problem in 56 > but not in 57. Would you still be interested which bug fixed it? I assume > you don't have an idea yet. Yes, I'd be interested, and no, I don't have any ideas what it might be.
Flags: needinfo?(matt.woodrow)
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3
I'll happy to work on the ticket.
Blocks: 1530979

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)

Can we just disable the sanity test entirely when we're doing test runs?
It's not particularly useful there anyway.

Let me ask back... Is there a way to disable it?

Not currently, but we could add one.

Matt, this actually blocks me for getting bug 1530979 implemented. Could you please advise or have a patch to allow us to disable the GFX sanity window? Your help would be appreciated. Thanks!

Flags: needinfo?(matt.woodrow)

Sorry for the slow response.

I think the easiest thing to do would be add an early return to shouldRunTest() [1] based on a pref or env var, or whatever the easiest way to detect marionette is.

[1] https://searchfox.org/mozilla-central/rev/f0ef51bfafc8ad0c3a2f523bf076edc57dc4891a/toolkit/components/gfx/SanityTest.jsm#265

Flags: needinfo?(matt.woodrow)

Thanks. I won't have the time in the next weeks, but I will have a look at this bug again after the next all hands.

Another option could actually be to set the sanity-test.running preference to true, by default. Maybe this will already help in most of the cases, whereby others mentioned that it is not always working for them. Nevertheless, I will file a new bug, and get this fixed for Marionette and geckodriver first.

Depends on: 1572687

Was all fixed by bug 1572687.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.