Closed Bug 1169337 Opened 4 years ago Closed 4 years ago

documentation on MOZ_UNSAFE_REF is confusing

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: froydnj, Assigned: Nika)

Details

Attachments

(1 file, 2 obsolete files)

The documentation for MOZ_UNSAFE_REF reads, in part:

"This attribute should be used for non-owning references that can be unsafe..."

but the definition of MOZ_UNSAFE_REF is:

#  define MOZ_OWNING_REF __attribute__((annotate("moz_strong_ref")))
#  define MOZ_NON_OWNING_REF __attribute__((annotate("moz_weak_ref")))
#  define MOZ_UNSAFE_REF(reason) __attribute__((annotate("moz_strong_ref")))

Note how the definition of the macro says that it's a strong, owning reference, unlike what the documentation would have you believe.

Also, it's not clear to me what "unsafe" means in this context, or why I would use it in place of MOZ{_NON,}_OWNING_REF.  Would you please fix the documentation to reflect reality on both of these points?
Flags: needinfo?(ehsan)
Assignee: nobody → michael
I've done some changes to the comments which (I think) clarifies the meaning of
these annotations a bunch.

I've feedback?-ed Ehsan, he can correct me if I have documented these annotations 
incorrectly.
Attachment #8612399 - Flags: feedback?(ehsan)
Comment on attachment 8612399 [details] [diff] [review]
Clarify MOZ_{NON_,}OWNING_REF/MOZ_UNSAFE_REF documentation

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

This looks great to me.  I have suggested some examples below that would hopefully make things even clearer.

But since I'm way too familiar with this stuff, I would like Nathan to look over the patch, so please flag him for review.

::: mfbt/Attributes.h
@@ +418,5 @@
> + * MOZ_OWNING_REF: Applies to declarations of pointers to reference counted
> + *   types.  This attribute tells the compiler that the raw pointer is a strong
> + *   reference, where ownership through methods such as AddRef and Release is
> + *   managed manually.  This can make the compiler ignore these pointers when
> + *   validating the usage of pointers otherwise.

I think it would be useful to add examples here.  Please mention pointers inside unions, and pointers stored in POD types where using nsCOMPtr/nsRefPtr would make the object non-POD.

@@ +427,1 @@
>   *   ignore these pointers when validating the usage of pointers otherwise.

You can add an example here as well, for example an mOwner member which is set to null by the Owner class's destructor where the containing class null-checks mOwner to ensure that it is pointing to a live object before accessing it.

@@ +429,5 @@
> + *   Occasionally there are non-owning references which are valid, but do not take
> + *   the form of a MOZ_NON_OWNING_REF.  Their safety may be dependent on the behaviour
> + *   of API consumers.  The string argument passed to this macro documents the safety
> + *   conditions.  This can make the compiler ignore these pointers when validating
> + *   the usage of pointers elsewhere.

Please also add an example, for example an nsIAtom* member that points to a static atom, which is known to be valid throughout the lifetime of the program because of the static atom semanics.

It may also be a good idea to mention that the use of this annotation is discouraged, where a strong reference, or one of the above two can be used instead.

@@ +452,5 @@
>  #  define MOZ_IMPLICIT __attribute__((annotate("moz_implicit")))
>  #  define MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT __attribute__((annotate("moz_no_arith_expr_in_arg")))
>  #  define MOZ_OWNING_REF __attribute__((annotate("moz_strong_ref")))
>  #  define MOZ_NON_OWNING_REF __attribute__((annotate("moz_weak_ref")))
> +#  define MOZ_UNSAFE_REF(reason) __attribute__((annotate("moz_weak_ref")))

If if helps make things clearer, we can change all of these to "moz_ignore_ref" and look for that attribute name in the analyzer.
Attachment #8612399 - Flags: feedback?(ehsan) → feedback+
My comment above should answer comment 0.
Flags: needinfo?(ehsan)
Added examples for each of the varieties of annotations.

I opted to use "moz_weak_ref" for MOZ_UNSAFE_REF, as an unsafe reference is, by definition, not a "strong" reference. (as for some reason, we are not AddRef/Release-ing the value ourselves). I'd also be OK with "moz_ignore_ref", but I think that changing the annotation to "moz_weak_ref" is acceptable for now.
Attachment #8612399 - Attachment is obsolete: true
Attachment #8613528 - Flags: review?(nfroyd)
Comment on attachment 8613528 [details] [diff] [review]
Clarify MOZ_{NON_,}OWNING_REF/MOZ_UNSAFE_REF documentation

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

::: mfbt/Attributes.h
@@ +415,5 @@
>   *   conversions.
>   * MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT: Applies to functions. Makes it a compile
>   *   time error to pass arithmetic expressions on variables to the function.
> + * MOZ_OWNING_REF: Applies to declarations of pointers to reference counted
> + *   types.  This attribute tells the compiler that the raw pointer is a strong

Does the analysis pass actually enforce the reference-counted type requirement?  (I'm not gating the patch on that, I'm just curious.)

@@ +419,5 @@
> + *   types.  This attribute tells the compiler that the raw pointer is a strong
> + *   reference, where ownership through methods such as AddRef and Release is
> + *   managed manually.  This can make the compiler ignore these pointers when
> + *   validating the usage of pointers otherwise.
> + *   Example uses include owned pointers inside of unions, and pointers stored

Let's add an empty comment line prior to this, to visually separate the paragraphs.

@@ +420,5 @@
> + *   reference, where ownership through methods such as AddRef and Release is
> + *   managed manually.  This can make the compiler ignore these pointers when
> + *   validating the usage of pointers otherwise.
> + *   Example uses include owned pointers inside of unions, and pointers stored
> + *   in POD types where using nsCOMPtr/nsRefPtr would make the object non-POD.

Let's say "...where using a smart pointer class would make..." since these classes don't exist in MFBT.

@@ +429,2 @@
>   *   ignore these pointers when validating the usage of pointers otherwise.
> + *   Examples include an mOwner pointer, which is nulled by the owning class's

Empty comment line prior to this.

@@ +434,5 @@
> + *   the form of a MOZ_NON_OWNING_REF.  Their safety may be dependent on the behaviour
> + *   of API consumers.  The string argument passed to this macro documents the safety
> + *   conditions.  This can make the compiler ignore these pointers when validating
> + *   the usage of pointers elsewhere.
> + *   Examples include an nsIAtom* member which is known at compile time to point to a

Empty comment line prior to this.  nsIAtom isn't part of MFBT, but I don't have a good alternative for this. ;)

Let's also add an example with the LazyIdleThread style of usage that you recently fixed up; I think that would make it more obvious how the API consumers issue above plays into things.  Maybe something like:

class Example
{
   ...
public:
   // aAction is owned by the caller.  If aAction is going to be freed or otherwise
   // become invalid, the caller is responsible for calling SetAction(nullptr).
   void SetAction(Action* aAction);

private:
   // Uses of mAction are not null-checked.
   Action* MOZ_UNSAFE_REF("See documentation for SetAction") mAction;
};

@@ +436,5 @@
> + *   conditions.  This can make the compiler ignore these pointers when validating
> + *   the usage of pointers elsewhere.
> + *   Examples include an nsIAtom* member which is known at compile time to point to a
> + *   static atom which is valid throughout the lifetime of the program.
> + *   Use of this annotation is discouraged when a strong reference or one of the above

Empty comment line prior to this.
Attachment #8613528 - Flags: review?(nfroyd) → review+
I've attached an updated patch which adds your formatting changes, and elaborates with
an additional example on MOZ_UNSAFE_REF.
Attachment #8613528 - Attachment is obsolete: true
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> 
> Does the analysis pass actually enforce the reference-counted type
> requirement?  (I'm not gating the patch on that, I'm just curious.)
> 

I don't think that the analysis actually enforces the reference-counted type requirement. Ehsan would know better though. 

Enforcing that requirement seems like it would be a good idea.
Flags: needinfo?(ehsan)
(In reply to Michael Layzell [:mystor] from comment #7)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> > 
> > Does the analysis pass actually enforce the reference-counted type
> > requirement?  (I'm not gating the patch on that, I'm just curious.)
> > 
> 
> I don't think that the analysis actually enforces the reference-counted type
> requirement. Ehsan would know better though. 

It doesn't, in the sense that it won't emit an error if you use this attribute with non-reference counted types.

> Enforcing that requirement seems like it would be a good idea.

I agree, but since the analysis is not even close to landing yet, please file a follow-up.  Thanks!
Flags: needinfo?(ehsan)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/229b03af6f2b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.