301316 - Wrapper preservation depends on chrome script accessing window.document

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix per comment 1

Comment 3

[Johnny Stenback (:jst)]

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 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Per jst's comments

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Wrong diff.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Real thing

Comment 7

[Johnny Stenback (:jst)]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189821 [details] [diff] [review]
Real thing

Requesting 1.8b4 approval.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed.