1359455 - Require passing getter_AddRefs to T**-style outparams

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

In bug 1357872 a bug was fixed where window objects were being leaked due to them being fetched using an XPCOM getter, where instead of using getter_AddRefs, a reference to a stack allocated raw pointer was used. 

As almost all parameters of type `T**` where T is refcountable should be called with `getter_AddRefs` to avoid leaks, we should have a static analysis to ensure this.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to ensure that getter_AddRefs is passed to T**-style outparams

MozReview-Commit-ID: 8mDpDCWZwLw

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Mark getter_AddRefs implementations as MOZ_GETTER_ADDREFS_TYPE

MozReview-Commit-ID: JPDGTkCsTw7

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Mark inout params generated in xpidl as MOZ_NON_OUTPARAM

MozReview-Commit-ID: 5yAAyAehyzU

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Ensure that getter_AddRefs is passed to T**-style outparams in layout

MozReview-Commit-ID: 9ik6pvkg81r

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Ensure that getter_AddRefs is passed to T**-style outparams in dom

MozReview-Commit-ID: KQrt8buHYtf

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Ensure that getter_AddRefs is passed to T**-style outparams in js

MozReview-Commit-ID: JZLTB3nNGyT

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Ensure that getter_AddRefs is passed to T**-style outparams in toolkit

MozReview-Commit-ID: EP39mVQOJhi

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Ensure that getter_AddRefs is passed to T**-style outparams in rdf

MozReview-Commit-ID: F4lBfc1c2VL

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Ensure that getter_AddRefs is passed to T**-style outparams in editor

MozReview-Commit-ID: 4QlVb4DN1cb

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 10: Ensure that getter_AddRefs is passed to T**-style outparams in gfx

MozReview-Commit-ID: AICO5EtpVfF

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 11: Ensure that getter_AddRefs is passed to T**-style outparams in accessible

MozReview-Commit-ID: BfSb58yIjcb

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 12: Ensure that getter_AddRefs is passed to T**-style outparams in parser/html

MozReview-Commit-ID: 3tAEsWFMMjd

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 13: Ensure that getter_AddRefs is passed to T**-style outparams in mtransport

MozReview-Commit-ID: IrZrvrW4Wf6

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 14: Ensure that getter_AddRefs is passed to T**-style outparams in security

MozReview-Commit-ID: 44DLRqm4YAn

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 15: Ensure that getter_AddRefs is passed to T**-style outparams in XPCOM

MozReview-Commit-ID: Eo2Uu0QkVpB

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 16: Ensure that getter_AddRefs is passed to T**-style outparams in netwerk

MozReview-Commit-ID: 2L4uhC1kZkB

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 17: Ensure that getter_AddRefs is passed to T**-style outparams in image

MozReview-Commit-ID: G4AJtHdbVe7

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 18: Make GetThrottleQueue follow XPCOM calling conventions

MozReview-Commit-ID: 4wasncHXzWI

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 19: Avoid leaking data in error cases in nsXULContentSink

MozReview-Commit-ID: F6ZqwDCxunf

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 20: Ensure that getter_AddRefs is passed to T**-style outparams in various other places

MozReview-Commit-ID: BsdVvdfXlLv

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8862995 [details] [diff] [review]
Part 17: Ensure that getter_AddRefs is passed to T**-style outparams in image

Review of attachment 8862995 [details] [diff] [review]:
-----------------------------------------------------------------

Deferring to Andrew

Comment 22

[Eric Rescorla (:ekr)]

Comment on attachment 8862990 [details] [diff] [review]
Part 13: Ensure that getter_AddRefs is passed to T**-style outparams in mtransport

Review of attachment 8862990 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't seem like the right answer to your problem.

I get that your static analysis is confused by this idiom, but the right answer seems to be to add an "I know what I'm doing" annotation that your static analysis can process (or, if you prefer, an exemption for this code, which, after all, is in a unit test).

Comment 23

[(no longer active)]

Comment on attachment 8862977 [details] [diff] [review]
Part 1: Add an analysis to ensure that getter_AddRefs is passed to T**-style outparams

Review of attachment 8862977 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice, thank you very much!

::: build/clang-plugin/GetterAddrefsChecker.cpp
@@ +12,5 @@
> +  AstMatcher->addMatcher(
> +    callExpr(forEachArgumentWithParam(
> +               expr().bind("expr"),
> +               parmVarDecl(pointerType(pointee(pointerType(pointee(isRefCounted()))))))));
> +  */

Stale comment?

::: build/clang-plugin/tests/TestGetterAddrefs.cpp
@@ +63,5 @@
> +
> +  AddrefUnawareType<RCAble*>().CallOnPointerToT(&rawPtr);
> +
> +  NotOutparamType(&rawPtr);
> +  NotOutparamType(getter_AddRefs(smartPtr)); // XXX: Maybe error here?

I think these would be nice as well, can you please file follow-ups and include the bug numbers here?

Comment 24

[(no longer active)]

(In reply to Eric Rescorla (:ekr) from comment #22)
> Comment on attachment 8862990 [details] [diff] [review]
> Part 13: Ensure that getter_AddRefs is passed to T**-style outparams in
> mtransport
> 
> Review of attachment 8862990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem like the right answer to your problem.
> 
> I get that your static analysis is confused by this idiom, but the right
> answer seems to be to add an "I know what I'm doing" annotation that your
> static analysis can process (or, if you prefer, an exemption for this code,
> which, after all, is in a unit test).

That is what the MOZ_DOES_NOT_ADDREF annotation does here.

Comment 25

[Eric Rescorla (:ekr)]

I meant an annotation that does not require the cast(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #24)
> (In reply to Eric Rescorla (:ekr) from comment #22)
> > Comment on attachment 8862990 [details] [diff] [review]
> > Part 13: Ensure that getter_AddRefs is passed to T**-style outparams in
> > mtransport
> > 
> > Review of attachment 8862990 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This doesn't seem like the right answer to your problem.
> > 
> > I get that your static analysis is confused by this idiom, but the right
> > answer seems to be to add an "I know what I'm doing" annotation that your
> > static analysis can process (or, if you prefer, an exemption for this code,
> > which, after all, is in a unit test).
> 
> That is what the MOZ_DOES_NOT_ADDREF annotation does here.

I  meant an annotation that does not require the cast to void *.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8862997 [details] [diff] [review]
Part 19: Avoid leaking data in error cases in nsXULContentSink

Gah.  Good thing we don't hit those in practice...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8862998 [details] [diff] [review]
Part 20: Ensure that getter_AddRefs is passed to T**-style outparams in various other places

>+  RefPtr<iface_> inst;                                    \

Can that not be an nsCOMPtr?

>+  nsCOMPtr<nsIFile> localFile = nullptr;

No need for the "= nullptr" bit.

>+    localFile.forget(aFile);

You could put this after the "exists" check, and make that use localFile instead of *aFile, and then you don't need the manual NS_RELEASE(*aFile) either.

r=me

Comment 28

[Nika Layzell [:nika] (ni? for response)]

(In reply to Eric Rescorla (:ekr) from comment #25)
> I  meant an annotation that does not require the cast to void *.

I have a new patch which I will attach shortly which avoids this cast to (void *). Basically the reason for the problem in the first place was that WrapRunnableRet creates a nsIRunnable which calls a method pointer with an opaque data source which the static analysis cannot understand. It cannot see any annotations on called method, nor does it know anything about the source of the data.

The new version of this patch instead uses a NS_NewRunnableFunction and a lambda to make it clear what is being called and with what parameters to the static analysis (as it isn't calling through a function pointer anymore).

Comment 29

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 13: Ensure that getter_AddRefs is passed to T**-style outparams in mtransport

This updated version doesn't static_cast to a void*

MozReview-Commit-ID: IrZrvrW4Wf6

Comment 30

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 20: Ensure that getter_AddRefs is passed to T**-style outparams in various other places

MozReview-Commit-ID: BsdVvdfXlLv

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to ensure that getter_AddRefs is passed to T**-style outparams

MozReview-Commit-ID: 8mDpDCWZwLw

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8863030 [details] [diff] [review]
Part 1: Add an analysis to ensure that getter_AddRefs is passed to T**-style outparams

Review of attachment 8863030 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/GetterAddrefsChecker.cpp
@@ +4,5 @@
> +
> +#include "GetterAddrefsChecker.h"
> +#include "CustomMatchers.h"
> +
> +void GetterAddrefsChecker::registerMatchers(MatchFinder* AstMatcher) {

Drive-by nit on this part: throughout this patch, you're using the spelling "GetterAddrefs" (with a lowercase "r" in "ref").  But perhaps this should really be GetterAddRefs (with capital R)?

We capitalize the "R" when spelling the functions AddRef() and getter_AddRefs().  A local grep only finds 26 instances of the "Addref" spelling (with uppercase "A" and lowercase "r") -- and nearly all of those are in comments.  Only 8 hits are in code, I think.

So: probably worth sticking with the prevailing spelling, particularly since this is about "getter_AddRefs" which uses a capital R in its spelling?  Hopefully this is easy to fix via search-and-replace in the patch...

Comment 33

[(no longer active)]

(In reply to Daniel Holbert [:dholbert] from comment #32)
> Drive-by nit on this part: throughout this patch, you're using the spelling
> "GetterAddrefs" (with a lowercase "r" in "ref").  But perhaps this should
> really be GetterAddRefs (with capital R)?

Yes, thanks for spotting this!

Comment 34

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8862994 [details] [diff] [review]
Part 16: Ensure that getter_AddRefs is passed to T**-style outparams in netwerk

Review of attachment 8862994 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache/nsCacheService.cpp
@@ +1895,5 @@
>  {
>      NS_ASSERTION(request->mThread, "no thread set in async request!");
>  
>      // Swap ownership, and release listener on target thread...
> +    

nit: whitespace

@@ +2013,1 @@
>          *result = descriptor;

this line should be removed.

Comment 35

[Benjamin Smedberg]

Comment on attachment 8862983 [details] [diff] [review]
Part 7: Ensure that getter_AddRefs is passed to T**-style outparams in toolkit

I can mark r+ on everything except the nsAppRunner bits with the REVIEWER-NOTE.

I expect that for profile reset, we *intentionally* hold the lock forever, and so the change here to hold it only temporarily is a regression. But I didn't review that originally, and it does mean we're intentionally leaking a C++ object also, which only doesn't show up because AFAIK we don't have automated tests for profile reset. We'd need to talk to MattN about the right way to do that.

Comment 36

[Nika Layzell [:nika] (ni? for response)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #35)
> Comment on attachment 8862983 [details] [diff] [review]
> Part 7: Ensure that getter_AddRefs is passed to T**-style outparams in
> toolkit
> 
> I can mark r+ on everything except the nsAppRunner bits with the
> REVIEWER-NOTE.
> 
> I expect that for profile reset, we *intentionally* hold the lock forever,
> and so the change here to hold it only temporarily is a regression. But I
> didn't review that originally, and it does mean we're intentionally leaking
> a C++ object also, which only doesn't show up because AFAIK we don't have
> automated tests for profile reset. We'd need to talk to MattN about the
> right way to do that.

Are we intentionally holding this lock forever in the profile reset code?

Comment 37

[Nika Layzell [:nika] (ni? for response)]

The reason why I thought that it wasn't intentional was because the comment above mentioned:
   // Check that the source profile is not in use by temporarily acquiring its lock.

Comment 38

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8862987 [details] [diff] [review]
Part 11: Ensure that getter_AddRefs is passed to T**-style outparams in accessible

># HG changeset patch
># User Michael Layzell <[e-mail]>
>
>Bug 1359455 - Part 11: Ensure that getter_AddRefs is passed to T**-style outparams in accessible, r=tbsaunde

not really an accurate description of what it does, so please fix that.  It also doesn't explain any of the why.

>    * Returns a text point for the accessible element.
>    */
>-  void ToTextPoint(HyperTextAccessible** aContainer, int32_t* aOffset,
>-                   bool aIsBefore = true) const;
>+  void ToTextPoint(HyperTextAccessible** MOZ_DOES_NOT_ADDREF aContainer,
>+                   int32_t* aOffset, bool aIsBefore = true) const;

for non xpcom stuff it would be nice if this was in the type system vs shoved in an attribute, but this could then also just return a pair of Accessible* and int.

anyway whatever its fine for now.

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8862980 [details] [diff] [review]
Part 4: Ensure that getter_AddRefs is passed to T**-style outparams in layout

Review of attachment 8862980 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following.

(Only two of my comments here are things that I'd actively like for you to change (which I've tagged as ISSUE), and there are several other notes for chunks that may require rebasing/removing depending on whether some spinoff bugs land before this.)

::: layout/base/AccessibleCaretManager.h
@@ +184,5 @@
>    // well as the range start node and the node offset. Otherwise, get the frame
>    // and offset for the range end in the last range instead.
>    nsIFrame* GetFrameForFirstRangeStartOrLastRangeEnd(
> +    nsDirection aDirection, int32_t* aOutOffset,
> +    nsINode** MOZ_DOES_NOT_ADDREF aOutNode = nullptr,

I filed bug 1361072 on this.  At first glance, I don't think our lack-of-addreffing here is actually useful, because there's only one caller that uses this param, and that caller immediately copies the result into an addref'ed variable after the function call.

If TYLin gets to that bug before this lands, then you can get rid of this chunk. Otherwise we can revert this annotation in bug 1361072.

::: layout/base/nsBidiPresUtils.h
@@ +413,5 @@
>     * @return  true if it finds that bidi is (or may be) required,
>     *          false if no potentially-bidi content is present.
>     */
>    static bool ChildListMayRequireBidi(nsIFrame*    aFirstChild,
> +                                      nsIContent** MOZ_NON_OUTPARAM aCurrContent);

ISSUE: I think this should be MOZ_DOES_NOT_ADDREF, instead of MOZ_NON_OUTPARAM.

You're clearly needing *some* annotation here, to exempt this param from analysis -- but "not-an-outparam" seems inaccurate.  aCurrContent really *is* really a sort of outparam (it's an in/out param, as the documentation above it indicates with "[in/out]"). So, MOZ_NON_OUTPARAM is confusing/contradictory.

The reason it's safe to skip the getter_AddRefs/refcounting dance here is as follows: this is a short helper function which simply walks part of the frame tree during layout and does not run script or modify the DOM tree, so we know that the nsIContent objects involved here will definitely stay alive through this function's callers.  So, not-addreffing (and annotating with MOZ_DOES_NOT_ADDREF) seems fine & justified.

::: layout/base/nsFrameManager.cpp
@@ +116,5 @@
>    void  Clear();
>  
>  protected:
> +  LinkedList<UndisplayedNode>* GetListFor(nsIContent** MOZ_NON_OUTPARAM aParentContent);
> +  LinkedList<UndisplayedNode>* GetOrCreateListFor(nsIContent** MOZ_NON_OUTPARAM aParentContent);

I just filed bug 1361086 with a patch to simplify these functions.  (There's no reason for these to use T** -- they can just be T*.)

Hopefully that'll land shortly -- if it makes it in before this patch-stack, you can remove this chunk.

::: layout/build/nsLayoutModule.cpp
@@ +396,5 @@
>  {                                                         \
>    *aResult = nullptr;                                      \
>    if (aOuter)                                             \
>      return NS_ERROR_NO_AGGREGATION;                       \
> +  RefPtr<iface_> inst;                                    \

ISSUE: I think this should be nsCOMPtr, not RefPtr.

It looks like every invocation of this macro has "iface_" being an xpcom interface (nsIFoo of some sort), and we typically use nsCOMPtr for nsIFoo (xpcom interfaces) vs. RefPtr for nsFoo (concrete types).

::: layout/xul/tree/TreeBoxObject.cpp
@@ +538,2 @@
>    nsAutoString childElt;
> +  GetCellAt(x, y, &row, getter_AddRefs(col), childElt);

Huh -- it looks like in our current code, this function probably leaks! (Because the "GetCellAt(..., &col, ...)" function we're calling here *does addref* the thing that's returned via |col|, and AFAICT nothing ever releases it, because |col| was a raw pointer.)

So it looks like this fixes that leak, probably, because now we'll release when the nsCOMPtr goes out of scope. Great!

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +325,5 @@
>  
>    // An internal hit test.  aX and aY are expected to be in twips in the
>    // coordinate system of this frame.
> +  void GetCellAt(nscoord aX, nscoord aY, int32_t* aRow,
> +                 nsTreeColumn** MOZ_DOES_NOT_ADDREF aCol,

I'm not particularly happy with this function in its current state, since there's another function *with this same name and nearly the same signature* that DOES call AddRef on this param.

We shouldn't have two functions with nearly-identical signatures, one of which addrefs its outparam and one of which does not.

I've noted this on bug 1361096 comment 1, which I'm happy for us to address as a followup.  From a quick skim, I think we fortunately get this right at callsites, for now (aside from the leak that you're probably-fixing in TreeBoxObject.cpp.)

Comment 40

[Nika Layzell [:nika] (ni? for response)]

(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #39)
> ::: layout/base/nsBidiPresUtils.h
> @@ +413,5 @@
> >     * @return  true if it finds that bidi is (or may be) required,
> >     *          false if no potentially-bidi content is present.
> >     */
> >    static bool ChildListMayRequireBidi(nsIFrame*    aFirstChild,
> > +                                      nsIContent** MOZ_NON_OUTPARAM aCurrContent);
> 
> ISSUE: I think this should be MOZ_DOES_NOT_ADDREF, instead of
> MOZ_NON_OUTPARAM.
> 
> You're clearly needing *some* annotation here, to exempt this param from
> analysis -- but "not-an-outparam" seems inaccurate.  aCurrContent really
> *is* really a sort of outparam (it's an in/out param, as the documentation
> above it indicates with "[in/out]"). So, MOZ_NON_OUTPARAM is
> confusing/contradictory.
> 
> The reason it's safe to skip the getter_AddRefs/refcounting dance here is as
> follows: this is a short helper function which simply walks part of the
> frame tree during layout and does not run script or modify the DOM tree, so
> we know that the nsIContent objects involved here will definitely stay alive
> through this function's callers.  So, not-addreffing (and annotating with
> MOZ_DOES_NOT_ADDREF) seems fine & justified.

I use MOZ_NON_OUTPARAM for in/out parameters right now. For example, if you look at part 3 you can see me modifying the xpidl code generator to emit MOZ_NON_OUTPARAM annotations for in/out parameters.

The reason why I have been using MOZ_NON_OUTPARAM in these situations instead of MOZ_DOES_NOT_ADDREF is at least partially because I want to add more meaning to MOZ_DOES_NOT_ADDREF in a folow-up to make it perform more assertions (for example, it will be illegal to pass getter_AddRefs() to a MOZ_DOES_NOT_ADDREF parameter, while it will probably be legal to pass it to MOZ_NON_OUTPARAM which is going to effectively be the "completely opt out of this static analysis" attribute).

I would be open to the suggestion that we should have a different name for the annotation or a new annotation for in/out parameters (although I'm not looking forward to going through all of the patches to make this change), however I think that MOZ_DOES_NOT_ADDREF has a different meaning which I would prefer to not mix with in/out parameters.

I'm NI?-ing you for your feedback on this decision.

> I'm not particularly happy with this function in its current state, since
> there's another function *with this same name and nearly the same signature*
> that DOES call AddRef on this param.
> 
> We shouldn't have two functions with nearly-identical signatures, one of
> which addrefs its outparam and one of which does not.
> 
> I've noted this on bug 1361096 comment 1, which I'm happy for us to address
> as a followup.  From a quick skim, I think we fortunately get this right at
> callsites, for now (aside from the leak that you're probably-fixing in
> TreeBoxObject.cpp.)

I plan to add another analysis in the future which wil make sure we get it right in the other direction too (making sure that we don't pass getter_AddRefs() to a MOZ_DOES_NOT_ADDREF parameter), but that will come later.

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8863030 [details] [diff] [review]
Part 1: Add an analysis to ensure that getter_AddRefs is passed to T**-style outparams

Review of attachment 8863030 [details] [diff] [review]:
-----------------------------------------------------------------

Driveby nit on documentation for the main annotations here:

::: mfbt/Attributes.h
@@ +527,5 @@
> + *   outparam types.
> + * MOZ_NON_OUTPARAM: Applies to paramater declarations. Parameters with this
> + *   annotation will not be treated as outparameters for the purpose of the
> + *   GetterAddrefs analysis.
> + * MOZ_DOES_NOT_ADDREF: Applies to paramater declarations. Parameters with this

Could you make this a little clearer, so that the usage of & distinction between these two annotations (MOZ_NON_OUTPARAM & MOZ_DOES_NOT_ADDREF) is a bit clearer?  It took me a little while to understand these when I was reviewing the layout/ patch, and I think the documentation could be more helpful.

In particular:
 - It should be clearer whether MOZ_NON_OUTPARAM would be appropriate for an in/out parameter. (Is "in/out-param" a type of outparam, or a type of non-outparam?)

 - It would be great if you could provide a brief description (or example) of what constitutes an appropriate usage for each of these annotations.  (Right now the documentation just explains *what they do*, without explaining *how/where to correctly use them*.)

 - It should be clearly stated that callers must NOT use getter_AddRefs() when passing a value for params with these annotations.  (Otherwise, we'd end up with an extra Release() invocation, potentially.)

Comment 42

[Daniel Holbert [:dholbert]]

[oops, midaired you on similar topic]

(In reply to Michael Layzell [:mystor] from comment #40)
> (In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment
> #39)
> > >    static bool ChildListMayRequireBidi(nsIFrame*    aFirstChild,
> > > +                                      nsIContent** MOZ_NON_OUTPARAM aCurrContent);
> > 
> > ISSUE: I think this should be MOZ_DOES_NOT_ADDREF, instead of
> > MOZ_NON_OUTPARAM.
> The reason why I have been using MOZ_NON_OUTPARAM in these situations
> instead of MOZ_DOES_NOT_ADDREF is at least partially because I want to add
> more meaning to MOZ_DOES_NOT_ADDREF in a folow-up to make it perform more
> assertions (for example, it will be illegal to pass getter_AddRefs() to a
> MOZ_DOES_NOT_ADDREF parameter, while it will probably be legal to pass it to
> MOZ_NON_OUTPARAM which is going to effectively be the "completely opt out of
> this static analysis" attribute).

FWIW, in ChildListMayRequireBidi, we don't want callers to a pass getter_AddRefs. So that particular assertion (from a hypothetical MOZ_DOES_NOT_ADDREF annotation) would be fine here.

I'm having a hard time thinking of cases we'd want to assert different things for these annotations. But if you strongly feel that we don't want MOZ_DOES_NOT_ADDREF for in/out params, then I'm OK with MOZ_NON_OUTPARAM as long as it's clearly documented (e.g. maybe saying "it's applicable to variables which are not purely outparams. For example, it would be appropriate to label an in/out-param as MOZ_NON_OUTPARAM, since it's not purely an outparam.")

> I would be open to the suggestion that we should have a different name for
> the annotation or a new annotation for in/out parameters (although I'm not
> looking forward to going through all of the patches to make this change),
> however I think that MOZ_DOES_NOT_ADDREF has a different meaning which I
> would prefer to not mix with in/out parameters.

Per comment 41, part of the problem is that the subtlly different meanings are simply not clear right now.  I don't necessary object to the naming or to the MOZ_NON_OUTPARA usage in this particular case if it's definitely-correct -- I just don't understand the distinction. Better documentation in part 1 would help with that, I think.

Comment 43

[Matthew N. [:MattN]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #35)
> which only doesn't show up because AFAIK we don't have
> automated tests for profile reset.

Just a quick note that we do have tests for reset but I don't know if there is any leak checking running on them since they are marionette: https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/marionette/test_refresh_firefox.py

Comment 44

[Matthew N. [:MattN]]

(In reply to Michael Layzell [:mystor] from comment #36)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #35)
> > Comment on attachment 8862983 [details] [diff] [review]
> > Part 7: Ensure that getter_AddRefs is passed to T**-style outparams in
> > toolkit
> > 
> > I can mark r+ on everything except the nsAppRunner bits with the
> > REVIEWER-NOTE.
> > 
> > I expect that for profile reset, we *intentionally* hold the lock forever,
> > and so the change here to hold it only temporarily is a regression. But I
> > didn't review that originally, and it does mean we're intentionally leaking
> > a C++ object also, which only doesn't show up because AFAIK we don't have
> > automated tests for profile reset. We'd need to talk to MattN about the
> > right way to do that.
> 
> Are we intentionally holding this lock forever in the profile reset code?

Honestly I don't really remember since it was over 5 years ago but I think the intention was to keep the profile locked while we are migrating data to a new profile (that's how a reset works) and moving the old profile to the desktop folder. This avoids a user starting up another instance of Firefox on the old profile between the current lock acquisition and the migration finishing and possibly causing corrupt data to be migrated. The lock should be able to be freed after ProfileResetCleanup[1] though.

[1] https://dxr.mozilla.org/mozilla-central/rev/48c0fd9c9ec5d68061ea7b59358874ae8da72572/toolkit/xre/nsAppRunner.cpp#4353

Comment 45

[Peter Van der Beken [:peterv]]

Comment on attachment 8862981 [details] [diff] [review]
Part 5: Ensure that getter_AddRefs is passed to T**-style outparams in dom

Review of attachment 8862981 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/ContentEventHandler.cpp
@@ +2945,5 @@
>    return NS_OK;
>  }
>  
>  static void AdjustRangeForSelection(nsIContent* aRoot,
> +                                    nsINode** MOZ_NON_OUTPARAM aNode,

I find the MOZ_NON_OUTPARAM a bit confusing here, I wouldn't expect aNode to be an in/out then (same for other in/out args below).

::: dom/media/gmp/GMPService.cpp
@@ +376,4 @@
>            host = &(actor->Host());
>            actor->SetCrashHelper(helper);
>          }
> +        callback->Done(actor.forget().take(), host);

Hmm, this code is pretty messy, but this isn't quite right. The callback's Done function doesn't take a ref, the actor holds a ref to itself. It seems more correct to annotate these GetGMP* getters as not addrefing their out param.

::: dom/xslt/xml/txXMLParser.cpp
@@ +54,4 @@
>          return NS_ERROR_FAILURE;
>      }
>  
> +    // NOTE: We leak a reference here. the node we created above will have its

I don't understand what you mean by "We leak a reference here." What really happens is that the txXPathNativeNodes stored in txLoadedDocumentEntry hold a strong reference to their nsINode. Maybe we should make createXPathNode(nsIDOMDocument* aDocument) call forget() on its |document| variable. And rename that createXPathNode so that its clear that it does that (createStrongXPathNode? bleh).

::: dom/xslt/xslt/txExecutionState.cpp
@@ +192,5 @@
>  
>      // look for an evaluated global variable
> +    mGlobalVariableValues.getVariable(name, getter_AddRefs(result));
> +    if (result) {
> +        if (result == mGlobalVarPlaceholderValue) {

Please don't delete the comment.

@@ +219,3 @@
>              NS_ENSURE_SUCCESS(rv, rv);
>  
> +            rv = mGlobalVariableValues.bindVariable(name, *getter_AddRefs(result));

This seems just wrong, bindVariable takes a |txAExprResult*|.

@@ +285,5 @@
>      popEvalContext();
>  
>      // Remove the placeholder and insert the calculated value
>      mGlobalVariableValues.removeVariable(name);
> +    rv = mGlobalVariableValues.bindVariable(name, *getter_AddRefs(result));

Same here.

Comment 46

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8862992 [details] [diff] [review]
Part 14: Ensure that getter_AddRefs is passed to T**-style outparams in security

Review of attachment 8862992 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just the one question.

::: security/certverifier/OCSPRequestor.cpp
@@ +166,3 @@
>    rv = nsNSSHttpInterface::createFcn(serverSession.get(), "http", path.get(),
>                                       method.get(), originAttributes, timeout,
> +                                     getter_AddRefs(requestSession));

Doesn't a similar change need to happen to the call to nsNSSHttpInterface::createSessionFcn? (Regardless, the real problem with this area of code is that it's leftover from when we needed to contort to NSS' OCSP API and we haven't finished updating it to not be a pain to deal with. That's not something to deal with in this bug, though.)

Comment 47

[Nika Layzell [:nika] (ni? for response)]

I can't really get these landed any time soon. I've pushed my local branch with these changes to github (https://github.com/mystor/mozilla-central/tree/bug1359455).

Andi, is there any chance you could take over driving this to completion?

Comment 48

[Andi [:andi]]

Sure, i think this is a good example for our intern Tristan that's going to begin next week.

Comment 49

[Andi [:andi]]

I think we should limit this checker to only a list of classes, Nika can you please provide a list of such classes that should be black-listed to be passed as T**?

Comment 50

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #49)
> I think we should limit this checker to only a list of classes, Nika can you
> please provide a list of such classes that should be black-listed to be
> passed as T**?

I don't think this makes sense as a blacklist-based checker - we don't really want any XPCOM-style-refcounted classes to be passed without getter_AddRefs to a T** outparameter. We have an xpcom convention that T**-style outparameters produce an already-addrefed value, which this analysis is intended to check.

Fixing this across the entire tree will be hard, but I don't think there's an easy way to create a blacklist here. 

If we want to land this quickly, we could do a fixit pass which inserts MOZ_DOES_NOT_ADDREF or similar in the places where it is needed across the tree, and then audit these locations to make sure that those locations actually don't addref their outparameters.

Comment 51

[Andi [:andi]]

(In reply to Nika Layzell [:mystor] from comment #50)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #49)
> > I think we should limit this checker to only a list of classes, Nika can you
> > please provide a list of such classes that should be black-listed to be
> > passed as T**?
> 
> I don't think this makes sense as a blacklist-based checker - we don't
> really want any XPCOM-style-refcounted classes to be passed without
> getter_AddRefs to a T** outparameter. We have an xpcom convention that
> T**-style outparameters produce an already-addrefed value, which this
> analysis is intended to check.
> 
> Fixing this across the entire tree will be hard, but I don't think there's
> an easy way to create a blacklist here. 
> 
> If we want to land this quickly, we could do a fixit pass which inserts
> MOZ_DOES_NOT_ADDREF or similar in the places where it is needed across the
> tree, and then audit these locations to make sure that those locations
> actually don't addref their outparameters.

I see your point here, thanks for clearing this out. Indeed this makes more sense to check if the object is AddRef compliant, instead of building a black-list.

Comment 52

[Nika Layzell [:nika] (ni? for response)]

It'd be awesome to get this finished up & landed :-)

Comment 53

[Andrew McCreight [:mccr8]]

Any updates here? This would be nice to have?

Comment 54

[Andi [:andi]]

:tjr since this is considered part of private-checkers is anyone from your team willing to take this? What do you think should be the priority of this?

Comment 55

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #54)

:tjr since this is considered part of private-checkers is anyone from your team willing to take this? What do you think should be the priority of this?

Unless the plugin was near-ready, I think it would come into the bucket of "We'd like to add this, if we can find the time, but we're not actively going to work on it right now." We're primarily focusing on the other two right now.