1300709 - Add support for "sessionstore-windows-restored" to Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

I cannot find any reference in Marionette which handles the sessionstore-windows-restored observer notification. As result tests will start too early after a restart of Firefox, and cause various kind of test failures.

It would be good to add this observer notification and expose the status as a variable maybe attached to the driver module.

Comment 1

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

Without this feature we seem to miss a couple of actions Firefox is actually performing during start-up and once all initial windows have been reopened:

https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1138

This seems to be the reason why we also have the test failure for update tests as covered by bug 1274943.

Comment 2

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

I will try to get this implemented now. Actually it shouldn't be that hard.

Comment 3

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

I would need some guidance here in how we actually want to get this implemented. As it looks like the observer is not always fired, so we cannot make it mandatory. But even in cases of restarts there might be cases when we do not want to wait for the session being restarted - those might be rare. But anyway I feel that we need a way to opt-in or opt-out from it. 

IMO we should implemented it in an opt-out way, so not each and every test would have to explicitly wait for it to get started. If we don't do it an initial page load in the first opened browser window could become a no-op because all tabs get replaced by sessionstore. It would be good to know how we could make the wait optional but not sure where we decide in Firefox if a session needs to be restored during startup. Mike, could you give some feedback maybe?

Also there is a way to use "History -> Restore previous session" to restore a session later on. For that I would say that tests which need that have to handle it themselves. So Marionette only cares about the session restored notification during startup.

Andreas, what do you think?

Comment 4

[David Burns :automatedtester]

As a default we don't want Marionette to know about previous sessions so we shouldnt care about waiting for that session.

Since restart tests are special we can add code for those tests to wait and have a way of only adding the observer for those tests.

Comment 5

[Andreas Tolfsen ❲:ato❳]

I don’t know much about sessionstore-windows-restored, but if that is fired when the previous session windows and tabs have been restored it sounds as if we should wait for that to fire before initialising the Marionette session altogether in https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/components/marionettecomponent.js#L140.

If it is only fired when an old session exists and it gets restored, we presumably need to check if this is the case before opting in to wait for the event.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> 
> IMO we should implemented it in an opt-out way, so not each and every test
> would have to explicitly wait for it to get started. If we don't do it an
> initial page load in the first opened browser window could become a no-op
> because all tabs get replaced by sessionstore. It would be good to know how
> we could make the wait optional but not sure where we decide in Firefox if a
> session needs to be restored during startup. Mike, could you give some
> feedback maybe?
> 

I'm not too too familiar with how SessionStore initialization works, but you might want to check out the nsISessionStartup interface:

http://searchfox.org/mozilla-central/source/browser/components/sessionstore/nsISessionStartup.idl

specifically onceInitialized and doRestore, which sound like what you're looking for.

Comment 7

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

This will be fixed as a side-along via my patch series on bug 1322383.

Comment 8

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

The patch moved to bug 1315611.

Comment 9

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

With bug 1315611 fixed we get this feature for free. \o/