342311 - xpcom/proxy refactoring

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Benjamin Smedberg]

This bug will track some xpcom/proxy refactoring that I did on the BSMEDBERG_20060330_XPTCALL_BRANCH as part of that xptcall refactoring. These conflicted badly with the threadmanager landing, so it's basically a rewritten patch. It will make the xptcall change itself a lot less painful to review to get these changes landed first.

Comment 1

[Benjamin Smedberg]

Attachment: xpcom/proxy refactoring, rev. 1

Summary of changes:

The proxyobjectmanager maintains a hashtable of nsProxyObject instances keyed on object/eventtarget/proxytype. This hash is protected by the POM monitor.

The nsProxyObject is the canonical nsISupports of the proxied object. Each nsProxyObject holds a strong reference to the nsProxyObjectManager singleton to guarantee that it will not be deleted while there are outstanding proxy references. The nsProxyObject implements a special Release() method which ensures that the final release of the proxied object occurs on the correct event target.

Each nsProxyObject maintains a singly-linked list of nsProxyEventObject instances which represent individual interfaces of the proxied object. This list is protected by the global POM monitor. Each nsProxyEventObject holds a strong reference to its parent nsProxyObject.

nsProxyEventClass objects cache nsIInterfaceInfo and method-info for a particular IID. They are not refcounted objects. nsProxyEventObject objects hold a raw pointer to their nsProxyEventClass because once a nsProxyEventClass is created, it lives as long as the global POM.

Each proxied call is represented by a single nsProxyObjectCallInfo object, which implements nsRunnable directly. Each nsProxyObjectCallInfo holds a strong reference to its parent nsProxyEventObject.

nsProxyEvent.h has been combined with nsProxyEventPrivate.h which now contains the class declarations for all the participating classes.

Comment 2

[Darin Fisher]

Comment on attachment 226501 [details] [diff] [review]
xpcom/proxy refactoring, rev. 1

>Index: xpcom/proxy/src/nsProxyEvent.cpp

>+NS_IMETHODIMP
>+nsProxyCallCompletedEvent::QueryInterface(REFNSIID aIID, void **aResult)
> {
>+    if (aIID.Equals(kFilterIID)) {
>+        *aResult = mInfo;
>+        mInfo->AddRef();
>         return NS_OK;
>     }
>+    return nsRunnable::QueryInterface(aIID, aResult);
>+}

It's probably a good idea to add a beefy comment here explaining why
the reader should not be alarmed by this violation of QueryInterface's
API contract.

I didn't give this the most thorough review, but it mostly looks fine
to me.  r=darin

Comment 3

[Benjamin Smedberg]

Fixed on trunk.

Comment 4

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

This seems to have caused a leak increase on balsa (both tests).

Comment 5

[Benjamin Smedberg]

Hrm, this seems to be entirely 24 proxies for nsUrlClassifierCallbackWrapper, with associated nsProxyObject and nsXPCWrappedNative.

I'm actually concerned that we don't seem to be leaking the nsProxyObjectManager... if we're leaking nsProxyObject we *should* be leaking the POM.

Comment 6

[Benjamin Smedberg]

Tony, can you give me an overview of how urlclassifier uses proxies?

Comment 7

[Darin Fisher]

http://lxr.mozilla.org/seamonkey/source/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp#629

Method calls made on nsUrlClassifierDBService are proxied to a background thread.  The method calls are asynchronous, so the callback is proxied to the main thread.  The callback is generally a JS object.  I know that Tony detected a memory leak with these objects on the 1.8 branch as well, so are you sure that this patch is to blame?

Comment 8

[Tony Chang (Google)]

(In reply to comment #7)
> I know that Tony detected
> a memory leak with these objects on the 1.8 branch as well, so are you sure
> that this patch is to blame?

On further inspection, it didn't like like nsProxyObjects were leaking.  At least, an equal number of constructors and destructors were called on nsProxyObjects.  I wasn't able to determine if it was a real leak I was seeing or just memory fragmentation.

Comment 9

[Peter Weilbacher]

This patch caused a warning

nsProxyEventObject.cpp
X:/trunk/mozilla/xpcom/proxy/src/nsProxyEventObject.cpp: In member function `virtual nsresult nsProxyEventObject::CallMethod(short unsigned int, const nsXPTMethodInfo*, nsXPTCMiniVariant*)':
X:/trunk/mozilla/xpcom/proxy/src/nsProxyEventObject.cpp:264: warning: cannot pass objects of non-POD type `class nsRefPtr<nsProxyObjectCallInfo>' through `...'; call will abort at runtime

with GCC 3.3.5 (on OS/2) about the line
   PROXY_LOG(("PROXY(%p): PostAndWait exit [%p %x]\n", this, proxyInfo, rv));

which in older GCC releases (I tried 3.2.2) actually gets an error.

Comment 10

[Alex Fritze]

This checkin introduced a bug: Synchronous proxy calls now always return NS_OK, irrespective of the actual result of the call.

There should be a "rv = proxyInfo->GetResult();" in nsProxyEventObject::CallMethod() somewhere.

Comment 11

[Benjamin Smedberg]

That's bug 344652

Comment 12

[Benjamin Smedberg]

I've fixed the warning (using .get()). The blocking bugs cover the other outstanding issues.