1114987 - Annotate GlobalObject::mGlobalObject as an owning ref

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[:ehsan akhgari]

Attachment: Convert GlobalObject::mGlobalObject to nsCOMPtr

Comment 2

[Nathan Froyd [:froydnj]]

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 3

[Olli Pettay [:smaug]]

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.

Comment 4

[:ehsan akhgari]

(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?

Comment 5

[Olli Pettay [:smaug]]

STRONG? More like RAW or some such, no?

Comment 6

[:ehsan akhgari]

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?

Comment 7

[Olli Pettay [:smaug]]

OWNING_REF sounds like someone has done extra Addref (and will do Release)

Comment 8

[:ehsan akhgari]

(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!

Comment 9

[:ehsan akhgari]

Actually, how about adding a third annotation called MOZ_LIVE_GUARANTEED_REF?

Comment 10

[Andrew McCreight [:mccr8]]

How is that different from just a weak reference?  Or, maybe if you want to be fancy, a const weak reference.

Comment 11

[Olli Pettay [:smaug]]

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.

Comment 12

[Olli Pettay [:smaug]]

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.

Comment 13

[Olli Pettay [:smaug]]

MOZ_UNSAFE_PTR might be more correct even.

Comment 14

[:ehsan akhgari]

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.)

Comment 15

[:ehsan akhgari]

Attachment: Add MOZ_UNSAFE_REF and mark GlobalObject::mGlobalObject as such

Comment 16

[Olli Pettay [:smaug]]

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?

Comment 17

[:ehsan akhgari]

FWIW I will clean up the name of the attribute later...

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67b3ac5b553a