1322460 - Fix addref/release on function return value that clang plugin reports on Windows

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Comment 1

[Ting-Yu Chou [:ting] (away)]

Attachment: wip

Comment 2

[Ting-Yu Chou [:ting] (away)]

Attachment: Bug 1322460 - Don't addref/release on the return value of prohibited functions.

Review commit: https://reviewboard.mozilla.org/r/98528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98528/

Comment 3

[(no longer active)]

Comment on attachment 8818473 [details]
Bug 1322460 - Don't addref/release on the return value of prohibited functions.

https://reviewboard.mozilla.org/r/98528/#review98882

::: accessible/windows/uia/uiaRawElmProvider.cpp:60
(Diff revision 1)
>    if (mAcc->IsDefunct())
>      return CO_E_OBJNOTCONNECTED;
>  
>    *aIdChild = CHILDID_SELF;
>    *aAcc = mAcc;
> -  mAcc->AddRef();
> +  mAcc.get()->AddRef();

Instead of doing this, I prefer if you did:

  RefPtr<AccessibleWrap> copy(mAcc);
  copy.forget(aAcc);

::: ipc/mscom/DispatchForwarder.cpp:104
(Diff revision 1)
>  {
>    // ITypeInfo as implemented by COM is apartment-neutral, so we don't need
>    // to wrap it (yay!)
>    if (mTypeInfo) {
>      *ppTInfo = mTypeInfo.get();
> -    mTypeInfo->AddRef();
> +    mTypeInfo.get()->AddRef();

Similarly, I prefer if you made a copy RefPtr and forget() that with ppTInfo.

::: ipc/mscom/Interceptor.cpp:239
(Diff revision 1)
>      if (entry && entry->mInterceptor) {
>        unkInterceptor = entry->mInterceptor;
>      } else {
>        // We're inserting unkInterceptor into the map but we still want to hang
>        // onto it locally so that we can QI it below.
> -      unkInterceptor->AddRef();
> +      unkInterceptor.get()->AddRef();

Hmm.  Wouldn't it be better if we made MapEntry::mInterceptor a RefPtr?

I think you should probably consult aklotz on this part of the patch...

::: ipc/mscom/WeakRef.cpp:124
(Diff revision 1)
>    if (FAILED(hr)) {
>      return hr;
>    }
>  
>    mWeakRefs.AppendElement(weakRef.get());
> -  weakRef->AddRef();
> +  weakRef.get()->AddRef();

Similarly here, instead of this (and the corresponding Release in ClearWeakRefs()), wouldn't it be better to make mWeakRefs be nsTArray<RefPtr<WeakRef>> instead?  Again, aklotz would be the right person to talk to here too.

Comment 4

[Ting-Yu Chou [:ting] (away)]

What do you think about ehsan's comments in comment 3 (the changes for Interceptor.cpp and WeakRef.cpp)?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

We have to be extremely careful with managing refcounts in mscom code, as some pointers are moved around in a background thread but the objects themselves (and their refcounts) are not safe to manipulate from there.

I'll have to take a closer look at this.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

OK, the following is safe:

* In ipc/mscom/WeakRef.h, you may convert WeakReferenceSupport::mWeakRefs to be a nsTArray<RefPtr<WeakRef>> and make any necessary changes to make that work.

* In ipc/mscom/Interceptor.h, MapEntry::mInterceptor may be converted to a RefPtr<IUnknown>, but MapEntry::mTargetInterface must *NOT* be converted.

* I agree with Ehsan about using the RefPtr/forget idiom for the change in DispatchForwarder (and elsewhere).

Also, I'd like to see any and all reviews that touch mscom code in the future. Thanks.

Comment 7

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8818473 [details]
Bug 1322460 - Don't addref/release on the return value of prohibited functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98528/diff/1-2/

Comment 8

[(no longer active)]

Comment on attachment 8818473 [details]
Bug 1322460 - Don't addref/release on the return value of prohibited functions.

https://reviewboard.mozilla.org/r/98528/#review99434

r+ on the non-mscom parts.  Thanks!

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8818473 [details]
Bug 1322460 - Don't addref/release on the return value of prohibited functions.

https://reviewboard.mozilla.org/r/98528/#review100868

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ce2094b181f
Don't addref/release on the return value of prohibited functions. r=aklotz,Ehsan

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ce2094b181f