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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Flags: in-testsuite-
Attached patch Patch that just dont_AddRef()s (obsolete) — Splinter Review
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 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)
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?
Hate, hate, hate.
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 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 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 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-
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)
(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)
Attached patch Patch v2Splinter Review
Green try: https://tbpl.mozilla.org/?tree=Try&rev=824d1dd7f6fa
Attachment #743555 - Attachment is obsolete: true
Attachment #749198 - Flags: review?(peterv)
Attachment #749198 - Flags: review?(peterv) → review+
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
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.

Attachment

General

Created:
Updated:
Size: