Closed Bug 580450 Opened 12 years ago Closed 12 years ago

Clean up IPC::URI


(Core :: IPC, defect)

Windows CE
Not set





(Reporter: jdm, Assigned: jdm)




(1 file, 1 obsolete file)

>+++ b/netwerk/ipc/NeckoMessageUtils.h
>+  // The contained URI is already addrefed on creation. We don't want another
>+  // addref when passing it off to its actual owner.
>+  operator nsCOMPtr<nsIURI>() const { return already_AddRefed<nsIURI>(mURI); }

This is leak-prone.  For example, just in this patch we leak URIs if
CookieServiceParent::RecvGetCookieString happens when !mCookieService.  Not
only that, but f you make the mistake of assigning to two different nsCOMPtrs
you end up double-releasing.

Are we really that worried about the performance impact of refcounting the URIs
when passing them across the ipdl boundary?  As implemented right now we have
no addref/release on the content side, but on the chrome side we have:

1)  An addref when creating.
2)  An addref/release pair when assigning to an nsCOMPtr
3)  A release when that nsCOMPtr goes out of scope.

If we made the member an nsCOMPtr and just had the operator() return nsIURI*,
we would have an addref/release on the content side, but no change in number of
addrefs/releases on the chrome side.  If we made operator() return
already_AddRefed<nsIURI> (by returning mURI.forget()), we would eliminate one
addref/release pair on the chrome side.  In either case, Read() would swap()
into mURI.

I'd prefer one or the other of the above solutions to what we have now.

>+  URI& operator=(URI&);

This is purposefully unimplemented, right?  Should that be documented?

Should the copy ctor similarly be private and unimplemented?
Attached patch Patch (obsolete) — Splinter Review
Comments addressed.  The |nsCOMPtr<nsIURI> uri = ipcuri| pattern no longer compiles, so all future code must be reworked to use either |nsCOMPtr<nsIURI> uri; uri = ipcuri| or |nsCOMPtr<nsIURI> uri(ipcuri)|.
Assignee: nobody → josh
Attachment #460312 - Flags: review?(dwitte)
Comment on attachment 460312 [details] [diff] [review]

Looks good! r=dwitte
Attachment #460312 - Flags: review?(dwitte) → review+
tracking-fennec: --- → ?
OS: Linux → Windows CE
Keywords: checkin-needed
Attached patch PatchSplinter Review
The previous version caused build failures on OSX and Maemo 5 due to an odd compiler decision.  Apparently on these platforms, anytime an IPC::URI is constructed from an nsIURI the copy constructor |URI(const URI&)| is also invoked on the temporary.  This obviously makes it impossible to make the copy constructor private and unimplemented, so I've reverted that change.  Ehsan also pointed out that the IPC::URI calls are implicit, so I've removed them for any uses that solely involve nsIURIs (as opposed to nsCOMPtrs, which are not automatically converted for some mysterious reason).  Anyways, I'm carrying forward the r+ because the changes are basically superficial.
Attachment #460312 - Attachment is obsolete: true
Attachment #462584 - Flags: review+
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.