256911 - SetWebBrowserChrome holds invalide nsCOMPtr refs

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[jhp (no longer active)]

This code in nsDocShellTreeOwner::SetWebBrowserChrome() is wrong:

 833                     nsCOMPtr<nsIEmbeddingSiteWindow>
ownerWin(do_QueryInterface(aWebBrowserChrome));
 834                     nsCOMPtr<nsIInterfaceRequestor>
requestor(do_QueryInterface(aWebBrowserChrome));
 835                 
 836                     // it's ok for ownerWin or requestor to be null.
 837                     mWebBrowserChrome = aWebBrowserChrome;
 838                     mOwnerWin = ownerWin;
 839                     mOwnerRequestor = requestor;

It assigns the nsCOMPtrs ownerWin and requestor to simple pointers.  So when it
goes out of scope, the nsCOMPtrs are destroyed, and mOwnerWin and
mOwnerRequestor hold no longer valid references.

Comment 1

[Darin Fisher]

Javier and I discussed this case, and we think that it might just be best to
support this broken pattern since it is so commonplace :-(

Moreover, it would be tricky to fix the code SetWebBrowserChrome code since it
is depending on holding weak references to avoid reference cycles.

We came up with a way of arranging the stub objects so that this usage can work.

Comment 2

[Darin Fisher]

Attachment: patch for ipcDConnectService.cpp

Here's the patch I applied to ipcDConnectService.cpp, which is based on
Javier's patch for nsJavaXPTCStub.cpp.

Comment 3

[timeless]

that's extremely bogus, if the object is implemented with tearoffs, then it's dead.

Comment 4

[Darin Fisher]

From #developers:

<timeless> [this bug] makes me very unhappy
<timeless> quote from my comment
<timeless> that's extremely bogus, if the object is implemented with tearoffs,
then it's dead.
<darin> timeless: it makes me unhappy too
<darin> timeless: but what can i do?  this pattern is used in a lot of places.
<timeless> track them down and shoot them
<timeless> :)
<darin> timeless: do you have time to fix them?
<darin> i don't
<timeless> i think i can find volunteers
<timeless> can you setup logging which would detect the problems and report stacks/
* timeless eyes ctho
<darin> timeless: if you want to do all of that then feel free
<timeless> i can get the volunteers, i'm not sure about the detection... clearly
you guys have some way of recognizing it
<timeless> is it just a matter of making objects auto wrapped by something?
<timeless> e.g. could we hack a build so that nsCOMPtr stores a dcom object instead?
<darin> timeless: we don't have a way of detecting the problem... instead, we
just make the lifetime of the tearoffs correspond to the lifetime of the
'master' object
<timeless> darin: would my suggestion work?
<darin> timeless: yeah, you could probably create a wrapper that has its
lifetime bound to that of the master, but when accessed talks to the tearoff
object.  the wrapper's AddRef/Release would AddRef/Release the tearoff as well
as the master.  the wrapper could then detect that it is being used with a dead
tearoff.
<timeless> could you write this up into the bug?

Comment 5

[Darin Fisher]

so, it is not possible to completely work around nsDocShellTreeItem.  we need to
fix it to take at least one owning reference to the aWebBrowserChrome parameter.

Comment 6

[jhp (no longer active)]

Attachment: embedding patch

The problem I'm having with my Java embedding work is as follows:  the
nsIWebBrowserChrome and nsIEmbeddingSiteWindow interfaces are implemented as a
Java object, which then gets passed to nsWebBrowser::SetContainerWindow().  In
order to handle this, I create a temporary XPCOM stub object to pass in. 
SetContainerWindow() calls SetWebBrowserChrome(), where the fun happens.  When
the program exits the scope of that function, the XPCOM stub object has a ref
count of 1, and mWebBrowserChrome, mOwnerWin, and mOwnerRequestor all point to
it.  But when returning back into Java, the temporary XPCOM stub object gets
released, and now those 3 supposed 'weak refs' point to nothing.

I was able to fix this by making them actual nsIWeakReferences.  This works for
my code since my implementation of weak references hold a weak ref to the
actual Java object, not a weak ref to the XPCOM stub.

This change makes it necessary for implementors of nsIWebBrowserChrome to also
implement nsISupportsWeakReference, hence the change to
gtk/src/EmbedWindow.cpp.

This patch fixes the specific issue I had, but it looks like there are other
member variables that could be changed to nsIWeakReferences.

Comment 7

[Darin Fisher]

ooooh... so, this means that embedders have to implement
nsISupportsWeakReference or suddenly all bets are off, right?  that means an
incompatible change in our frozen embedding API, right? :-(

is it possible instead to make our Java objects keep an owning reference to the
XPTC stubs somehow?

Comment 8

[jhp (no longer active)]

Attachment: embedding patch v2 (diff -w)

I discussed this with Darin and we weren't able to come up with a good solution
for fixing this on my end.  So instead, I present this patch, based on a
suggestion by Darin.  Here, we save an actual weak reference if the passed in
object implements nsISupportsWeakReference.  If not, it does what it does now. 
So this uses weak refs for my case and fixes that, but doesn't change how
current embeddors work.

Comment 9

[Justin Wood (:Callek)]

should not the IID change, though someone feel free to correct me if I am wrong
on this.

Comment 10

[Mike Shaver (:shaver emeritus)]

The IID does not need to change, IMO: the interface is both binary and
semantically compatible with what we have today (assuming that Javier tested the
current case =) ), so revving the IID would cause needless incompatibility.

I like this approach, as I discussed with Darin on IRC.  Well done.

Comment 11

[Darin Fisher]

Attachment: review comments for v2 patch

Comment 12

[jhp (no longer active)]

Attachment: embedding patch v3

New patch addressing Darin's comments.	Also, I created some helper functions
to get the appropriate member variable, depending on whether we're using
weakrefs or not.  Also, fixed several places where the code was checking the
status of the non-weakref member variables; this would fail in the weakref case
since they are always null.

Comment 13

[Darin Fisher]

Comment on attachment 162626 [details] [diff] [review]
embedding patch v3

>Index: browser/webBrowser/nsWebBrowser.cpp

> NS_IMETHODIMP nsWebBrowser::GetContainerWindow(nsIWebBrowserChrome** aTopWindow)
> {
>    NS_ENSURE_ARG_POINTER(aTopWindow);
> 
>+   if(mDocShellTreeOwner) {
>+      if (mDocShellTreeOwner->mWebBrowserChromeSupportsWeak) {
>+        return mDocShellTreeOwner->mWebBrowserChromeWeak->
>+                         QueryReferent(NS_GET_IID(nsIWebBrowserChrome),
>+                                       NS_REINTERPRET_CAST(void**, aTopWindow));
>+      }
>       *aTopWindow = mDocShellTreeOwner->mWebBrowserChrome;
>-   else
>+   } else {
>       *aTopWindow = nsnull;
>+   }
>    NS_IF_ADDREF(*aTopWindow);
> 
>    return NS_OK;   
> }

it seems like this function wants to return NS_OK even when *aTopWindow
is null.  but QueryReferent might return an error when it fails to 
return non-null.  should you make that case return NS_OK too?  my guess
is "yes"


>Index: components/windowwatcher/src/nsWindowWatcher.cpp

>+    nsCOMPtr<nsISupportsWeakReference> supportsweak(do_QueryInterface(inChrome));
>+    if (supportsweak) {
>+      mChromeSupportsWeak = PR_TRUE;
>+      supportsweak->GetWeakReference(getter_AddRefs(mChromeWeak));
>+    } else {
>+      mChromeSupportsWeak = PR_FALSE;
>+      mChrome = inChrome;
>+    }

use mChromeWeak != nsnull in place of mChromeSupportsWeak?

Comment 14

[jhp (no longer active)]

Attachment: embedding patch v3.1

New patch with Darin's suggestions.

Comment 15

[Darin Fisher]

Comment on attachment 162637 [details] [diff] [review]
embedding patch v3.1

i think you need to set mChromeWeak to null when you do "mChrome = inChrome;"

r=darin with that fixed

Comment 16

[Mike Shaver (:shaver emeritus)]

Comment on attachment 162637 [details] [diff] [review]
embedding patch v3.1

>+   nsCOMPtr<nsIWebBrowserChrome> webBrowserChrome;
>+   GetWebBrowserChrome(getter_AddRefs(webBrowserChrome));
>+
>+   NS_ENSURE_STATE(mTreeOwner || webBrowserChrome);

Given that virtually no of the callers of GetWebBrowserChrome seem to check the
result returned, and that the only failure mode is signalled by a null
WBC, I think we should just have it return an already_AddReffed<>.

> NS_IMETHODIMP
> nsDocShellTreeOwner::SetPosition(PRInt32 aX, PRInt32 aY)
> {
>-  if (mOwnerWin)
>-  {
>-    return mOwnerWin->SetDimensions(nsIEmbeddingSiteWindow::DIM_FLAGS_POSITION,
>-                                    aX, aY, 0, 0);
>+  nsCOMPtr<nsIEmbeddingSiteWindow> ownerWin;
>+  if (NS_SUCCEEDED(GetOwnerWin(getter_AddRefs(ownerWin)))) {
>+    return ownerWin->SetDimensions(nsIEmbeddingSiteWindow::DIM_FLAGS_POSITION,
>+                                   aX, aY, 0, 0);
>   }
>   return NS_ERROR_NULL_POINTER;
> }

Dominant brace style is this file seems to be
if (foo)
{
   then;
}

even for single-line then bodies.  I greatly prefer your style (when
consistently
applied to single-line then bodies following single-line if predicates), but I
think
we should maintain consistency here.

>+  } else {
>+    if (mWebBrowserChrome) {
>+      *aChrome = mWebBrowserChrome;
>+      NS_ADDREF(mWebBrowserChrome);
>+      return NS_OK;
>+    }
>+  }

Fold that to
}
else if (mWebBrowserChrome)
{
please.  (To eliminate excess block, and brace-conform.)

>+  if (mWebBrowserChromeWeak != nsnull) {
>+    return mWebBrowserChromeWeak->
>+                        QueryReferent(NS_GET_IID(nsIEmbeddingSiteWindow),
>+                                      NS_REINTERPRET_CAST(void**, aWindow));
>+  } else {
>+    if (mOwnerWin) {

No need for else-after-return.	(Here or elsewhere.)

>+    nsresult GetWebBrowserChrome(nsIWebBrowserChrome** aWindow);
>+    nsresult GetOwnerWin(nsIEmbeddingSiteWindow** aWindow);
>+    nsresult GetOwnerRequestor(nsIInterfaceRequestor** aWindow);

Some comments here about lifecycle and weak/strong ref management would be
greatly appreciated (as in the interface def, IMO).

With those changes, sr=shaver.	If you want to discuss the return type of GWBC,
I'm amenable to reasonable arguments. =)

Comment 17

[jhp (no longer active)]

Committed to trunk with Shaver's suggestions.