Status

()

Core
IPC
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows CE
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

11.94 KB, patch
jdm
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
>+++ 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?
(Assignee)

Comment 1

8 years ago
Created attachment 460312 [details] [diff] [review]
Patch

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 2

8 years ago
Comment on attachment 460312 [details] [diff] [review]
Patch

Looks good! r=dwitte
Attachment #460312 - Flags: review?(dwitte) → review+
(Assignee)

Updated

8 years ago
tracking-fennec: --- → ?
OS: Linux → Windows CE
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 3

8 years ago
Created attachment 462584 [details] [diff] [review]
Patch

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+
(Assignee)

Comment 4

8 years ago
http://hg.mozilla.org/mozilla-central/rev/caec0c83740f
Status: NEW → RESOLVED
Last Resolved: 8 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.