754225 - Sync window handling and waitForPageLoad() code with windowMap

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

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

By using the 'outer-window-destroyed' observer notification the window entry gets removed too early from the windowMap. That causes the following failure if another window gets opened right after and e.g. a keypress action is synthesized:

ERROR | Test Failure: {"exception": {"message": "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendKeyEvent]", "name": "NS_ERROR_FAILURE", "lineNumber": 339}}

That means that our handleWindow code grabbed the old chrome window, which then has been destroyed some milliseconds later.

Further you can blame me but I missed the 'Requires Gecko 2.0' flag on MDN which means with 1.5.12 we have silently dropped support for Firefox 3.6 which is bad. Given that we should release a Mozmill 1.5.13 version.

Comment 1

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

So the big issue here is that I can't find a replacement for 'outer-window-destroyed' and outerWindowId which works with Firefox < 4. 

We could make use of 'dom-window-destroyed' which also works but on the other side gets fired too quickly. So in a lot of cases the waitFor loop in waitForPageLoaded() doesn't notice the change before the window gets closed. So it waits forever. :(

The only sane way I can see here is to use callbacks in the windowMap update/remove methods which will notify consumers about updates. I got a prototype working but its still failing for some tests in a row. Probably because of the bfcache. Lets see if I can figure this out today.

But I'm not how safely we can/want this implemented for Mozmill 1.5. Personally I would say people who test 3.6 should use an older version of Mozmill. In this case 1.5.11. But I'm kinda sad about dropping the version support for 3.6 that way. :/

Clint, what would you say? Keep it as it is right now for 1.5 and do no more fix for 1.5 via this bug and fully concentrate on a more robust method for 2.0?

Comment 2

[cmtalbert]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> But I'm not how safely we can/want this implemented for Mozmill 1.5.
> Personally I would say people who test 3.6 should use an older version of
> Mozmill. In this case 1.5.11. But I'm kinda sad about dropping the version
> support for 3.6 that way. :/
> 
> Clint, what would you say? Keep it as it is right now for 1.5 and do no more
> fix for 1.5 via this bug and fully concentrate on a more robust method for
> 2.0?
I'm sad that we dropped support for 3.6 this way, but it was getting so hard to maintain that support, that it was bound to happen at some point. I don't blame you at all. 3.6 is about to be unsupported across the board (or already is) so, I think yes, we just need to write a post/blog that 1.5.12 no longer supports 3.6,and if you need 3.6, you have to use 1.5.11.  It's sad, but it's the end of an era, and honestly, I'm really glad to see the Gecko 1.9.2 codebase get EOL'd.

And I agree, let's just concentrate all our efforts on 2.0.

Comment 3

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

Thanks Clint for the quick response. It makes me happy that you think the same way. I will write a blog post on Monday.

Anthony, the above means that you wouldn't be able to run ondemand update tests for 1.9.2 -> 12 builds on our Jenkins instance. If you have to do those, please use the old method on qa-set. Thanks. I will also go ahead and remove the 1.9.2 test jobs in our Jenkins instance, if you don't mind.

I will investigate possible solutions how to handle this as best in Mozmill 2.0 on Monday.

Comment 4

[u279076]

That's fine, thanks for the heads up. I've been doing the 1.9.2 update testing manually anyway since it's all automatic with prompting now, and our automation does not test that scenario.

Comment 5

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

The first thing we have to be clear about are all the possible states we would have to take care of for waitForPageLoad(). So lets try to get those collected so I can create appropriate tests:

* tab has already been loaded -> return immediately

* tab is currently loading -> wait for loaded

* tab has finished loading and window is closed -> we currently miss the load event due to the 100ms delay in waitForPageLoaded() and wait until timeout

* tab is about to reload -> not sure if we can catch this outside of controller.open() when tests themselves trigger a reload. But from inside we can use a flag which let us wait and not bail out too early before the former page has been unloaded.

* tab is getting loaded from bfcache -> that should work due to the pagehide/pageshow events

* tab/Window is restored from session restore -> that's bug 753763

* frame content gets loaded -> same as above

* redirect when loading a page -> not sure if we handle that correctly atm

* window has been opened -> Should work fine

* window is closed -> we can also miss the loaded event due to the delay in waitFor() if it gets closed too quickly.

* modal dialog open/close -> it's own module and outside of Mozmill at the time.

Comment 6

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

As discussed we want to do the following:

* move the window map to mozmill.js and leave a deprecated message behind in controller.js

* add feature support for callbacks whenever an entry in the window map gets changed

* add a new method to the controller object to quickly access the window values out of the map

* Update waitForPageLoaded() and other instances to work with the callbacks


I will start working on it by next week.

Comment 7

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

As long as we don't see failures I don't see a need to get this fixed on the 1.5 branch.

Comment 8

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

This bug will not be taken for version 2.1. We might want to re-evaluate for 2.2.

Comment 9

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

Mozmill will reach its end of life soon. We are currently working on getting
all the tests for Firefox ported to Marionette. For status updates please see
bug 1080766.