Closed
Bug 867098
Opened 11 years ago
Closed 11 years ago
Don't implicitly convert to already_AddRefed in js/xpconnect/
Categories
(Core :: XPConnect, enhancement)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: ayg, Assigned: ayg)
References
Details
Attachments
(1 file, 1 obsolete file)
3.06 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Flags: in-testsuite-
Assignee | ||
Comment 1•11 years ago
|
||
I don't have the foggiest idea what this code is doing or how "identity" is supposed to be refcounted, so I don't know if this fix is the right way to do it at all.
Attachment #743555 -
Flags: review?(bzbarsky)
Comment 2•11 years ago
|
||
Comment on attachment 743555 [details] [diff] [review] Patch that just dont_AddRef()s I can't tell why this code makes any sense even before your changes. Maybe Bobby can?
Attachment #743555 -
Flags: review?(bzbarsky) → review?(bobbyholley+bmo)
Comment 3•11 years ago
|
||
The way it works is that XPCWrappedNative::GetNewOrUsed gets a xpcObjectHelper& helper, and does: nsISupports *identity = helper.GetCanonical(); ... foo = new XPCWrappedNative(identity, Scope, set); ... helper.forgetCanonical(); ... fun, right?
Comment 4•11 years ago
|
||
Hate, hate, hate.
Comment 5•11 years ago
|
||
Can the XPCWrappedNative be changed to take a reference to an xpcObjectHelper, then the constructor would call GetCanonical? It seems to me that would be less weird. Though that wouldn't work for the usage in XPCWrappedNative::Morph().
Comment 6•11 years ago
|
||
Comment on attachment 743555 [details] [diff] [review] Patch that just dont_AddRef()s sounds like Ms2ger has laready looked at the patch.
Attachment #743555 -
Flags: review?(bobbyholley+bmo) → review?(Ms2ger)
Comment 7•11 years ago
|
||
Comment on attachment 743555 [details] [diff] [review] Patch that just dont_AddRef()s Peter is responsible for this all.
Attachment #743555 -
Flags: review?(Ms2ger) → review?(peterv)
Comment 8•11 years ago
|
||
Comment on attachment 743555 [details] [diff] [review] Patch that just dont_AddRef()s Review of attachment 743555 [details] [diff] [review]: ----------------------------------------------------------------- Since |new XPCWrappedNative| is now infallible you should just pass in forgetCanonical() into the XPCWrappedNative constructor.
Attachment #743555 -
Flags: review?(peterv) → review-
Assignee | ||
Comment 9•11 years ago
|
||
Sorry -- could you tell me more precisely what you want the code to look like? I have no idea what any of this stuff does. Do you mean that in the three cases where "identity" is passed to the constructor, I should change it to "nativeHelper.forgetCanonical()" (which will set nativeHelper's mCanonical and mCanonicalStrong to nullptr)? Or something else?
Flags: needinfo?(peterv)
Comment 10•11 years ago
|
||
(In reply to :Aryeh Gregor from comment #9) > Do you mean that in the > three cases where "identity" is passed to the constructor, I should change > it to "nativeHelper.forgetCanonical()" (which will set nativeHelper's > mCanonical and mCanonicalStrong to nullptr)? Yes. forgetCanonical() returns already_AddRefed<nsISupports>, which the XPCWrappedNative constructor takes. You should also remove the existing calls to forgetCanonical() and the null-checking after |new XPCWrappedNative(...)|.
Flags: needinfo?(peterv)
Assignee | ||
Comment 11•11 years ago
|
||
Green try: https://tbpl.mozilla.org/?tree=Try&rev=824d1dd7f6fa
Attachment #743555 -
Attachment is obsolete: true
Attachment #749198 -
Flags: review?(peterv)
Updated•11 years ago
|
Attachment #749198 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 12•11 years ago
|
||
My dev machine's root filesystem decided to die two days ago, so I'd appreciate it if someone else could check this in for me.
Keywords: checkin-needed
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/03e980e8dcb4
Keywords: checkin-needed
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/03e980e8dcb4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•