ChromeWorker timeout is not stopped when addon is disabled or removed

NEW
Unassigned

Status

()

P5
enhancement
4 years ago
9 months ago

People

(Reporter: noitidart, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Created attachment 8538407 [details]
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"
(Reporter)

Comment 2

4 years ago
Created attachment 8538410 [details]
IdleStateLocked@jetpack WITHOUT terminate.xpi
(Reporter)

Comment 3

4 years ago
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
Attachment #8538410 - Attachment description: this one does not have terminate in shutdown. you will see browser console will continue to get "debug-timeout-test" logged → IdleStateLocked@jetpack WITHOUT terminate.xpi
(Reporter)

Comment 4

4 years ago
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.

Updated

4 years ago
Component: Untriaged → DOM
Product: Firefox → Core
(Reporter)

Comment 5

4 years ago
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
I think this belongs in DOM:Events - please fix if you disagree.
Component: DOM → DOM: Events

Comment 7

4 years ago
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?
Component: DOM: Events → DOM: Workers

Updated

4 years ago
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: 35 Branch → Trunk
(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

4 years ago
Are these jetpack add-ons? The IDs sort of imply that. How are you creating the chrome worker?
Flags: needinfo?(noitidart)
(Reporter)

Comment 10

4 years ago
(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?
(Reporter)

Comment 11

4 years ago
(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() }
});
Flags: needinfo?(noitidart)
(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.
(Reporter)

Comment 13

4 years ago
(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
(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.
(Reporter)

Comment 15

4 years ago
(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)?
I wouldn't bother, we may want to remove it some day.
(Reporter)

Comment 17

4 years ago
(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() */ }`?
(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.
(Reporter)

Comment 19

4 years ago
(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.

Updated

9 months ago
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.