1113060 - ChromeWorker timeout is not stopped when addon is disabled or removed

Status: NEW

(Core :: DOM: Workers, enhancement, P5)

Description

[noitidart]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141211142524

Steps to reproduce:

I created a ChromeWorker which on startup calls a function `queryState`, this function sets timeout to call itself in 5sec.



Actual results:

If i dont put myWorker.terminate() in shutdown proc of my addon the loop goes on


Expected results:

the worker should be terminated as addon was disabled/unisntalled


SIMPLIFIED TEST CASES

Here is my test case without terminate in shutdown. you will see that after you do disable or uninstall addon, the log keeps coming every 5 sec:

This is the test case with terminate in shutdown, you will see after you do disable or unisntall it will stop the logging: https://github.com/yajd/IdleStateLocked/blob/patch-1/bootstrap.js#L47

Comment 1

[noitidart]

Attachment: IdleStateLocked@jetpack WITH TERMINATE.xpi

with terminate in shutdown. after disable/uninstall of addon the timeout stops running, this is seen as the log stops logging "debug-timeout-test"

Comment 2

[noitidart]

Attachment: IdleStateLocked@jetpack WITHOUT terminate.xpi

Comment 3

[noitidart]

Comment on attachment 8538410 [details]
IdleStateLocked@jetpack WITHOUT terminate.xpi


this one does not have terminate in shutdown. you will see browser console will continue to get "debug-timeout-test" logged

Comment 4

[noitidart]

note: this one is obvious but just pointing out: because the log message is the same, the counter will increase every 5sec as it logs. in browser console.

Comment 5

[noitidart]

If there was someway to like addEventListener on chrome shutdown it would be nice.

Because in Linux X11 we have to XOpenDisplay and then when we're done with it we have to XCloseDisplay.

If we can get like self.addEventListener('shutdown', ....

That would be fantastic.

How are things done right now? Normally shutdown stuff is done via unloaders but ChromeWorkers don't have access to the bootstrap unloader

Comment 6

[u279076]

I think this belongs in DOM:Events - please fix if you disagree.

Comment 7

[:Gijs (he/him)]

Can we enumerate workers somehow? If not, how is the add-on manager meant to figure out which workers are running script from an add-on that is being disabled?

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to :Gijs Kruitbosch from comment #7)
> Can we enumerate workers somehow? If not, how is the add-on manager meant to
> figure out which workers are running script from an add-on that is being
> disabled?

No, there is no mechanism to enumerate running workers. If an addon makes a ChromeWorker it needs to remember and shut it down itself when it gets unloaded.

Comment 9

[:Gijs (he/him)]

Are these jetpack add-ons? The IDs sort of imply that. How are you creating the chrome worker?

Comment 10

[noitidart]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > Can we enumerate workers somehow? If not, how is the add-on manager meant to
> > figure out which workers are running script from an add-on that is being
> > disabled?
> 
> No, there is no mechanism to enumerate running workers. If an addon makes a
> ChromeWorker it needs to remember and shut it down itself when it gets
> unloaded.

To shutdown a chrome worker the only way is .terminate? So in shutdown proc of bootstrap addon we would .terminate on all our workers which we had to keep track of in an array?

Is there like a self.beforeTerminate = function() {} (or a self.shutdown=func) in chromeworker scope so we can do like XCloseDisplay in there?

Comment 11

[noitidart]

(In reply to :Gijs Kruitbosch from comment #9)
> Are these jetpack add-ons? The IDs sort of imply that. How are you creating
> the chrome worker?

Oh I just like to add @jetpack to my id because it sounds so cool :P

Its regular bootstrap addon.
I create ChromeWorker with: https://github.com/yajd/IdleStateLocked/blob/patch-1/bootstrap.js#L19

chromeWorker_pollLockedState = new ChromeWorker(self.path.workers + 'pollLockedState.js');

I guess I'll add to the line after this: unload(function() { chromeWorker_pollLockedSate.terminate() }


and if i want to run a shutdown thing before i terminate it then ill do:

unload(function() {
 chromeWorker_pollLockedSatel.postMessage({aTopic:'shutdown'});
 chromeWorker_pollLockedSate.terminate() }
});

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to noitidart from comment #10)
> To shutdown a chrome worker the only way is .terminate?

Workers will shut themselves down if they are GC'd *and* there is no code "running" in the worker. Since you use setTimeout that means the worker is "running" until the timeout finishes. If you do a setInterval then the worker will never go away since it is always "running". Calling terminate() on the worker kills it for sure.

> Is there like a self.beforeTerminate = function() {} (or a
> self.shutdown=func) in chromeworker scope so we can do like XCloseDisplay in
> there?

Yes, there is a non-standard "close" event that is fired at the worker global scope before the worker is killed. Either use self.addEventListener() or set self.onclose = function(event) { ... } to use it.

Comment 13

[noitidart]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #12)
> (In reply to noitidart from comment #10)
> > Is there like a self.beforeTerminate = function() {} (or a
> > self.shutdown=func) in chromeworker scope so we can do like XCloseDisplay in
> > there?
> 
> Yes, there is a non-standard "close" event that is fired at the worker
> global scope before the worker is killed. Either use self.addEventListener()
> or set self.onclose = function(event) { ... } to use it.

Thanks brent. It looks like self.onclose and self.addEventListener('close',.. are not documented here: https://developer.mozilla.org/en-US/docs/Web/API/Worker

By non-standard do you mean they are not available to Worker but only to ChromeWorker? If that's the case then Ill add documentation for it here: https://developer.mozilla.org/en-US/docs/Web/API/ChromeWorker

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to noitidart from comment #13)
> By non-standard do you mean they are not available to Worker but only to
> ChromeWorker?

No, I mean that it's not part of the official DOM Worker specification, so it doesn't exist in any other browsers.

Comment 15

[noitidart]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #14)
> (In reply to noitidart from comment #13)
> > By non-standard do you mean they are not available to Worker but only to
> > ChromeWorker?
> 
> No, I mean that it's not part of the official DOM Worker specification, so
> it doesn't exist in any other browsers.

Oh so even Workers have this close feature but only in firefox. So it would make more sense to add onclose and close to here: https://developer.mozilla.org/en-US/docs/Web/API/Worker with a note saying they are non-standard (not availabe in workers in other browsers)?

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I wouldn't bother, we may want to remove it some day.

Comment 17

[noitidart]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #12)
> (In reply to noitidart from comment #10)
> > To shutdown a chrome worker the only way is .terminate?
> 
> Workers will shut themselves down if they are GC'd *and* there is no code
> "running" in the worker. Since you use setTimeout that means the worker is
> "running" until the timeout finishes. If you do a setInterval then the
> worker will never go away since it is always "running". Calling terminate()
> on the worker kills it for sure.
> 

So using a `setInterval` would be a bug then correct? Or is it expected people understand this? Just wondering it's one of these two becuase its not mentioned in the documentation.

I always thought that on shutdown of addon ChromeWorkers are sthudown, and all ctypes libraries loaded with `ctypes.open('....')` get closed automatically, is this true? Or do i have to do `self.onclose = function() { /* itterate to library.close() */ }`?

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to noitidart from comment #17)
> So using a `setInterval` would be a bug then correct?

There's nothing inherently wrong with using setInterval, it just means that the worker will live as long as the timer is running.

> Or is it expected people understand this?

It's not ordinarily a problem because workers are force-closed when a user navigates away from the page that created the worker. Addons that create workers don't get automatic lifetime restrictions like that, so you have to be a little more careful.

> Or do i have to do `self.onclose = function() { /* itterate to library.close() */ }`?

I don't know for sure, I would suggest that you test it. But I would expect that if the worker is closed properly (e.g. either via GC or terminate()) then the ctypes code will clean up appropriately.

Comment 19

[noitidart]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #18)
> (In reply to noitidart from comment #17)
> > So using a `setInterval` would be a bug then correct?
> 
> There's nothing inherently wrong with using setInterval, it just means that
> the worker will live as long as the timer is running.
> 
> > Or is it expected people understand this?
> 
> It's not ordinarily a problem because workers are force-closed when a user
> navigates away from the page that created the worker. Addons that create
> workers don't get automatic lifetime restrictions like that, so you have to
> be a little more careful.
> 
> > Or do i have to do `self.onclose = function() { /* itterate to library.close() */ }`?
> 
> I don't know for sure, I would suggest that you test it. But I would expect
> that if the worker is closed properly (e.g. either via GC or terminate())
> then the ctypes code will clean up appropriately.

Thanks brent. I verified that its not just setInterval but allso recursive functions that will keep the ChromeWorker going. See this here uses setTimeout to keep the function going: https://github.com/yajd/IdleStateLocked/blob/patch-1/modules/workers/pollLockedState.js

Ok ill try to test it and figure out some quirks to add to the documentation, it would be nice to have the info out there for addon scope, as addons can be big cause of poor perf.