666335 - Content-script code still executing after worker is detached

Status: RESOLVED FIXED

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

Description

[Nicolas Côté-Nolin]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.57 Safari/534.24
Build Identifier: addon-sdk-1.0

I have reason to believe there is still a leak in content-script workers. When a function is called by setInterval, the code of that function is still executed after the worker is unloaded. The following 2 snippets demonstrate the situation :

tabs.on('ready', function(tab) {
		
    if (!(tab.url === "http://localhost/")) {
	return;
    }
		
    var worker = tab.attach({
        contentScript: 'setInterval(function() { self.postMessage("Test!");}, 2000);',
        onMessage: function(message) {
            console.log(message);
        },
        onDetach: function() {
            console.log("worker detached");
        }
    });
});

----

pageMod.PageMod({
    include: "http://localhost/",
    contentScriptWhen: 'ready',
    contentScript: 'setInterval(function() { self.postMessage("Test!");}, 2000);',
    onAttach: function onAttach(worker) {
        worker.on('message', function(message) {
            console.log(message);
        });
        worker.on('detach', function() {
            console.log("worker detached");
        });
    }
});




Reproducible: Always

Steps to Reproduce:
1. Open firefox
2. Go to http://localhost/
3. Open a second tab, go to google.com
4. Close the 1st tab (localhost).

Actual Results:  
Output in console :
info: Test!
info: Test!
info: Test!
info: worker detached
info: worker detached
error: An exception occurred.
Traceback (most recent call last):
  File "resource://jid0-ik4aoel78ao5udcgocwq4eep76c-at-jetpack-api-utils-lib/timer.js", line 77, in notifyOnInterval
    this._callback.apply(null, this._params);
  File "resource://jid0-ik4aoel78ao5udcgocwq4eep76c-at-jetpack-api-utils-lib/content/worker.js", line 115, in null
    callback.apply(null, params);
  File "javascript:setInterval(function() { self.postMessage("Test!");}, 2000);", line 1, in null
  File "resource://jid0-ik4aoel78ao5udcgocwq4eep76c-at-jetpack-api-utils-lib/content/content-proxy.js", line 92, in null
    let unwrapResult = Function.prototype.apply.apply(fun, [obj, args]);
[Exception... "Not enough arguments [nsIDOMWindowInternal.postMessage]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://jid0-ik4aoel78ao5udcgocwq4eep76c-at-jetpack-api-utils-lib/securable-module.js -> resource://jid0-ik4aoel78ao5udcgocwq4eep76c-at-jetpack-api-utils-lib/content/content-proxy.js :: <TOP_LEVEL> :: line 92"  data: no]
error: An exception occurred.


Expected Results:  
All code execution in the content-script should stop executing. The exception that is thrown (will be throw every 2 seconds in this example), shouldn't happen because the tab has been closed and we don't expect the code to be still executing.

In that case, the side effect is fairly light. But in a bigger add-on, this could cause firefox to become very unstable and slow as the number of exception throw could become really high.

Comment 1

[Nicolas Côté-Nolin]

I've just realised that the setInterval and setTimeout are actually overrided with the ones from the timer module when the sandbox is created. That explains why they keep on executing even after the tab is closed.

Modifying the setInterval (https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/worker.js#L111) and setTimeout (https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/worker.js#L99) with the following code seems to do the tricks :

setTimeout: function setTimeout(callback, delay) {
    let params = Array.slice(arguments, 2);
    var id = timer.setTimeout(function(worker) {
      try {
        // Addon Worker might be unloaded at this point
        if (this._addonWorker === null) {
          // Clear interval so it isn't called anymore
          this.clearTimeout(id);
          return;
        }
        callback.apply(null, params);
      } catch(e) {
        worker._asyncEmit('error', e);
      }
    }.bind(this), delay, this._addonWorker);
    
    return id;
  }

setInterval: function setInterval(callback, delay) {
    let params = Array.slice(arguments, 2);
    var id = timer.setInterval(function(worker) {
      try {
        // Addon Worker might be unloaded at this point
        if (this._addonWorker === null) {
          // Clear interval so it isn't called anymore
          this.clearInterval(id);
          return;
        }
        callback.apply(null, params); 
      } catch(e) {
        worker._asyncEmit('error', e);
      }
    }.bind(this), delay, this._addonWorker);
    
    return id;
  }

This might be a really naive fix as my knowledge of the SDK codebase is fairly limited. So I don't really feel confident putting up a patch before someone confirm that this fix makes senses.

Comment 2

[Alexandre Poirot [:ochameau]]

I think that it is developer responsability to cleanup content script on detach event so that such exceptions are not thrown. If we start doing it automatically, we may want to do it for everything. For example, if we do it for setTimeout, we may expect that DOM listener set through addEventListener to be removed automatically?
But I may be wrong, we may consider setInterval/setInterval as special case.
Or it may be more a documentation/example issues?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

I think addon developers would indeed expect listeners set through addEventListener to be removed automatically when a page is unloaded, since that's what happens to listeners set by a web page, and it's also what happens to functions passed in calls to setTimeout and setInterval in a web page.

And I suspect that is indeed what happens for content script listeners set through addEventListener: when the page is unloaded, those listeners are removed, even if the content script doesn't explicitly call removeEventListener for them.  Otherwise, addEventListener would leak documents.

So I don't think this is a special case.  Rather, when a web page is unloaded, content scripts should be unloaded in the same way, such that code within them no longer executes, even if it is scheduled to do so.

Perhaps we can accomplish this by making setTimeout/setInterval in the content script be the same ones provided to the web page?  My only concern there would be whether that enables the web page to cancel a function call scheduled by the content script by guessing its timeoutID/intervalID, in which case we probably want to continue to segregate setTimeout/setInterval but make sure to cancel all scheduled calls when detaching the content script.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Unregister automatically timeouts and intervals on worker destroy.

myk: you are right, if we use same setTimeout/setInterval than document's one, the web page would be able to cancel content scripts timeouts (I verified).

So here is a patch that allow to unregister them when the worker is destroyed, on tab/window close.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: DOM listeners test case

So I verified that DOM listeners set by the content script doesn't lead to leaks when the document is unloaded (tab/window close for ex).
Everything is cleaned up, no listener is called, nothing leak.

But, If you keep the document alive and only call worker.destroy(),
timeouts/intervals will be unregistered with previous patch, but all DOM listeners (click, message, ...) will stay registered and "leak". 

Here is a unit test that show a message event that fires in content script after worker.destroy().

Comment 6

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

Comment on attachment 543932 [details] [diff] [review]
Unregister automatically timeouts and intervals on worker destroy.

Review of attachment 543932 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, few Nits.

::: packages/api-utils/lib/content/worker.js
@@ +106,3 @@
>        try {
> +        delete self._timers[id];
> +        console.log("Call custom timer");

Please remove this console message it's unnecessary.

@@ +308,5 @@
>    },
>    _destructor: function _destructor() {
>      this._removeAllListeners();
> +    // Unregister all setTimeout/setInterval
> +    for(let id in this._timers)

please add space for (

@@ +309,5 @@
>    _destructor: function _destructor() {
>      this._removeAllListeners();
> +    // Unregister all setTimeout/setInterval
> +    for(let id in this._timers)
> +      timer.clearTimeout(id);

Please add comment that crearTimeout works for interval as well cause it's confusing otherwise.

Comment 7

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/f5cd5e5c17d044c8d7c26e0112c037b6700511cc