Closed Bug 796942 Opened 12 years ago Closed 12 years ago

Can't access dead object exceptions

Categories

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

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: patrick, Unassigned)

Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

Installed and disabled an addon that uses simple-storage.



Actual results:

An exception was thrown in simple-storage:

TypeError: can't access dead object




Expected results:

No exception is thrown.
This also happens during runtime when accessing require('simple-storage').quotaUsage.

Exception trace on disable:

error: An exception occurred.
Traceback (most recent call last):
  File "about:addons", line 1, in oncommand
    <?xml version="1.0"?>
  File "chrome://mozapps/content/extensions/extensions.xml", line 1023, in set_userDisabled
    this.mAddon.userDisabled = val;
  File "resource:///modules/XPIProvider.jsm", line 7749, in 
    XPIProvider.updateAddonDisabledState(aAddon, val);
  File "resource:///modules/XPIProvider.jsm", line 3831, in XPI_updateAddonDisabledState
    BOOTSTRAP_REASONS.ADDON_DISABLE);
  File "resource:///modules/XPIProvider.jsm", line 3711, in XPI_callBootstrapMethod
    this.bootstrapScopes[aId][aMethod](params, aReason);
  File "file:///Users/plin/Library/Application%20Support/Firefox/Profiles/v82uccod.dev/extensions/jid0-pPr9ydvwjKWJx1jvtqVelWfLgzo@jetpack/bootstrap.js", line 200, in shutdown
    unload(loader, reason);
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/api-utils/lib/loader.js", line 307, in unload
    notifyObservers(subject, 'sdk:loader:destroy', reason);
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/api-utils/lib/system/events.js", line 58, in 
    data: data
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/api-utils/lib/unload.js", line 74, in onunload
    unload(reason);
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/api-utils/lib/unload.js", line 53, in unload
    observers.forEach(function(observer) {
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/api-utils/lib/unload.js", line 55, in 
    observer(reason);
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/api-utils/lib/unload.js", line 64, in 
    unloaders.slice().forEach(function(unloadWrapper) {
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/api-utils/lib/unload.js", line 65, in 
    unloadWrapper(reason);
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/api-utils/lib/unload.js", line 38, in unloadWrapper
    originalDestructor.call(obj, reason);
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/addon-kit/lib/simple-storage.js", line 130, in JsonStore_unload
    this._write();
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/addon-kit/lib/simple-storage.js", line 157, in JsonStore__write
    if (this.quotaUsage > 1)
  File "resource://jid0-ppr9ydvwjkwjx1jvtqvelwflgzo-at-jetpack/addon-kit/lib/simple-storage.js", line 78, in 
    JSON.stringify(this.root).length / this.quota :
TypeError: can't access dead object
Re-post from bug 695480:

(In reply to patrick from comment #73)
> As I mentioned, I think that this is a larger issue than just simple-storage.
> 
> I also see these errors on a simple callback from xhr. For example:
> 
> var obj = { /* something */ };
> 
> var xhr = new XMLHttpRequest();
> xhr.open("GET", url, true);
> xhr.onreadystatechange = function() {
>     // Manipulate obj
>     obj.foo = 1;
> };
> xhr.send();
> 
> obj should remain live, but it has already been marked as a dead object. Am
> I crazy or is this behaving unexpectedly? This is a common pattern...
Summary: Can't access dead object exception in simple-storage → Can't access dead object exceptions
The above code should work, assuming it's creating an XMLHttpRequest object in chrome and creating obj in chrome.

Would you be able to create a minimal extension that shows the XMLHttpRequest problem?
This is the result of bug 775067. The exception comes because of a nukeSandbox feature that was invented to detach a sandbox from the rest of the world to get rid a lots of "leaks". Where leak is not the right term... But anyway if I'm not mistaken here the idea was that once the addon is disabled all it's object should be GCed, and their objects should not be accessible. I'm not here to judge weather this is the right thing to do or not, just pointing out where is this originated from.

So the question is what are the impact of this? (As I can see so far understandable confusion as for one for sure...)

So pages trying to access the object of the disabled addon will see this error and it will fail. I do think it is relatively harmless, but I can see how this error can be totally confusing on the other hand.
I'd still like to see a test add-on and the specific error. And to know when or where this is happening.

If nukeSandbox is to blame, then it means it must be happening when the add-on is disabled, or from a content script sandbox after the page is unloaded (after which point you wouldn't expect the XHR callback to be called in any case). But you also wouldn't expect the obj.foo assignment to be the failure point, since it's in the same compartment as the code working with it and there would be no wrapper to be cut. If I understand things right, the failure point would have to be the attempt to access the onreadystatechange callback, or the attempt of that code to access a property of the sandbox global.
I can understand (somewhat) if the exception is thrown when the addon is disabled. Additionally, I've seen "dead object" exceptions thrown when trying to access simple-storage quotaUsage during runtime.

Simply:

var ss = require('simple-storage');

sometime later:

if (ss.quotaUsage > 1) {
    // Do some clean up
}

throws an exception.

This is more worrisome than the exceptions thrown when the addon is disabled.
(In reply to Kris Maglione [:kmag] from comment #5)
> I'd still like to see a test add-on and the specific error. And to know when
> or where this is happening.

Me too. I'm just guessing here. One thing is sure, this exception comes from
a dead proxy object. Which likely be the result of a nukeSandbox. I'm not sure if that's the case here but the disabling clearly uses nukeSandbox.

> 
> If nukeSandbox is to blame, then it means it must be happening when the
> add-on is disabled, or from a content script sandbox after the page is
> unloaded (after which point you wouldn't expect the XHR callback to be
> called in any case). But you also wouldn't expect the obj.foo assignment to
> be the failure point, since it's in the same compartment as the code working
> with it and there would be no wrapper to be cut. If I understand things
> right, the failure point would have to be the attempt to access the
> onreadystatechange callback, or the attempt of that code to access a
> property of the sandbox global.

I really need to see the whole example. And I see what you mean and yes if the obj is from the same compartment then you are right. I just interpret { /* something */ }; differently.
(In reply to patrick from comment #6)
> I can understand (somewhat) if the exception is thrown when the addon is
> disabled. Additionally, I've seen "dead object" exceptions thrown when
> trying to access simple-storage quotaUsage during runtime.
> 
> Simply:
> 
> var ss = require('simple-storage');
> 
> sometime later:
> 
> if (ss.quotaUsage > 1) {
>     // Do some clean up
> }
> 
> throws an exception.
> 
> This is more worrisome than the exceptions thrown when the addon is disabled.

Without disabling the addon? Tomorrow I will try to talk to Alex if we're using nukeSandbox for something else too. Also will check the code if the same bits are used elsewhere in the platform than in nukeSandbox. In the meanwhile a minimal example, or at least a way to reproduce the issue with some available addon would be a big help.
But what about comment 2?  Do we think that's a different issue?  (I'd very much like to see a testcase.)
A minimized testcase is definitely essential. Looking at the code, the only way I can see this happening is if 'this.root' points to an object from a dead compartment, which seems to be possible.
Upon deeper examination of obj, I found that within the object children there was a reference to a window document. When the tab goes away, the reference is marked as dead.

To an extent, this makes sense. On the other hand, let's say that we need to perform some work for each tab that a user opens. If the user closes the tab before the work is complete, then is it acceptable that that work is lost? A hypothetical case, for sure, but one that I would think would be encountered...
If you need to do cleanup work when a document is destroyed, you need to do it before it is destroyed, generally with something like a pagehide or unload handler. Can you please provide a minimized test add-on for the simple-storage case? It seems likely that you're doing something similar there.
I do think that you are right. There may be a some legitimately dead objects that simple-storage is rejecting. I will need to examine it. Thanks.

I do question whether long-running work should be allowed to complete when holding a reference to a window document (or similar). I do think that there is some expectation that when a reference is obtained, the "rug" won't be pulled from under it.
From our point of view, it's impossible to tell "long-running work" apart from "will never terminate" work.  And the latter is the sort of memory leak that we aimed to fix.

So yeah, if the user closes a tab, no more access to the things from that tab...
Sure. I understand the motivation behind the change. I just feel that it's a pretty radical change from what has been expected behavior.
Right, I forgot to link the original origin of the compartment nuking thing: bug 695480. Anyway, is this a won't fix then? Or shall we try to do a little bit more for the confused developers? Like either adding some more descriptive exception message or writing some docs that helps them understand what might have just happened? I clearly see why is the current behavior useful, but from a js view point is kind of unexpected indeed.
Yes, it was a radical change....

And I thought we had docs already.
Correct. https://blog.mozilla.org/addons/2012/09/12/what-does-cant-access-dead-object-mean

patrick:
So I'll just close this bug for now, but feel free to reopen it again if you run into some dead objects where there shouldn't be any or something.
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
Sure. Thanks a lot.

If there are docs, then I must have missed them, besides the blog posts. Can you please point me to them.

I think that because this is such a big change, the docs should have a prominent "Read Me First" part the of the development guide. This would help to clear confusion.
(In reply to patrick from comment #19)
> If there are docs, then I must have missed them, besides the blog posts. Can
> you please point me to them.

I'm not sure... I CC-ed khuey he might know more about if there are any more docs about this and if yes where. If not then I'm really not the right person to decide if it's needed.
You need to talk to add-ons people if you're dissatisfied with the docs.  I doubt khuey even knows where the add-on documentation lives.  I'd drop the guy who wrote the blog post an e-mail, or find him on IRC, or cc him on this bug.
Ouch!  My addon has been broken since Firefox 15.  I'm pretty sure it never leaked because references were only held for a short period to a deleted iframe, then dropped.  None of the users complained, but I just spotted the errors in the console.
Necromancy is not right, I know, but I just wanted to put it there that this breaks any code that tries to hold on to DOM objects in fennec extension content scope. 

An example would be parsing an XML string (received from background) into a XML doc on tabLoad and trying to use it some time later (possibly after a docLoad or two) leads to 'Can't access dead object'. I guess it's a deadObject since the document it originated with is gone but still it's a weird thing to encounter and debugging gets you nowhere near the truth.

This made us rewrite the legacy code that uses XML resource files and works just fine in Firefox to use JSON instead for fennec compatibility...

Another example is this commit https://github.com/eligrey/FileSaver.js/commit/3109255efe9b46960ba0809cd20b704cd4b69158 I had to contribute to FileSaver.js lib to get it working with Fennec. The original dev created an anchor element on init and reused it for each download thereafter. Naturally, it's a deadObject in Fennec.
You need to log in before you can comment on or make changes to this bug.