Annotate GlobalObject::mGlobalObject as an owning ref

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Attachment #8540710 - Flags: review?(bugs)
Assignee: nobody → ehsan.akhgari
Blocks: 1114683
Comment on attachment 8540710 [details] [diff] [review]
Convert GlobalObject::mGlobalObject to nsCOMPtr

Wouldn't it be cleaner to getter_AddRefs and make GetISupportsFromJSObject follow XPCOM conventions?
Comment on attachment 8540710 [details] [diff] [review]
Convert GlobalObject::mGlobalObject to nsCOMPtr

May I ask why this change?

Given that GlobalObject is being using in various graphics related
Web APIs, this would slow them down a bit.
Attachment #8540710 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8540710 [details] [diff] [review]
> Convert GlobalObject::mGlobalObject to nsCOMPtr
> 
> May I ask why this change?

Please see bug 1114683.

> Given that GlobalObject is being using in various graphics related
> Web APIs, this would slow them down a bit.

Should we annotate mGlobalObject with MOZ_STRONG_REF instead?
STRONG? More like RAW or some such, no?
I renamed MOZ_STRONG_REF to MOZ_OWNING_REF a while ago.

The point of these annotations is to document whether the object's lifetime is guaranteed to extend over that of the pointer (i.e. the pointer always points to valid memory.)  Given that GlobalObject is MOZ_STACK_CLASS and mGlobalObject points to the global, I think that is guaranteed here.  Does that make sense to you?
Flags: needinfo?(bugs)
OWNING_REF sounds like someone has done extra Addref (and will do Release)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #7)
> OWNING_REF sounds like someone has done extra Addref (and will do Release)

That's not necessarily the intention.  I can't really think of a better name, if you can, I'd be happy to rename en masse!
Summary: Convert GlobalObject::mGlobalObject to nsCOMPtr → Annotate GlobalObject::mGlobalObject as an owning ref
Actually, how about adding a third annotation called MOZ_LIVE_GUARANTEED_REF?
Flags: needinfo?(bugs)
How is that different from just a weak reference?  Or, maybe if you want to be fancy, a const weak reference.
MOZ_LIVE_GUARANTEED_REF sounds ok to me. It is a raw ref, but explicitly says with the annotation
that it is guaranteed (by code inspection) to be ok.

MOZ_UNSAFE_REF might work too. That should hint the reviewer to check its usage more carefully.
Flags: needinfo?(bugs)
I think I'd prefer MOZ_UNSAFE_REF or some such.

Could we even do something like
MOZ_UNSAFE_REF("explanation why it is safe after all")?
Raw pointers have caused us enough harm that making them more annoying to use might be ok.
MOZ_UNSAFE_PTR might be more correct even.
I'll go with MOZ_UNSAFE_REF since down the line we may want o use the same for C++ references as well (although they are not as common.)
Attachment #8540710 - Attachment is obsolete: true
Comment on attachment 8544175 [details] [diff] [review]
Add MOZ_UNSAFE_REF and mark GlobalObject::mGlobalObject as such

>   JS::Rooted<JSObject*> mGlobalJSObject;
>   JSContext* mCx;
>-  mutable nsISupports* mGlobalObject;
>+  mutable nsISupports* MOZ_UNSAFE_REF("Valid because GlobalObject is a stack "
>+                                      "class, and mGlobalObject points to the "
>+                                      "global, so it won't be destroyed as long "
>+                                      "as GlobalObject lives on the stack") mGlobalObject;
A bit long explanation ;) but fine.


>@@ -515,16 +515,20 @@
>  * MOZ_OWNING_REF: Applies to declarations of pointer types.  This attribute
>  *   tells the compiler that the raw pointer is a strong reference, and that
>  *   property is somehow enforced by the code.  This can make the compiler
>  *   ignore these pointers when validating the usage of pointers otherwise.
>  * MOZ_NON_OWNING_REF: Applies to declarations of pointer types.  This attribute
>  *   tells the compiler that the raw pointer is a weak reference, and that
>  *   property is somehow enforced by the code.  This can make the compiler
>  *   ignore these pointers when validating the usage of pointers otherwise.
>+ * MOZ_UNSAFE_REF: Applies to declarations of pointer types.  This attribute
>+ *   should be used for non-owning references that can be unsafe, and their
>+ *   safety needs to be validated through code inspection.  The string argument
>+ *   passed to this macro documents the safety conditions.
I wonder what is the difference of MOZ_NON_OWNING_REF and MOZ_UNSAFE_REF


>+#  define MOZ_UNSAFE_REF(reason) __attribute__((annotate("moz_strong_ref")))
"moz_raw_ref" or some such?
Attachment #8544175 - Flags: review?(bugs) → review+
FWIW I will clean up the name of the attribute later...
https://hg.mozilla.org/mozilla-central/rev/67b3ac5b553a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.