1037970 - Using tab.open() in add-on startup causes an gBrowser is undefined error

Status: RESOLVED DUPLICATE of bug 1042239

(Add-on SDK Graveyard :: General, defect, P1)

Description

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

For more details see https://github.com/mozilla/jpm/issues/93

1. create a jpm-based add-on that tried to open windows on start
1. run it via jpm -b -v run

Error: https://gist.github.com/canuckistani/c813b2a7169ef8e06762

Delaying by ~200msec on a Macbook Air seems to fix this. Test code is here:

https://github.com/canuckistani/dump-bugs

Comment 1

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

We should throw a better error here if one tries to use `tabs.open()` and there are no windows open, or opening a new window isn't a bad option either I think.  To prevent this from happening during startup though I think it is reasonable to wait for the first window to open before starting jetpacks, since we are already forcing all jetpacks to wait for a hiddenWindow to load (for the sdk/addon/window module).

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

IMO this issue should block releasing jpm.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

It sounds like a bug in the bootstrap.js that comes from JPM, since add-on runner waits for `sessionstore-windows-restored` notification before running an add-on, at which point opening tabs should not cause any errors.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Unless of course this assumption here is wrong:
https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js#L84-L95

It could be that JPM finishes to bootstrap when `hiddenDOMWindow` is already present but `sessionstore-windows-restored` has not yet fired, in which case we need to find a better way to find out if sessionstore has already restored widows or not.

Comment 5

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

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #4)
> Unless of course this assumption here is wrong:
> https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js#L84-
> L95
> 
> It could be that JPM finishes to bootstrap when `hiddenDOMWindow` is already
> present but `sessionstore-windows-restored` has not yet fired, in which case
> we need to find a better way to find out if sessionstore has already
> restored widows or not.

Dave any ideas on better ways to know what state the application startup is in? (ie done or not)

Comment 6

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #4)
> > Unless of course this assumption here is wrong:
> > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js#L84-
> > L95
> > 
> > It could be that JPM finishes to bootstrap when `hiddenDOMWindow` is already
> > present but `sessionstore-windows-restored` has not yet fired, in which case
> > we need to find a better way to find out if sessionstore has already
> > restored widows or not.
> 
> Dave any ideas on better ways to know what state the application startup is
> in? (ie done or not)

Generally you just wait for whatever event you care about to say that everything is ready. Doesn't the bootstrap script already wait for sessionstore-windows-restored? That would mean that all windows are opened, including the hidden window but you might have to wait for them to finish loading, document.readyState should be your friend there.

Comment 7

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

(In reply to Dave Townsend [:mossop] from comment #6)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> > (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> > #4)
> > > Unless of course this assumption here is wrong:
> > > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js#L84-
> > > L95
> > > 
> > > It could be that JPM finishes to bootstrap when `hiddenDOMWindow` is already
> > > present but `sessionstore-windows-restored` has not yet fired, in which case
> > > we need to find a better way to find out if sessionstore has already
> > > restored widows or not.
> > 
> > Dave any ideas on better ways to know what state the application startup is
> > in? (ie done or not)
> 
> Generally you just wait for whatever event you care about to say that
> everything is ready. Doesn't the bootstrap script already wait for
> sessionstore-windows-restored? That would mean that all windows are opened,
> including the hidden window but you might have to wait for them to finish
> loading, document.readyState should be your friend there.

How do we know that the application hasn't finished starting up? so that we know to listen to "sessionstore-windows-restored" otherwise if we try to listen to it after it fired the add-ons won't start, because they'll wait for "sessionstore-windows-restored" forever, or do I have something wrong here?

When add-ons are run with `jpm run` or `cfx run` the `reason` (converted from the reasonCode) provided to `startup` is "install", so atm the these add-ons don't wait for the "sessionstore-windows-restored" event which is likely causing this issue.  When the reason is "startup" they listen for "sessionstore-windows-restored" and afaik that use case works.

So I'm wondering if there is some way to know that we are in a "startup" & "install" use case, when the reason we get is only "install"?

Comment 8

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> (In reply to Dave Townsend [:mossop] from comment #6)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> > > (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> > > #4)
> > > > Unless of course this assumption here is wrong:
> > > > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js#L84-
> > > > L95
> > > > 
> > > > It could be that JPM finishes to bootstrap when `hiddenDOMWindow` is already
> > > > present but `sessionstore-windows-restored` has not yet fired, in which case
> > > > we need to find a better way to find out if sessionstore has already
> > > > restored widows or not.
> > > 
> > > Dave any ideas on better ways to know what state the application startup is
> > > in? (ie done or not)
> > 
> > Generally you just wait for whatever event you care about to say that
> > everything is ready. Doesn't the bootstrap script already wait for
> > sessionstore-windows-restored? That would mean that all windows are opened,
> > including the hidden window but you might have to wait for them to finish
> > loading, document.readyState should be your friend there.
> 
> How do we know that the application hasn't finished starting up? so that we
> know to listen to "sessionstore-windows-restored" otherwise if we try to
> listen to it after it fired the add-ons won't start, because they'll wait
> for "sessionstore-windows-restored" forever, or do I have something wrong
> here?
> 
> When add-ons are run with `jpm run` or `cfx run` the `reason` (converted
> from the reasonCode) provided to `startup` is "install", so atm the these
> add-ons don't wait for the "sessionstore-windows-restored" event which is
> likely causing this issue.  When the reason is "startup" they listen for
> "sessionstore-windows-restored" and afaik that use case works.

I guess see the discussion in bug 854937. I'm still not sure if that was the right fix after all. We could easily add a new flag to the data passed to bootstrap saying what phase the script is being called from. You might try looking at nsIAppStartup.startingUp. I don't know when that is flipped

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)
> > (In reply to Dave Townsend [:mossop] from comment #6)
> > > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> > > > (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> > > > #4)
> > > > > Unless of course this assumption here is wrong:
> > > > > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js#L84-
> > > > > L95
> > > > > 
> > > > > It could be that JPM finishes to bootstrap when `hiddenDOMWindow` is already
> > > > > present but `sessionstore-windows-restored` has not yet fired, in which case
> > > > > we need to find a better way to find out if sessionstore has already
> > > > > restored widows or not.
> > > > 
> > > > Dave any ideas on better ways to know what state the application startup is
> > > > in? (ie done or not)
> > > 
> > > Generally you just wait for whatever event you care about to say that
> > > everything is ready. Doesn't the bootstrap script already wait for
> > > sessionstore-windows-restored? That would mean that all windows are opened,
> > > including the hidden window but you might have to wait for them to finish
> > > loading, document.readyState should be your friend there.
> > 
> > How do we know that the application hasn't finished starting up? so that we
> > know to listen to "sessionstore-windows-restored" otherwise if we try to
> > listen to it after it fired the add-ons won't start, because they'll wait
> > for "sessionstore-windows-restored" forever, or do I have something wrong
> > here?
> > 
> > When add-ons are run with `jpm run` or `cfx run` the `reason` (converted
> > from the reasonCode) provided to `startup` is "install", so atm the these
> > add-ons don't wait for the "sessionstore-windows-restored" event which is
> > likely causing this issue.  When the reason is "startup" they listen for
> > "sessionstore-windows-restored" and afaik that use case works.
> 
> I guess see the discussion in bug 854937. I'm still not sure if that was the
> right fix after all. We could easily add a new flag to the data passed to
> bootstrap saying what phase the script is being called from. You might try
> looking at nsIAppStartup.startingUp. I don't know when that is flipped

Just gave that a try, it switches too soon, at "final-ui-startup", which is before "sessionstore-windows-restored"

Comment 10

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

(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)
> > > (In reply to Dave Townsend [:mossop] from comment #6)
> > > > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> > > > > (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> > > > > #4)
> > > > > > Unless of course this assumption here is wrong:
> > > > > > https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/addon/runner.js#L84-
> > > > > > L95
> > > > > > 
> > > > > > It could be that JPM finishes to bootstrap when `hiddenDOMWindow` is already
> > > > > > present but `sessionstore-windows-restored` has not yet fired, in which case
> > > > > > we need to find a better way to find out if sessionstore has already
> > > > > > restored widows or not.
> > > > > 
> > > > > Dave any ideas on better ways to know what state the application startup is
> > > > > in? (ie done or not)
> > > > 
> > > > Generally you just wait for whatever event you care about to say that
> > > > everything is ready. Doesn't the bootstrap script already wait for
> > > > sessionstore-windows-restored? That would mean that all windows are opened,
> > > > including the hidden window but you might have to wait for them to finish
> > > > loading, document.readyState should be your friend there.
> > > 
> > > How do we know that the application hasn't finished starting up? so that we
> > > know to listen to "sessionstore-windows-restored" otherwise if we try to
> > > listen to it after it fired the add-ons won't start, because they'll wait
> > > for "sessionstore-windows-restored" forever, or do I have something wrong
> > > here?
> > > 
> > > When add-ons are run with `jpm run` or `cfx run` the `reason` (converted
> > > from the reasonCode) provided to `startup` is "install", so atm the these
> > > add-ons don't wait for the "sessionstore-windows-restored" event which is
> > > likely causing this issue.  When the reason is "startup" they listen for
> > > "sessionstore-windows-restored" and afaik that use case works.
> > 
> > I guess see the discussion in bug 854937. I'm still not sure if that was the
> > right fix after all. We could easily add a new flag to the data passed to
> > bootstrap saying what phase the script is being called from. You might try
> > looking at nsIAppStartup.startingUp. I don't know when that is flipped
> 
> Just gave that a try, it switches too soon, at "final-ui-startup", which is
> before "sessionstore-windows-restored"

ok found something that works! nsISessionStartup.onceInitialized \o/

Comment 11

[Dave Townsend [:mossop]]

Sure, but is nsIAppStartup.startingUp true or false when startup is called during application startup? Looking at the code I think it should be true.

Comment 12

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

(In reply to Dave Townsend [:mossop] from comment #11)
> Sure, but is nsIAppStartup.startingUp true or false when startup is called
> during application startup? Looking at the code I think it should be true.

So by the time `require("sdk/addon/runner").startup()` is called `nsIAppStartup.startingUp` can be `true` or `false`, it's a race condition since we read the `harness-options.json` or `package.json` asynchronously.

If we want `require("sdk/addon/runner")` to be a module that always works, then we'll have to write a patch that doesn't involve modifying the `bootstrap.js`.

If we modified the bootstrap.js file to check `nsIAppStartup.startingUp` when it's `startup()` function is called, then we'll have to track whether or not "sessionstore-windows-restored" has fired in the bootstrap.js as well, then pass a boolean to `require("sdk/addon/runner").startup()` to indicate if "sessionstore-windows-restored" has fired yet or not, so that addon/runner knows to wait for it or not.  Or we can continue to use addon/runner in a 'doesn't always work state, and move the code that waits for "sessionstore-windows-restored" into the bootstrap.js and remove it from addon/runner.