I was debugging the issues that arise with the patch in bug 300562, and here's what I think is going on: The way wrapper preservation works is by stashing away wrappers for nodes that might need to be marked and marking them when an XPCWrappedNative for a node that they can be reached from is marked. Now in the testcase I posted in that bug (this bug's URL field), we compile the event handler on the button and preserve its wrapper. So now if a wrapper for any node in that document is reachable, the button's wrapper will be marked. But why would a node in that document be reachable? No script in the document holds reference to any nodes. Chrome doesn't hold references to any nodes. The window is reachable (it's the global object for the JSContext, hence a GC root), but the window is not a node. The reason this house of cards sort of stands up in the pre-XPCNativeWrapper world is that when the "document" property of a window is resolved we define it on the window with JSPROP_READONLY | JSPROP_ENUMERATE (see nsWindowSH::OnDocumentChanged). Note lack of JSPROP_SHARED. This means that once someone has looked up window.document we have the document's wrapper as a prop on the window's wrapper, so after that point the Document node is reachable and wrapper preservation works for nodes in that document. And apparently someone in our chrome consistently looks up window.document on content documents, which makes the whole thing work. With XPCNativeWrapper, we're no longer calling nsWindowSH::NewResolve for XPCNativeWrapper(window).document lookups, of course. But without brendan's patch, the XPCNativeWrapper for the window had the XPCNativeWrapper for the document as a property on it, so the document is still reachable and things are still ok. With brendan's patch, that's no longer the case, and in my testcase the whole set of wrappers for the document gets GCed. Once bug 296967 is fixed XPCNativeWrapper will be calling nsWindowSH::NewResolve, so this patch should become safe to check in at that point (since the pre-XPCNativeWrapper status quo will be restored). But I really don't like this dependency on something in our chrome getting window.document. In particular, in an embedding app that won't be happening... And in fact, I just asked a Camino user to test current trunk Camino on that testcase. It fails, and Camino sure isn't using any XPCNativeWrappers. So we really need to fix this part.

I believe we need this fixed for 1.8b4.... I tried adding a Mark hook to nsWindowSH, but the problem there is that I don't have an obvious way to check whether a wrapper already exists for the document. If one does not, then attempting to create said wrapper in the hook leads to a call to JS_NewObject while in GC, which naturally asserts in the JS engine. Perhaps the PostCreate hook for nsDocumentSH should do something to define the document as a property on the window? Have to be careful to make sure the document is the "current document" for the window, but other than that...

Comment on attachment 189774 [details] [diff] [review] Proposed fix per comment 1 - In nsDocumentSH::PostCreate(): + JSObject* parentObj = ::JS_GetParent(cx, obj); + NS_ENSURE_TRUE(parentObj, NS_ERROR_UNEXPECTED); + + JSClass* parentClass = JS_GET_CLASS(cx, parentObj); + NS_ENSURE_TRUE(parentClass && (parentClass->flags & JSCLASS_HAS_PRIVATE) && + (parentClass->flags & JSCLASS_PRIVATE_IS_NSISUPPORTS), + NS_ERROR_UNEXPECTED); + + nsCOMPtr<nsIXPConnectWrappedNative> winNative = + do_QueryInterface((nsISupports*)::JS_GetPrivate(cx, parentObj)); + NS_ENSURE_TRUE(winNative, NS_ERROR_UNEXPECTED); The code: sXPConnect->GetWrappedNativeOfJSObject(cx, parentObj, getter_AddRefs(wrapper)); will do all of the above for you. + nsCOMPtr<nsIDOMWindow> win = do_QueryWrappedNative(winNative); + NS_ENSURE_TRUE(win, NS_ERROR_UNEXPECTED); + + nsCOMPtr<nsIDOMDocument> currentDoc; + win->GetDocument(getter_AddRefs(currentDoc)); + if (doc == currentDoc) { Come to think of it, wouldn't checking if the document in the wrapped document's script global object equals the wrapped document be much less code? And have fewed dependencies on JSObject parentage etc? r- with the assumption that that's true...

Comment on attachment 189820 [details] [diff] [review] Per jst's comments Wrong diff.

Comment on attachment 189821 [details] [diff] [review] Real thing + nsCOMPtr<nsIDOMWindow> win = + do_QueryInterface(doc->GetScriptGlobalObject()); + if (!win) { + // No window, nothing else to do here + return NS_OK; + } + + nsCOMPtr<nsIDOMDocument> currentDoc; + win->GetDocument(getter_AddRefs(currentDoc)); Use nsPIDOMWindow and call GetExtantDocument() on it, less reference counting (returns a raw pointer), and it guarantees that we don't create a document in win if there isn't one already. r=jst with that.

So, yeah, I knew about this problem and I had a very different fix for it in bug 241518, which I was expecting to land much sooner after I wrote it. Perhaps an alternative is to land the nsIDOMGCParticipant and PreserveWrapper changes from that patch (in particular, AppendReachableList), but not the event listener + nsISupportsWeakReference parts. I'm certainly getting tired of merging those changes forward.

Well, I knew about it, at least, but also knew that it wasn't a problem because the 'document' property was reliably defined.

If those changes are ready to go, then by all means. I'd be just as happy without this hack... I do think we want to do something here soonish -- this bug is a pain in the behind in camino and other embedding apps... and it's blocking us properly GCing things when chrome accesses them through XPCNativeWrapper.

Comment on attachment 189821 [details] [diff] [review] Real thing sr=dbaron The other patch has been pretty much ready for a while, but it's probably a little large for checking in now.

Comment on attachment 189821 [details] [diff] [review] Real thing Requesting 1.8b4 approval.

Fixed.