Closed Bug 1300709 Opened 6 years ago Closed 5 years ago

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

Categories

(Testing :: Marionette, defect)

defect
Not set
major

Tracking

(firefox-esr52 wontfix, firefox53 fixed, firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: pi-marionette-server)

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.
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.
Blocks: 1274943
Severity: normal → major
I will try to get this implemented now. Actually it shouldn't be that hard.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
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?
Flags: needinfo?(mconley)
Flags: needinfo?(ato)
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.
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.
Flags: needinfo?(ato)
No longer blocks: 1299216
(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.
Flags: needinfo?(mconley)
This will be fixed as a side-along via my patch series on bug 1322383.
Depends on: 1322383
The patch moved to bug 1315611.
Depends on: 1315611
No longer depends on: 1322383
With bug 1315611 fixed we get this feature for free. \o/
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.