Last Comment Bug 695480 - (hueyfix) Remove support for Chrome -> Content leaks
(hueyfix)
: Remove support for Chrome -> Content leaks
Status: RESOLVED FIXED
[MemShrink:P1]
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 11 votes (vote)
: mozilla15
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
http://blog.kylehuey.com/post/2189234...
Depends on: 787690 799329 816784 745681 746834 746837 746868 749106 749159 749526 749738 749749 749964 750009 751242 751420 751621 752468 773980 778318 783273 786576 786577 792208 809896
Blocks: 749532 bc-leaks 669240 674535 675412 745149 752877
  Show dependency treegraph
 
Reported: 2011-10-18 13:08 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2014-05-31 10:20 PDT (History)
58 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First draft (11.75 KB, patch)
2012-02-22 10:22 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (14.69 KB, patch)
2012-04-16 21:04 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (14.87 KB, patch)
2012-04-16 22:23 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jst: review+
Details | Diff | Splinter Review
Patch (15.98 KB, patch)
2012-04-17 20:26 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (17.17 KB, patch)
2012-04-18 17:45 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
wmccloskey: review+
bobbyholley: review+
Details | Diff | Splinter Review
MEMPATCH (17.17 KB, patch)
2012-06-09 01:36 PDT, ashpet1
no flags Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-18 13:08:19 PDT
A leak scenario we see a lot is JS holding onto a dead Window object.  This leads to holding onto a whole bunch of stuff.

We should consider cutting this chain somehow.  For instance, in a "chrome holds onto content Window" scenario, the situation looks something like:

Chrome compartment |                     | Content Compartment |XPCWrappedNative
                   |                     |                     |    owning
Window JSObject  ->|->XPConnect Wrapper->|-> Window JSObject ->| nsGlobalWindow

It seems like we might be able to cut out some of the middle here (in particular the content compartment).  A similar scenario might apply with windows of differing origins.
Comment 1 Nicholas Nethercote [:njn] 2011-10-25 12:22:30 PDT
Bug 669845 is an example that might be fixed by this.
Comment 2 Nicholas Nethercote [:njn] 2011-10-25 12:22:50 PDT
Blake, does this sound possible to you?
Comment 3 Blake Kaplan (:mrbkap) 2011-12-05 23:34:18 PST
I think we might be able to do something here, but we need to be fairly careful in terms of when we sever the connection. Currently, when a window is closed or navigated (and not held in the bfcache) we clear the scope of the window, nuking "expando" properties (properties not declared in IDL but stuck on the window object via global variables, etc.); however, IDL-defined properties do continue to work thanks to XPConnect's lazy resolution.

So, by doing this, we could potentially break code that actually does want to maintain a reference to a window, but is holding it alive, since we're kind of breaking the contract of a GC.

That being said, the easiest (and possibly cleanest) place to implement this would probably be to detach the cross compartment wrapper in the JS engine (fastest way to glory would be to brain transplant it with a /dev/null proxy). We could even warn in the error console when we do so to try to give developers a chance to clean up their stuff.
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-06 04:37:10 PST
Expandos can't be accessed cross-compartment anyways (with the exception of chrome touching content), right?  (I suppose this changes in the compartment-per-global world :-/)  I think we could get away with saying that chrome has to deal with the change and then only do this for cross-compartment wrappers.

This will get trickier with compartment-per-global unfortunately.
Comment 5 Blake Kaplan (:mrbkap) 2011-12-07 18:05:19 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> Expandos can't be accessed cross-compartment anyways (with the exception of
> chrome touching content), right?

document.domain can cause this too, even before compartment-per-global.

We could definitely do this for chrome -> content Xray wrappers only if we wanted to, though.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-07 18:08:29 PST
(In reply to Blake Kaplan (:mrbkap) from comment #5)
> We could definitely do this for chrome -> content Xray wrappers only if we
> wanted to, though.

I think that's a good place to start, especially since leaks caused by chrome are generally much longer lived than leaks caused by content.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-22 10:22:05 PST
Created attachment 599680 [details] [diff] [review]
First draft
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-24 11:23:13 PST
This breaks InstallTrigger, but other than that the browser seems to mostly work.  I'd like to reprioritize this at MemShrink next week.
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-16 21:04:37 PDT
Created attachment 615611 [details] [diff] [review]
Patch
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-16 22:23:08 PDT
Created attachment 615616 [details] [diff] [review]
Patch
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-16 23:19:38 PDT
Comment on attachment 615616 [details] [diff] [review]
Patch

This patch passes all tests, and fixes the leak in Bug 735401 (that was the only one I tried).
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-16 23:31:00 PDT
A recap of the basic idea here:

When we tear down a Window, we reach into the chrome compartments and grab all of the cross-compartment wrappers.  We iterate through them and find any that point to objects that are parented to the dying window.  We then replace the cross-compartment wrapper's proxy handler with a special handler that throws on every access, and clear out the slot that pointed to the cross-compartment value.

There is some special handling here for Windows.  We don't want to break the wrapper that points to the Window object when the inner window is torn down, but rather we want to break it only when the outer window is torn down.  Thus our function needs to know whether or not it should break wrappers that point to the Window.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-17 14:27:28 PDT
Comment on attachment 615616 [details] [diff] [review]
Patch

># HG changeset patch
># Parent c7f98e49184f85f4e1a7f856b80124740df283c4
># User Kyle Huey <khuey@kylehuey.com>
>
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -1349,16 +1349,30 @@ nsGlobalWindow::FreeInnerObjects()
>     static_cast<nsDOMOfflineResourceList*>(mApplicationCache.get())->Disconnect();
>     mApplicationCache = nsnull;
>   }
> 
>   mIndexedDB = nsnull;
> 
>   NotifyWindowIDDestroyed("inner-window-destroyed");
> 
>+  {
>+    if (!cx) {
>+      nsContentUtils::ThreadJSContextStack()->GetSafeJSContext(&cx);
>+    }
>+
>+    JSAutoRequest ar(cx);
>+
>+    JSObject* obj = GetGlobalJSObject();
>+    if (obj) {
>+      js_NukeChromeCrossCompartmentWrappersForGlobal(cx, obj,
>+                                                     JS_DontNukeGlobalObject);
>+    }
>+  }

Maybe move this around a bit and do something like this:

  JSObject* obj = FastGetGlobalJSObject();
  if (obj) {
    if (!cx) {
      nsContentUtils::ThreadJSContextStack()->GetSafeJSContext(&cx);
    }

    JSAutoRequest ar(cx);

    js_NukeChromeCrossCompartmentWrappersForGlobal(cx, obj,
                                                   JS_DontNukeGlobalObject);
  }

Also, I wonder if we need to protect against any goofy shutdown situations where we free a window very late and we may no longer have a safe JSContext? IOW, maybe null check cx after calling GetSafeJSContext(). Same thing for the second chunk that does the same thing...

+DeadObjectProxy::getPropertyDescriptor(JSContext *cx, JSObject *wrapper,
+                                       jsid id, bool set,
+                                       PropertyDescriptor *desc)
+{
+    printf("DeadObjectProxy\n");

Probably want to remove the printf's in these methods.

r=jst for the DOM changes, but somebody who knows the proxy and wrapper code etc should look at this. If Bill's that guy, then that's all good, if not, then maybe bholley should have a look at those pieces here too, in addition to Bill?
Comment 14 Bill McCloskey (:billm) 2012-04-17 15:24:21 PDT
Comment on attachment 615616 [details] [diff] [review]
Patch

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

I love the idea of this patch! I think it still needs a little work, though.

I have a few high-level issues. First, is the the special cross-compartment setSlot method necessary any more? I think the assert that used to hit is no longer on that path. The only thing the new method seems to do now is to assert cross-compartmentness, which doesn't seem necessary. Second, the extra SetProxyX methods in jsproxy.h could probably just become static functions in jswrapper.cpp. Then we wouldn't need to add anything to jsfriendapi.h.

I'm fine with bholley looking over the wrapper changes. It all looks good to me, but he probably knows better.

::: js/src/jsfriendapi.h
@@ +850,5 @@
> +    JS_DontNukeGlobalObject
> +} JSNukedGlobalHandling;
> +
> +extern JS_FRIEND_API(JSBool)
> +js_NukeChromeCrossCompartmentWrappersForGlobal(JSContext *cx, JSObject *obj,

It would be better to put this in namespace js and remove the prefix. Same for the enum.

::: js/src/jsproxy.h
@@ +202,5 @@
>  inline void
> +SetProxyHandler(JSObject *obj, ProxyHandler *handler)
> +{
> +    JS_ASSERT(IsProxy(obj));
> +    return SetReservedSlot(obj, JSSLOT_PROXY_HANDLER, PrivateValue(handler));

No need for return here.

::: js/src/jswrapper.cpp
@@ +960,5 @@
> +    virtual bool enumerate(JSContext *cx, JSObject *wrapper, AutoIdVector &props) MOZ_OVERRIDE;
> +    virtual bool fix(JSContext *cx, JSObject *wrapper, Value *vp) MOZ_OVERRIDE;
> +
> +    /* Spidermonkey extensions. */
> +    virtual JSString *obj_toString(JSContext *cx, JSObject *wrapper) MOZ_OVERRIDE;

Is there any reason that you didn't implement the other ProxyHandler methods? For example, the regexp_toShared default handler just asserts. Is it possible for chrome code to cause that handler to be invoked on a content object?

@@ +976,5 @@
> +                                       jsid id, bool set,
> +                                       PropertyDescriptor *desc)
> +{
> +    printf("DeadObjectProxy\n");
> +    JS_SetPendingException(cx, STRING_TO_JSVAL(JS_NewStringCopyZ(cx, "DEAD")));

I think the error handling for this should probably be similar to what we do with JSMSG_CLEARED_SCOPE. Can you just cargo-cult that?

@@ +1040,5 @@
> +    return JS_NewStringCopyZ(cx, "[object DeadObject]");
> +}
> +
> +DeadObjectProxy DeadObjectProxy::singleton;
> +int DeadObjectProxy::sDeadObjectFamily = 42;

Can you just leave this uninitialized? Otherwise people might think the value is significant (I did at first).

@@ +1043,5 @@
> +DeadObjectProxy DeadObjectProxy::singleton;
> +int DeadObjectProxy::sDeadObjectFamily = 42;
> +
> +JS_FRIEND_API(JSBool)
> +js_NukeChromeCrossCompartmentWrappersForGlobal(JSContext *cx, JSObject *obj,

A comment explaining this function would be good.

@@ +1056,5 @@
> +    // that point to an object that shares a global with obj.
> +    CompartmentVector &vector = rt->compartments;
> +    AutoValueVector toNuke(cx);
> +
> +    for (JSCompartment **p = vector.begin(), **end = vector.end(); p != end; ++p) {

Better to use CompartmentIter for this.

@@ +1063,5 @@
> +            continue;
> +
> +        // Iterate the wrappers looking for anything interesting.
> +        WrapperMap &pmap = (*p)->crossCompartmentWrappers;
> +        for (WrapperMap::Enum e(pmap); !e.empty(); e.popFront()) {

Doing two iterations seems unnecessary here. Enum allows elements to be removed from the hashtable via the removeFront() method.

::: js/src/vm/ObjectImpl-inl.h
@@ +194,5 @@
> +js::ObjectImpl::setSlotCrossCompartment(uint32_t slot, const js::Value &value)
> +{
> +    JS_ASSERT(slotInRange(slot));
> +
> +    js::HeapSlot& heapSlot = getSlotRef(slot);

& goes after the space.
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-17 20:26:38 PDT
Created attachment 616000 [details] [diff] [review]
Patch
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-17 20:30:45 PDT
(In reply to Bill McCloskey (:billm) from comment #14)
> Comment on attachment 615616 [details] [diff] [review]
> Patch
> 
> Review of attachment 615616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I love the idea of this patch! I think it still needs a little work, though.
> 
> I have a few high-level issues. First, is the the special cross-compartment
> setSlot method necessary any more? I think the assert that used to hit is no
> longer on that path. The only thing the new method seems to do now is to
> assert cross-compartmentness, which doesn't seem necessary. Second, the
> extra SetProxyX methods in jsproxy.h could probably just become static
> functions in jswrapper.cpp. Then we wouldn't need to add anything to
> jsfriendapi.h.

Turns out the special cross compartment stuff is not necessary anymore.  I left the SetProxyX methods in jsproxy.h since they don't need anything else in jsfriendapi.h anymore.

> I'm fine with bholley looking over the wrapper changes. It all looks good to
> me, but he probably knows better.

k

> ::: js/src/jsfriendapi.h
> @@ +850,5 @@
> > +    JS_DontNukeGlobalObject
> > +} JSNukedGlobalHandling;
> > +
> > +extern JS_FRIEND_API(JSBool)
> > +js_NukeChromeCrossCompartmentWrappersForGlobal(JSContext *cx, JSObject *obj,
> 
> It would be better to put this in namespace js and remove the prefix. Same
> for the enum.

Done.

> ::: js/src/jsproxy.h
> @@ +202,5 @@
> >  inline void
> > +SetProxyHandler(JSObject *obj, ProxyHandler *handler)
> > +{
> > +    JS_ASSERT(IsProxy(obj));
> > +    return SetReservedSlot(obj, JSSLOT_PROXY_HANDLER, PrivateValue(handler));
> 
> No need for return here.

Fixed.

> ::: js/src/jswrapper.cpp
> @@ +960,5 @@
> > +    virtual bool enumerate(JSContext *cx, JSObject *wrapper, AutoIdVector &props) MOZ_OVERRIDE;
> > +    virtual bool fix(JSContext *cx, JSObject *wrapper, Value *vp) MOZ_OVERRIDE;
> > +
> > +    /* Spidermonkey extensions. */
> > +    virtual JSString *obj_toString(JSContext *cx, JSObject *wrapper) MOZ_OVERRIDE;
> 
> Is there any reason that you didn't implement the other ProxyHandler
> methods? For example, the regexp_toShared default handler just asserts. Is
> it possible for chrome code to cause that handler to be invoked on a content
> object?

I thought I only had to override the base traps and everything else would be ok.  After talking to jorendorff on IRC he told me to override the base traps and the SM extensions.

> @@ +976,5 @@
> > +                                       jsid id, bool set,
> > +                                       PropertyDescriptor *desc)
> > +{
> > +    printf("DeadObjectProxy\n");
> > +    JS_SetPendingException(cx, STRING_TO_JSVAL(JS_NewStringCopyZ(cx, "DEAD")));
> 
> I think the error handling for this should probably be similar to what we do
> with JSMSG_CLEARED_SCOPE. Can you just cargo-cult that?

Done (I think).

> @@ +1040,5 @@
> > +    return JS_NewStringCopyZ(cx, "[object DeadObject]");
> > +}
> > +
> > +DeadObjectProxy DeadObjectProxy::singleton;
> > +int DeadObjectProxy::sDeadObjectFamily = 42;
> 
> Can you just leave this uninitialized? Otherwise people might think the
> value is significant (I did at first).

Done.

> @@ +1043,5 @@
> > +DeadObjectProxy DeadObjectProxy::singleton;
> > +int DeadObjectProxy::sDeadObjectFamily = 42;
> > +
> > +JS_FRIEND_API(JSBool)
> > +js_NukeChromeCrossCompartmentWrappersForGlobal(JSContext *cx, JSObject *obj,
> 
> A comment explaining this function would be good.

Didn't add this yet, need to think a bit on what to say.

> @@ +1056,5 @@
> > +    // that point to an object that shares a global with obj.
> > +    CompartmentVector &vector = rt->compartments;
> > +    AutoValueVector toNuke(cx);
> > +
> > +    for (JSCompartment **p = vector.begin(), **end = vector.end(); p != end; ++p) {
> 
> Better to use CompartmentIter for this.

'CompartmentIter' doesn't exist, as far as I can tell.

> @@ +1063,5 @@
> > +            continue;
> > +
> > +        // Iterate the wrappers looking for anything interesting.
> > +        WrapperMap &pmap = (*p)->crossCompartmentWrappers;
> > +        for (WrapperMap::Enum e(pmap); !e.empty(); e.popFront()) {
> 
> Doing two iterations seems unnecessary here. Enum allows elements to be
> removed from the hashtable via the removeFront() method.

You mean iterating over the wrappers and then iterating over the array of wrappers we found to get rid of?

> ::: js/src/vm/ObjectImpl-inl.h
> @@ +194,5 @@
> > +js::ObjectImpl::setSlotCrossCompartment(uint32_t slot, const js::Value &value)
> > +{
> > +    JS_ASSERT(slotInRange(slot));
> > +
> > +    js::HeapSlot& heapSlot = getSlotRef(slot);
> 
> & goes after the space.

This is gone anyways.
Comment 17 Nicholas Nethercote [:njn] 2012-04-17 20:51:04 PDT
> > > +    for (JSCompartment **p = vector.begin(), **end = vector.end(); p != end; ++p) {
> > 
> > Better to use CompartmentIter for this.
> 
> 'CompartmentIter' doesn't exist, as far as I can tell.

JS_IterateCompartments, perhaps?
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-17 21:43:02 PDT
(In reply to Nicholas Nethercote [:njn] from comment #17)
> > > > +    for (JSCompartment **p = vector.begin(), **end = vector.end(); p != end; ++p) {
> > > 
> > > Better to use CompartmentIter for this.
> > 
> > 'CompartmentIter' doesn't exist, as far as I can tell.
> 
> JS_IterateCompartments, perhaps?

Ah, I think Bill meant CompartmentsIter.  I'll make that change locally.
Comment 19 Bill McCloskey (:billm) 2012-04-17 21:58:58 PDT
Comment on attachment 616000 [details] [diff] [review]
Patch

I just noticed that if you pass DontNukeGlobalObject for nukeGlobal, then the function doesn't seem to do anything: it always executes the continue statement. Should the if condition use && instead of || ?

> 'CompartmentIter' doesn't exist, as far as I can tell.

Sorry, I guess it's 'CompartmentsIter'. It's in jscompartment.h.

> You mean iterating over the wrappers and then iterating over the array of
> wrappers we found to get rid of?

Yeah, I was thinking it might be easier to replace the call to |toNuke.append(v)| with something like this:
    e.removeFront();
    SetProxyPrivate(wobj, JSVAL_NULL);
    SetProxyHandler(wobj, &DeadObjectProxy::singleton);
Then you could remove the toNuke loop. Unless I'm being dumb and there's some reason the extra loop is important.

Also, can you replace JS_TRUE and JS_FALSE with true and false? We're now doing that even for functions that return JSBool.
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-17 22:07:48 PDT
(In reply to Bill McCloskey (:billm) from comment #19)
> Comment on attachment 616000 [details] [diff] [review]
> Patch
> 
> I just noticed that if you pass DontNukeGlobalObject for nukeGlobal, then
> the function doesn't seem to do anything: it always executes the continue
> statement. Should the if condition use && instead of || ?

Ah crap, you're right.

> > 'CompartmentIter' doesn't exist, as far as I can tell.
> 
> Sorry, I guess it's 'CompartmentsIter'. It's in jscompartment.h.

Yeah.

> > You mean iterating over the wrappers and then iterating over the array of
> > wrappers we found to get rid of?
> 
> Yeah, I was thinking it might be easier to replace the call to
> |toNuke.append(v)| with something like this:
>     e.removeFront();
>     SetProxyPrivate(wobj, JSVAL_NULL);
>     SetProxyHandler(wobj, &DeadObjectProxy::singleton);
> Then you could remove the toNuke loop. Unless I'm being dumb and there's
> some reason the extra loop is important.

I don't think its important, I just copied from the other function that does wrapper transplanting.

> Also, can you replace JS_TRUE and JS_FALSE with true and false? We're now
> doing that even for functions that return JSBool.

Ok.
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-04-18 01:49:04 PDT
I (or blake) should probably review the wrapper stuff here. Let me know when it's ready.
Comment 22 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-18 17:45:04 PDT
Created attachment 616377 [details] [diff] [review]
Patch
Comment 23 Bill McCloskey (:billm) 2012-04-18 18:30:02 PDT
Comment on attachment 616377 [details] [diff] [review]
Patch

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

I wonder if eventually we should remove ProxyHandler::trace. It seems kind of dangerous here. Are there any plans to use it for the new DOM bindings?
Comment 24 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-19 21:22:43 PDT
(In reply to Bill McCloskey (:billm) from comment #23)
> Comment on attachment 616377 [details] [diff] [review]
> Patch
> 
> Review of attachment 616377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder if eventually we should remove ProxyHandler::trace. It seems kind
> of dangerous here. Are there any plans to use it for the new DOM bindings?

That's a peterv, or maybe a bz question.
Comment 25 Bobby Holley (:bholley) (busy with Stylo) 2012-04-24 09:21:20 PDT
Comment on attachment 616377 [details] [diff] [review]
Patch

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp

I've decided that I don't know this code well enough to review it. So it'll need review from jst, blake, or bz. I also don't have a great handle on the do/don't nuke global stuff.

>diff --git a/js/src/jsfriendapi.h b/js/src/jsfriendapi.h
>--- a/js/src/jsfriendapi.h
>+++ b/js/src/jsfriendapi.h
>@@ -785,16 +785,26 @@ CastToJSFreeOp(FreeOp *fop)
> 
> /*
>  * Get an error type name from a number.
>  * If no exception is associated, return NULL.
>  */
> extern JS_FRIEND_API(const jschar*)
> GetErrorTypeNameFromNumber(JSContext* cx, const unsigned errorNumber);
> 
>+/* Implemented in jswrapper.cpp. */
>+typedef enum NukedGlobalHandling {
>+    NukeGlobalObject,
>+    DontNukeGlobalObject
>+} NukedGlobalHandling;

This is misnamed, yeah? IIUC, we're not nuking any real objects, just wrappers. So NukeForGlobalObject, maybe?

> inline void
>+SetProxyHandler(JSObject *obj, ProxyHandler *handler)
>+{
>+    JS_ASSERT(IsProxy(obj));
>+    SetReservedSlot(obj, JSSLOT_PROXY_HANDLER, PrivateValue(handler));
>+}
>+
>+inline void
>+SetProxyPrivate(JSObject *obj, const Value &value)
>+{
>+    JS_ASSERT(IsProxy(obj));
>+    SetReservedSlot(obj, JSSLOT_PROXY_PRIVATE, value);
>+}
>+
>+inline void

Why are we adding these? It doesn't look like they're used externally. If the goal is just to prevent slot munging within spidermonkey, then we should convert the existing sites that set handlers and private as well. And it should probably be a separate patch.

>+    virtual bool fix(JSContext *cx, JSObject *wrapper, Value *vp) MOZ_OVERRIDE;

You'll be racing with Eddy who's removing fix (and doing a bunch of other proxy refactoring) over in bug 703537. No big deal though.

>+/*
>+ * NukeChromeCrossCompartmentWrappersForGlobal reaches into chrome and cuts
>+ * all of the cross-compartment wrappers that point to objects parented to
>+ * obj's global.  The snag here is that we need to avoid cutting wrappers that
>+ * point to the window object on page navigation (inner window destruction)
>+ * and only do that on tab close (outer window destruction).  Thus the
>+ * option of how to handle the global object.
>+ */
>+JS_FRIEND_API(JSBool)
>+js::NukeChromeCrossCompartmentWrappersForGlobal(JSContext *cx, JSObject *obj,
>+                                                js::NukedGlobalHandling nukeGlobal)
>+{
>+    CHECK_REQUEST(cx);
>+
>+    JSRuntime *rt = cx->runtime;
>+    JSObject *global = &obj->global();
>+
>+    // Iterate through scopes looking for system cross compartment wrappers
>+    // that point to an object that shares a global with obj.
>+
>+    for (CompartmentsIter c(rt); !c.done(); c.next()) {
>+        // Skip non-system compartments because this breaks the web.
>+        if (!js::IsSystemCompartment(c))
>+            continue;
>+
>+        // Iterate the wrappers looking for anything interesting.
>+        WrapperMap &pmap = c->crossCompartmentWrappers;
>+        for (WrapperMap::Enum e(pmap); !e.empty(); e.popFront()) {
>+            const Value &k = e.front().key;
>+            if (k.isString())
>+                continue;

Need a comment explaining what this is about.

>+            const Value &v = e.front().value.get();
>+            JSObject *wobj = &v.toObject();

I'd be for collapsing these two lines, unless we need v for something that I don't see.

>+            JSObject *wrapped = UnwrapObject(wobj);
>+            if (JSObjectOp op = wrapped->getClass()->ext.innerObject)
>+                wrapped = op(cx, wrapped);

Just pass stopAtOuter=false to UnwrapObject. And even if you didn't, you'd want OBJ_TO_INNER_OBJECT here.

>+
>+            if (nukeGlobal == DontNukeGlobalObject && wrapped == global)
>+                continue;
>+
>+            if (&wrapped->global() == global) {
>+                // We found a wrapper to nuke.
>+                e.removeFront();
>+
>+                SetProxyPrivate(wobj, JSVAL_NULL);
>+                SetProxyHandler(wobj, &DeadObjectProxy::singleton);
>+
>+                if (IsFunctionProxy(wobj)) {
>+                    wobj->setReservedSlot(JSSLOT_PROXY_CALL, JSVAL_NULL);
>+                    wobj->setReservedSlot(JSSLOT_PROXY_CONSTRUCT, JSVAL_NULL);
>+                }
>+
>+                wobj->setReservedSlot(JSSLOT_PROXY_EXTRA + 0, JSVAL_NULL);
>+                wobj->setReservedSlot(JSSLOT_PROXY_EXTRA + 1, JSVAL_NULL);

Can we make this a method on Proxy (or Wrapper) instead?
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2012-04-24 14:51:48 PDT
CCing eddy, since he should be in the loop here given his proxy refactoring.
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2012-04-25 04:55:28 PDT
Also, I'd appreciate if this could be tested on top of compartment-per-global before landing so that we don't add further road blocks on that front.

The patches are here: https://github.com/bholley/mozilla-central/commits/cpgtrain

Currently there's 3 patches (one of which is some test fixes that have yet to land), and the two r=mrbkap patches. Current failures are a crash in the android reftest harness, and a DOMWindow/DocShell leak orange on some (but not all) platforms. I'll hopefully have answers for all that soon.
Comment 28 Justin Lebar (not reading bugmail) 2012-04-25 08:11:32 PDT
https://tbpl.mozilla.org/?tree=Try&rev=73e772319727
Comment 29 Justin Lebar (not reading bugmail) 2012-04-25 12:37:56 PDT
Pushed to try again because the last one came right before the downtime and didn't get builds on all platforms.

https://tbpl.mozilla.org/?tree=Try&rev=4932c597e644
Comment 30 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-25 21:07:10 PDT
(In reply to Bobby Holley (:bholley) from comment #25)
> Comment on attachment 616377 [details] [diff] [review]
> Patch
> 
> >diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
> >--- a/dom/base/nsGlobalWindow.cpp
> >+++ b/dom/base/nsGlobalWindow.cpp
> 
> I've decided that I don't know this code well enough to review it. So it'll
> need review from jst, blake, or bz. I also don't have a great handle on the
> do/don't nuke global stuff.

jst reviewed it.

> >diff --git a/js/src/jsfriendapi.h b/js/src/jsfriendapi.h
> >--- a/js/src/jsfriendapi.h
> >+++ b/js/src/jsfriendapi.h
> >@@ -785,16 +785,26 @@ CastToJSFreeOp(FreeOp *fop)
> > 
> > /*
> >  * Get an error type name from a number.
> >  * If no exception is associated, return NULL.
> >  */
> > extern JS_FRIEND_API(const jschar*)
> > GetErrorTypeNameFromNumber(JSContext* cx, const unsigned errorNumber);
> > 
> >+/* Implemented in jswrapper.cpp. */
> >+typedef enum NukedGlobalHandling {
> >+    NukeGlobalObject,
> >+    DontNukeGlobalObject
> >+} NukedGlobalHandling;
> 
> This is misnamed, yeah? IIUC, we're not nuking any real objects, just
> wrappers. So NukeForGlobalObject, maybe?

Fixed.

> > inline void
> >+SetProxyHandler(JSObject *obj, ProxyHandler *handler)
> >+{
> >+    JS_ASSERT(IsProxy(obj));
> >+    SetReservedSlot(obj, JSSLOT_PROXY_HANDLER, PrivateValue(handler));
> >+}
> >+
> >+inline void
> >+SetProxyPrivate(JSObject *obj, const Value &value)
> >+{
> >+    JS_ASSERT(IsProxy(obj));
> >+    SetReservedSlot(obj, JSSLOT_PROXY_PRIVATE, value);
> >+}
> >+
> >+inline void
> 
> Why are we adding these? It doesn't look like they're used externally. If
> the goal is just to prevent slot munging within spidermonkey, then we should
> convert the existing sites that set handlers and private as well. And it
> should probably be a separate patch.

Yes.  If we want to convert the existing uses lets do that in a followup.

> >+    virtual bool fix(JSContext *cx, JSObject *wrapper, Value *vp) MOZ_OVERRIDE;
> 
> You'll be racing with Eddy who's removing fix (and doing a bunch of other
> proxy refactoring) over in bug 703537. No big deal though.

His loss ;-)

> >+/*
> >+ * NukeChromeCrossCompartmentWrappersForGlobal reaches into chrome and cuts
> >+ * all of the cross-compartment wrappers that point to objects parented to
> >+ * obj's global.  The snag here is that we need to avoid cutting wrappers that
> >+ * point to the window object on page navigation (inner window destruction)
> >+ * and only do that on tab close (outer window destruction).  Thus the
> >+ * option of how to handle the global object.
> >+ */
> >+JS_FRIEND_API(JSBool)
> >+js::NukeChromeCrossCompartmentWrappersForGlobal(JSContext *cx, JSObject *obj,
> >+                                                js::NukedGlobalHandling nukeGlobal)
> >+{
> >+    CHECK_REQUEST(cx);
> >+
> >+    JSRuntime *rt = cx->runtime;
> >+    JSObject *global = &obj->global();
> >+
> >+    // Iterate through scopes looking for system cross compartment wrappers
> >+    // that point to an object that shares a global with obj.
> >+
> >+    for (CompartmentsIter c(rt); !c.done(); c.next()) {
> >+        // Skip non-system compartments because this breaks the web.
> >+        if (!js::IsSystemCompartment(c))
> >+            continue;
> >+
> >+        // Iterate the wrappers looking for anything interesting.
> >+        WrapperMap &pmap = c->crossCompartmentWrappers;
> >+        for (WrapperMap::Enum e(pmap); !e.empty(); e.popFront()) {
> >+            const Value &k = e.front().key;
> >+            if (k.isString())
> >+                continue;
> 
> Need a comment explaining what this is about.

Added.

> >+            const Value &v = e.front().value.get();
> >+            JSObject *wobj = &v.toObject();
> 
> I'd be for collapsing these two lines, unless we need v for something that I
> don't see.

Fixed.

> >+            JSObject *wrapped = UnwrapObject(wobj);
> >+            if (JSObjectOp op = wrapped->getClass()->ext.innerObject)
> >+                wrapped = op(cx, wrapped);
> 
> Just pass stopAtOuter=false to UnwrapObject. And even if you didn't, you'd
> want OBJ_TO_INNER_OBJECT here.

Fixed.

> >+
> >+            if (nukeGlobal == DontNukeGlobalObject && wrapped == global)
> >+                continue;
> >+
> >+            if (&wrapped->global() == global) {
> >+                // We found a wrapper to nuke.
> >+                e.removeFront();
> >+
> >+                SetProxyPrivate(wobj, JSVAL_NULL);
> >+                SetProxyHandler(wobj, &DeadObjectProxy::singleton);
> >+
> >+                if (IsFunctionProxy(wobj)) {
> >+                    wobj->setReservedSlot(JSSLOT_PROXY_CALL, JSVAL_NULL);
> >+                    wobj->setReservedSlot(JSSLOT_PROXY_CONSTRUCT, JSVAL_NULL);
> >+                }
> >+
> >+                wobj->setReservedSlot(JSSLOT_PROXY_EXTRA + 0, JSVAL_NULL);
> >+                wobj->setReservedSlot(JSSLOT_PROXY_EXTRA + 1, JSVAL_NULL);
> 
> Can we make this a method on Proxy (or Wrapper) instead?

In a followup.
Comment 31 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-25 21:25:49 PDT
(In reply to Bobby Holley (:bholley) from comment #27)
> Also, I'd appreciate if this could be tested on top of
> compartment-per-global before landing so that we don't add further road
> blocks on that front.
> 
> The patches are here:
> https://github.com/bholley/mozilla-central/commits/cpgtrain
> 
> Currently there's 3 patches (one of which is some test fixes that have yet
> to land), and the two r=mrbkap patches. Current failures are a crash in the
> android reftest harness, and a DOMWindow/DocShell leak orange on some (but
> not all) platforms. I'll hopefully have answers for all that soon.

Yeah, this patch adds some new test failures.  I'm not going to block landing this, but I'll work on fixing them.
Comment 32 Bobby Holley (:bholley) (busy with Stylo) 2012-04-25 23:05:59 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #31)
> Yeah, this patch adds some new test failures.  I'm not going to block
> landing this, but I'll work on fixing them.

Um, can we? I've spent months getting Mochitest-O green with CPG, and this patch turns it orange as Halloween.

CPG is days away from landing, and a much larger effort than this bug. If you disagree with me here, please get jst's opinion before pushing.
Comment 33 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-25 23:38:51 PDT
I can work on these test failures while you work on the other remaining stuff.  If we do get to a point where this stuff is the only remaining blocker its pretty easy to disable cutting chrome->chrome wrappers.

Also I already pushed this before you told me not to :-)

https://hg.mozilla.org/mozilla-central/rev/cc5254f9825f
Comment 34 Bobby Holley (:bholley) (busy with Stylo) 2012-04-25 23:58:14 PDT
(In reply to Bobby Holley (:bholley) from comment #27)
> Currently there's 3 patches (one of which is some test fixes that have yet
> to land), and the two r=mrbkap patches. Current failures are a crash in the
> android reftest harness, and a DOMWindow/DocShell leak orange on some (but
> not all) platforms.

Turns out the second issue here is an orangefactor thing and unrelated to cpg. See bug 734554 comment 71.
Comment 35 Kris Maglione [:kmag] 2012-04-26 15:36:19 PDT
Adding dev-doc-needed since this is something developers should be aware of. At the least, it should be mentioned in Firefox 15 for Developers on MDC and the leak testing wiki pages, and probably somewhere on the pages about wrappers. If someone can spell out the exact ramifications of the final patch, I can do the docs.
Comment 36 Loic 2012-04-27 09:32:49 PDT
This patch seems to be very powerful to stop memory leaks from add-ons.

But in terms of bugfixing in Firefox, can it be a case of not seeing the forest for the trees, I mean, is this patch not going to hide some legitimate memory leaks in Firefox code and prevent from fixing them?
Comment 37 Justin Lebar (not reading bugmail) 2012-04-27 09:37:28 PDT
> But in terms of bugfixing in Firefox, can it be a case of not seeing the forest for the trees, I 
> mean, is this patch not going to hide some legitimate memory leaks in Firefox code and prevent from 
> fixing them?

No, in fact it's going to fix the memory leaks in Firefox itself, too.  For example, bug 675412 was fixed by this bug.
Comment 38 Andrew McCreight [:mccr8] 2012-04-27 09:45:36 PDT
> is this patch not going to hide some legitimate memory leaks in Firefox code and prevent from fixing them?

It would be theoretically possible for there to be a large glob of memory held alive in chrome that shouldn't be, that would only be noticed because that glob of memory happened to hold alive content that shows up as a zombie compartment.  But there is no guarantee that the chrome blob would hold alive content, so it may not be noticed even without this patch.  Furthermore, in practice, chrome leaks have been a problem mostly because they keep alive content, not because they are so bad themselves.
Comment 39 Nicholas Nethercote [:njn] 2012-04-27 20:48:02 PDT
> But in terms of bugfixing in Firefox, can it be a case of not seeing the
> forest for the trees, I mean, is this patch not going to hide some
> legitimate memory leaks in Firefox code and prevent from fixing them?

I understand your concern -- in a way this is less elegant or "pure" than just fixing the leaks.  But experience has shown us that even in Firefox itself, "just fixing the leaks" is *really* hard.  And "just fixing the leaks" in *add-ons* is an endless game of whack-a-mole.  We'll never even get close.

With that in mind -- bring on the pragmatic solution! :)
Comment 40 Nicholas Nethercote [:njn] 2012-04-27 21:08:27 PDT
This patch's landing has interesting implications for the top 100 add-on testing (bug 730737).  Lots of leaks that will manifest in older versions won't manifest in versions that have this patch.
 
There's a similar question about AMO reviews -- should reviewers still look for zombie compartments?  Most (all?) of them will no longer be possible.  Perhaps our documentation about this should be updated?

I'm inclined to not take any action immediately, just to give the patch time to bake in nightlies for a while, and to see the effects.  Speaking of which, do we have ideas how to gauge the effect of this patch in the wild?  I guess telemetry will give us some ideas.
Comment 41 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-27 21:11:10 PDT
If we want to continue testing addons we should pick a fixed revision of Gecko to test against that's before this patch (how about the base of Aurora 14?)
Comment 42 Nicholas Nethercote [:njn] 2012-04-27 21:18:47 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #41)
> If we want to continue testing addons we should pick a fixed revision of
> Gecko to test against that's before this patch (how about the base of Aurora
> 14?)

I would like to continue testing them until this patch makes it into a Firefox release.  Assuming it doesn't get backed out, that'll be Firefox 15, which will be released on August 28.

See also bug 745149 comment 8.
Comment 43 Radek 'sysKin' Czyz 2012-04-27 21:44:23 PDT
Hi, can you please clarify one thing for me :)

Imagine a scenario.
1. Hypothetical leaky chrome creates a reference from chrome to document.body of every website
2. I open ten twitter feeds
3. I close nine twitter feeds

This bug didn't change anything in this scenario: ten DOM trees remain in memory because they're referenced, right?

The "leak" (lifetime problem) described above is still bad. My concern is that it can no longer be found as a zombie compartment, so it might be harder to find or easier to ignore.

Are my worries justified in any way? :)
Comment 44 Boris Zbarsky [:bz] 2012-04-27 22:56:57 PDT
> This bug didn't change anything in this scenario: ten DOM trees remain in memory because
> they're referenced, right?

No.  This bug is all about breaking the references.  After step 3 in your steps, you have nine references in chrome that don't reference anything and throw if you try to do anything with them, and one reference to the document.body of the remaining twitter feed.
Comment 45 Radek 'sysKin' Czyz 2012-04-28 00:46:44 PDT
(In reply to Boris Zbarsky (:bz) from comment #44)
> No.

Ah! And I thought the references are only broken when the *compartment* is trying to disappear (which only happens after all ten pages are closed, not nine).
Thanks for answering :)
Comment 46 Boris Zbarsky [:bz] 2012-04-28 00:57:19 PDT
Even if that were the case, Bobby is about to land compartment-per-global.  ;)
Comment 47 Willy_ Foo_Foo 2012-04-28 08:02:31 PDT
Lower End systems are seeming to experience on Windows Xp & Linux systems
"Not Responding Situations from Chrome for several seconds"(heavy sites)


https://bugzilla.mozilla.org/show_bug.cgi?id=105843

would this bug be also a part of this bug(depends)? As both help each other & will help in speeding up
overall performances(cache preferred instead of loading into mem again) well as reduce loading times & less zombie compartments
Comment 48 Nicholas Nethercote [:njn] 2012-05-02 15:44:36 PDT
Good news:  I had an old version of McAfee SiteAdvisor 3.4.1 -- the one that leaked every content compartment ever opened -- lying around.  I reinstalled it.  With a release build (FF12) it still exhibited the problem, but with a Nightly build there were no zombie compartments!  Mission accomplished.
Comment 49 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-02 18:40:02 PDT
Sweet! :)
Comment 50 Nicholas Nethercote [:njn] 2012-05-02 22:05:08 PDT
I did some more testing with two FF15 dev builds on my Linux64 box -- one ("Before") just prior to bug 695480 landing, and one ("After") from a couple of days after that.

Of the six add-ons from bug 700547 that I tried, bug 695480 prevents zombie compartments in all of then, *including* Firebug!  Full details below.



Bug 669730: Firebug 1.9.1
https://addons.mozilla.org/en-US/firefox/addon/firebug/?src=hp-dl-mostpopular
Before: Open Firebug (F12) on a page, then close.  The page's compartments
        leak.  Subsequent leaks (usually) replace prior ones.
After:  No zombie compartments!


Bug 735401: PicPasso 1.0.1
https://addons.mozilla.org/addon/picpasso/
Before: Click the PicPasso icon (just once;  don't select anything from the
        dropdown menu).  Then visit pages with login forms:  I tried Gmail,
        Facebook, Twitter, MDN.  No need to actually login.  Various zombie
        compartments, sometimes one, sometimes multiple, and they'd
        sometimes be replaced by a subsequent one.
After   No zombie compartments!


Bug 740085: LoL 1.5
https://addons.mozilla.org/en-US/firefox/addon/lol/
Before:  Leaks *every* content compartment.
After:   No zombie compartments!


Bug 742232: Youtube ratings preview 1.03
https://addons.mozilla.org/en-US/firefox/addon/youtube-bogus-remover/
Before:  Leaks youtube.com compartment if you do a youtube search. 
After:   No zombie compartments!


Bug 745149: Enter Selects 7
https://addons.mozilla.org/en-US/firefox/addon/enter-selects/
Before: Do "select as Desktop background" on an image, then cancel.  The
        compartment holding the image leaks.
After:  No zombie compartments!


Bug 747167: HttpFox 0.8.10
https://addons.mozilla.org/addon/httpfox/
Before:  Click the HttpFox icon, hit "start", open and close
         http://www.mozilla.org/en-US/firefox/central/;  it leaks until you
         hit "clear".
After:   No zombie compartments!
Comment 51 Nicholas Nethercote [:njn] 2012-05-02 22:28:26 PDT
One more for good luck:

Bug 728589: Link Resolve 0.7
https://addons.mozilla.org/en-US/firefox/addon/linkresolve/
Before: Do a Google search, right click on a search result link, choose
        "resolve link" from the menu.  Then close all tabs.  Zombies remain
        for Google and the linked-to page.
After:  No zombie compartments!
Comment 52 Justin Lebar (not reading bugmail) 2012-05-02 22:38:10 PDT
This apparently doesn't fix leaks in the old Jetpack SDK (e.g. bug 751420).  We should aggressively move people off old versions; I've filed bug 751466.
Comment 53 Nicholas Nethercote [:njn] 2012-05-09 19:12:06 PDT
Another add-on fixed by this change:

Bug 732782: AutoPager 0.7.1.4
https://bugzilla.mozilla.org/show_bug.cgi?id=732782
Before: Visit Google, click "yes" when offered the rule for the page, do a search,
        scroll to the bottom of the page.  google.com becomes a zombie compartment.
After:  No zombie compartments!
Comment 54 ashpet1 2012-06-09 01:36:50 PDT
Created attachment 631629 [details] [diff] [review]
MEMPATCH
Comment 55 Clint Priest 2012-09-02 07:16:59 PDT
FYI: It appears this patch is causing perfectly legitimate code to break.  I'll be changing my plugin to compensate, but here is what I'm doing:

if(this.ElementCount && this.ElementCount.parentNode)
	this.ElementCount.parentNode.removeChild(this.ElementCount);

It's the this.ElementCount.parentNode call that is throwing the exception.  I have a reference to this.ElementCount and it has already been removed from the DOM, however after a page change, accessing that ElementCount.parentNode causes "Can't access dead object."

I understand and agree with the intent of this fix, but I believe that the above code should not be throwing an exception, should it?
Comment 56 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-02 07:37:40 PDT
If elementCount or elementCount.parentNode points to some object in a closed window/tab, then throwing 
exception is the right thing to do.
Removing element from DOM isn't enough, if the element has still its ownerDocument pointing to the
document in the closed window/tab.
Comment 57 Boris Zbarsky [:bz] 2012-09-02 07:42:13 PDT
> I have a reference to this.ElementCount and it has already been removed from the DOM

But it still keeps that DOM tree alive via its ownerDocument pointer.  So your extension was in fact leaking the web page that this.ElementCount came from.
Comment 58 Clint Priest 2012-09-02 07:43:51 PDT
What if I wanted to add it to the new document, rather than re-create a new element each time?
Comment 59 Clint Priest 2012-09-02 07:44:41 PDT
If I have removed it from the document, then why would it's ownerDocument point to anything?  It's been removed from the document already...
Comment 60 Boris Zbarsky [:bz] 2012-09-02 07:49:00 PDT
> What if I wanted to add it to the new document

Then you should be creating it in the chrome document and using importNode to create copies for content documents as needed.

> then why would it's ownerDocument point to anything? 

Because that's how ownerDocument is defined.  It points to the document the node was created in (or possibly adopted into if it was adopted), even when it's not in the DOM tree.  You can test this trivially in a webpage:

  alert((new Image()).ownerDocument == document);

for example.
Comment 61 Clint Priest 2012-09-02 07:53:51 PDT
> Because that's how ownerDocument is defined.  It points to the document the
> node was created in (or possibly adopted into if it was adopted), even when
> it's not in the DOM tree.  You can test this trivially in a webpage:

When you say defined, are you referring to W3C standards?  It doesn't make sense to me that if I have removed an element from a document, that checking to see if it has a parentNode should throw an exception.  Seems an internal FF issue to me since I may want to keep that element and add it to or adopt it to another window/document at a later time and shouldn't expect an exception just by checking if it has a parent node.

Is there some other way to remove it properly from a document besides the removeChild() I'm doing there?

Don't get me wrong here, I don't want to cause memory leaks, I just don't see why what I am doing *should* do that, since my intention by the removeChild() was to divorce it from the document I had added it to (thereby leaving no pointer to that document to cause a memory leak.)
Comment 62 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-09-02 07:55:37 PDT
(In reply to Clint from comment #61)
> Don't get me wrong here, I don't want to cause memory leaks, I just don't
> see why what I am doing *should* do that, since my intention by the
> removeChild() was to divorce it from the document I had added it to (thereby
> leaving no pointer to that document to cause a memory leak.)

But that's not possible.  Because of the way ownerDocument is defined, a node can never be completely divorced from the document (without associating it with a new document, via adoptNode).
Comment 63 Clint Priest 2012-09-02 08:01:42 PDT
I was just looking through the DOM level 3 spec, doesn't say anything about .ownerDocument having to stay defined, but I can see how it would be useful in many cases to know which document it was last a part of, and that even if removed, it is still a part of that document.

In any case, is there a way to completely remove it from a document besides removeChild()?
Comment 64 Justin Lebar (not reading bugmail) 2012-09-02 09:25:04 PDT
> In any case, is there a way to completely remove it from a document besides removeChild()?

Kyle said in comment 62:

> Because of the way ownerDocument is defined, a node can never be completely divorced from [a]
> document (without associating it with a new document, via adoptNode).

So even removeChild() is not sufficient to "completely [divorce] a node from [its] document".  A node is always associated with some non-null document.

If you really want to keep these nodes around, your plugin could adoptNode them into some document which sticks around forever.

> Seems an internal FF issue to me since I may want to keep that element and add it to or adopt it 
> to another window/document at a later time and shouldn't expect an exception just by checking if 
> it has a parent node.

Boris and Olli are two of Mozilla's foremost experts on web standards.  I agree that the behavior here is counter-intuitive and understand that this seems like an internal FF issue, but you may have to believe them when they say that this is How The Web Works and that it isn't a bug.

The DOM spec [1] says that the ownerDocument is changed by the |adopt| algorithm.  If you search through the spec, you'll notice that we never invoke the adopt algorithm with a document which could be null.

[1] http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-node-document
Comment 65 Clint Priest 2012-09-02 10:51:16 PDT
Makes sense, I would just argue whether an exception should be thrown when I'm trying to read if the element has a parentNode or not, I see the dilemma though, that I was holding a reference to a document that was gone.  Perhaps in those cases, the ownerDocument (since it was destroyed) should be set to null.  

In my particular instance, I was done with the node at that point, so I just delete this.ElementCount node now and I'm done with it.

You may want to re-visit the idea that an exception is being thrown during a benign attempt to check a valid property of an object though.
Comment 66 Boris Zbarsky [:bz] 2012-09-02 11:25:47 PDT
To be clear: the behavior implemented in this bug is technically a violation of web standards.  Per standards, if you're holding a node, we should keep alive that node's document as well as all nodes in that document, that document's window, and so forth.

Of course that means that when you hold a node you leak the entire document the node came from, its window, and everything hanging off that window.

This can be a problem for web sites, even, if they do a lot of navigation in subframes and hold on to elements from those subframes.  But there we can't change anything about breaking web compat.  For extensions, though, the situation is different.  Yes, the change that was made breaks compat.  It was judged to be ok to do that in order to avoid leaks.

Specifically, the fix that was implemented in this bug is to replace the node you're holding with a totally different object that's not a node at all, so the node can go away, which it has to do so that it's document can go away.  This totally different object throws on _all_ property accesses, period, because it has no idea it's taking the place of a node.

It _would_ be possible to try for some sort of more complicated fix that would make the parentNode check work, but then you might think you have a node... whereas you do not, ad trying to do anything like inserting it into a DOM tree would fail.  So the decision was made to go for a "fail early" approach so subtle bugs don't crop up.  I realize that in your particular case that means that things broke when they might not have otherwise, and I'm sorry about that.
Comment 67 patrick 2012-10-01 18:23:25 PDT
I have an addon that holds a reference to simple storage:

var ss = require('simple-storage')

At some point in the future, when I check the quota:

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

I am now seeing:

TypeError: can't access dead object

Can I no longer hold a simple storage reference and must require('simple-storage') each time, to avoid this?
Comment 68 Boris Zbarsky [:bz] 2012-10-02 06:57:51 PDT
Patrick, is the "simple-storage" thing part of the SDK?
Comment 69 patrick 2012-10-02 08:12:26 PDT
Yes, simple-storage is part of the Addon SDK. I thought that perhaps I was holding onto a reference when I shouldn't. But it seems to me that simple-storage, as part of the SDK, should at least remain alive for the lifetime of the addon until it is disabled or uninstalled.

I am seeing exceptions in simple-storage also onunload, besides during runtime.

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

Is the dead object marking too aggressive and prematurely setting them unusable?
Comment 70 Bobby Holley (:bholley) (busy with Stylo) 2012-10-02 08:19:21 PDT
Patrick - file a separate bug and CC :gabor and poirot.alex@gmail.com?
Comment 71 patrick 2012-10-02 08:32:19 PDT
Thanks. Done in Bug 796942. But I think that the issue larger than simple-storage.
Comment 72 Boris Zbarsky [:bz] 2012-10-02 08:32:49 PDT
It sounds like an SDK bug to me where for "simple-storage" it's returning the storage of the currently loaded page, not a private storage area...  But yes, separate bug, please.
Comment 73 patrick 2012-10-02 11:45:35 PDT
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...
Comment 74 Justin Lebar (not reading bugmail) 2012-10-02 11:54:53 PDT
Patrick, what you're seeing is interesting, but it's really confusing to tackle follow-up issues in this bug.
Comment 75 patrick 2012-10-02 12:08:01 PDT
Sure. I did not want to open a new issue for each "dead object" error that I am seeing, as I believe that they are all related. It just seems that in a number instances, legitimate live objects are getting marked as dead.

In the previous case, this has no relation to the DOM or Window, and runs within the main addon code. I am not seeing the error occur every pass through the code fragment, but I believe that to be more of a timing issue.

Is there, or should there be, a general "can't access dead object" issue?
Comment 76 Justin Lebar (not reading bugmail) 2012-10-02 12:16:44 PDT
> Sure. I did not want to open a new issue for each "dead object" error that I am seeing, as I believe 
> that they are all related. It just seems that in a number instances, legitimate live objects are 
> getting marked as dead.

If you want to file just one bug for all the issues which you believe are related, or use bug 796942 for that purpose, that's a fine start.  We can always split the bug if we discover that the issues are unrelated.  (Obviously, by "X and Y are related", I mean something more than "X and Y are both caused by bug 695480"; there are tons of ways to cause this error, but your issue is that you believe we're throwing that error when we shouldn't be.)
Comment 77 patrick 2012-10-02 12:23:29 PDT
Yes, great. I'll use bug 796942. Thanks.

Note You need to log in before you can comment on or make changes to this bug.