1042239 - There should be a promise that is resolved when all of the sessionstore-windows-restored observers have done their work

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

As is done with `gOnceInitializedDeferred` from:

http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStartup.js#217

on desktop.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/39

Hey Margaret,

Do you have some idea how I can test write a test for this on mobile?

Comment 2

[:Margaret Leibovic]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #1)
> Created attachment 8460614 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/39
> 
> Hey Margaret,
> 
> Do you have some idea how I can test write a test for this on mobile?

I think you should be able to write a JavaScriptTest to test this. Here's an example that tests a .jsm:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testHomeProvider.java
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testHomeProvider.js

The only issue I could see is if this SessionStore code runs before the test code.

This is part of our robocop test framework. Here's instructions on how to run the test:
https://wiki.mozilla.org/Auto-tools/Projects/Robocop#Running_tests

Comment 3

[:Margaret Leibovic]

Comment on attachment 8460614 [details] [review]
Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/39

I left some comments on the pull request. My main concern is that I would like us to avoid depending on add-on SDK modules in core Fennec code, so I'd like to find a way to avoid importing the Startup module in SessionStore.js.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8460614 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/39
> 
> I left some comments on the pull request. My main concern is that I would
> like us to avoid depending on add-on SDK modules in core Fennec code, so I'd
> like to find a way to avoid importing the Startup module in SessionStore.js.

So I'd like to have a solution that can work on all applications, do you have another suggestion?

With this patch application xyz can support jetpacks by importing Startup.js and calling Startup.init() before startup has completed.

If we do a custom solution for mobile, then other applications will have to do there own custom implementations too, and patch the addon-sdk for support.

Dave or Irakli do you have any suggestions here?

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > Comment on attachment 8460614 [details] [review]
> > Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/39
>
> If we do a custom solution for mobile, then other applications will have to
> do there own custom implementations too, and patch the addon-sdk for support.

their*

Comment 6

[:Margaret Leibovic]

I was thinking we could just create a new observer service notification where you currently put Startup.init(), but I suppose the issue there is that you want to be loading your module to listen for a different notification, so that doesn't actually fix your problem :/

When does add-on code get loaded? Is there a way to just make sure that this Startup module is loaded (and the observer added) before SessionStore.js is loaded?

I think we need to come to a solution where we're only loading this module if there's an add-on that depends on it.

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to :Margaret Leibovic from comment #6)
> I was thinking we could just create a new observer service notification
> where you currently put Startup.init(), but I suppose the issue there is
> that you want to be loading your module to listen for a different
> notification, so that doesn't actually fix your problem :/

This could be part of the solution, I'd have to load the jsm to listen for this notification somehow though like you say.  There must be a place to do this though, I just don't know where at the moment.  Perhaps someone here knows who to ask?

> When does add-on code get loaded? Is there a way to just make sure that this
> Startup module is loaded (and the observer added) before SessionStore.js is
> loaded?

Restartless add-ons seem to be able to load before startup occurs, and they can load afterwards, there is race condition here which jetpacks need to avoid to work (as they work now, by depending on the sdk/addon/window module).

I don't see a useful way to know if startup has already occurred or not (across applications like mobile, Fx, thunderbird, or whatever else), which would eliminate the need for the module in my patch.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> I don't see a useful way to know if startup has already occurred or not
> (across applications like mobile, Fx, thunderbird, or whatever else), which
> would eliminate the need for the module in my patch.

Thought I had mentioned it somewhere else, but nsIAppStartup.startingUp is true when bootstrap scripts are called during startup and false when they are called later.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #8)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> > I don't see a useful way to know if startup has already occurred or not
> > (across applications like mobile, Fx, thunderbird, or whatever else), which
> > would eliminate the need for the module in my patch.
> 
> Thought I had mentioned it somewhere else, but nsIAppStartup.startingUp is
> true when bootstrap scripts are called during startup and false when they
> are called later.

Yes you mentioned it in bu 1037970 comment 8, and I responded in bug 1037970 comment 9.

This won't work for us afaict, because it switches sooner than would be useful for our use case.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #9)
> (In reply to Dave Townsend [:mossop] from comment #8)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> > > I don't see a useful way to know if startup has already occurred or not
> > > (across applications like mobile, Fx, thunderbird, or whatever else), which
> > > would eliminate the need for the module in my patch.
> > 
> > Thought I had mentioned it somewhere else, but nsIAppStartup.startingUp is
> > true when bootstrap scripts are called during startup and false when they
> > are called later.
> 
> Yes you mentioned it in bu 1037970 comment 8, and I responded in bug 1037970
> comment 9.
> 
> This won't work for us afaict, because it switches sooner than would be
> useful for our use case.

I disagree. It switches after add-ons are loaded, so if in startup() it is true you can assume you have to wait for sessionstore, if it's false you don't. There might be a small edge case where an add-on could be enabled between final-ui-startup and sessionstore completion, but it isn't something that could happen through the UI so it would only be something triggered by some other add-on and I don't even know if it would be possible.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #10)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #9)
> > (In reply to Dave Townsend [:mossop] from comment #8)
> > > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> > > > I don't see a useful way to know if startup has already occurred or not
> > > > (across applications like mobile, Fx, thunderbird, or whatever else), which
> > > > would eliminate the need for the module in my patch.
> > > 
> > > Thought I had mentioned it somewhere else, but nsIAppStartup.startingUp is
> > > true when bootstrap scripts are called during startup and false when they
> > > are called later.
> > 
> > Yes you mentioned it in bu 1037970 comment 8, and I responded in bug 1037970
> > comment 9.
> > 
> > This won't work for us afaict, because it switches sooner than would be
> > useful for our use case.
> 
> I disagree. It switches after add-ons are loaded, so if in startup() it is
> true you can assume you have to wait for sessionstore, if it's false you
> don't. There might be a small edge case where an add-on could be enabled
> between final-ui-startup and sessionstore completion, but it isn't something
> that could happen through the UI so it would only be something triggered by
> some other add-on and I don't even know if it would be possible.

I didn't notice that you mentioned this in bug 1037970, I responded there in bug 1037970 comment 12.

In short the `sdk/addon/runner` module can be called when `nsIAppStartup.startingUp` is true or false, because it can be called asynchronously.  So we need some way for the module to know if "sessionstore-windows-restored" has fired without modifying the bootstrap.js, so that the addon/runner module always works.

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

I think I have a solution that'll work for now, if we include the `Startup.js` in jetpack bootstraps then it will only load when an add-on needs it, and I won't touch the Session Store code.

Comment 13

[:Margaret Leibovic]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #12)
> I think I have a solution that'll work for now, if we include the
> `Startup.js` in jetpack bootstraps then it will only load when an add-on
> needs it, and I won't touch the Session Store code.

That would be excellent!

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/40/files

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8464760 [details]
Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/40/files

Nice!

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=b35538b87c80

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/89ab761e5816

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89ab761e5816

Comment 19

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/bbb2e02d9e448fa2a12f8f4d1c56eeeac3eebe85
Bug 1042239 creating a promise which resolves when the system has started r=Mossop