xpcom/proxy refactoring

RESOLVED FIXED in mozilla1.9alpha1

Status

()

--
enhancement
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 226501 [details] [diff] [review]
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.
Attachment #226501 - Flags: review?(darin)

Comment 2

12 years ago
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
Attachment #226501 - Flags: review?(darin) → review+
(Assignee)

Comment 3

12 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
This seems to have caused a leak increase on balsa (both tests).
(Assignee)

Comment 5

12 years ago
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.
(Assignee)

Comment 6

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

Updated

12 years ago
Depends on: 344623

Comment 7

12 years ago
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

12 years ago
(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

12 years ago
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

12 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

12 years ago
That's bug 344652
Depends on: 344652
(Assignee)

Comment 12

12 years ago
I've fixed the warning (using .get()). The blocking bugs cover the other outstanding issues.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 345142
(Assignee)

Updated

12 years ago
Blocks: 349002
You need to log in before you can comment on or make changes to this bug.