867098 - Don't implicitly convert to already_AddRefed in js/xpconnect/

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch that just dont_AddRef()s

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.

Comment 2

[Boris Zbarsky [: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?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[Boris Zbarsky [:bzbarsky]]

Hate, hate, hate.

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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

[Bobby Holley (:bholley)]

Comment on attachment 743555 [details] [diff] [review]
Patch that just dont_AddRef()s

sounds like Ms2ger has laready looked at the patch.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 743555 [details] [diff] [review]
Patch that just dont_AddRef()s

Peter is responsible for this all.

Comment 8

[Peter Van der Beken [: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.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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?

Comment 10

[Peter Van der Beken [: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(...)|.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v2

Green try: https://tbpl.mozilla.org/?tree=Try&rev=824d1dd7f6fa

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03e980e8dcb4

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03e980e8dcb4