Closed
Bug 1114987
Opened 8 years ago
Closed 8 years ago
Annotate GlobalObject::mGlobalObject as an owning ref
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
Details
Attachments
(1 file, 1 obsolete file)
4.12 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8540710 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ehsan.akhgari
![]() |
||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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-
Assignee | ||
Comment 4•8 years ago
|
||
(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•8 years ago
|
||
STRONG? More like RAW or some such, no?
Assignee | ||
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
OWNING_REF sounds like someone has done extra Addref (and will do Release)
Flags: needinfo?(bugs)
Assignee | ||
Comment 8•8 years ago
|
||
(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!
Assignee | ||
Updated•8 years ago
|
Summary: Convert GlobalObject::mGlobalObject to nsCOMPtr → Annotate GlobalObject::mGlobalObject as an owning ref
Assignee | ||
Comment 9•8 years ago
|
||
Actually, how about adding a third annotation called MOZ_LIVE_GUARANTEED_REF?
Flags: needinfo?(bugs)
Comment 10•8 years ago
|
||
How is that different from just a weak reference? Or, maybe if you want to be fancy, a const weak reference.
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
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•8 years ago
|
||
MOZ_UNSAFE_PTR might be more correct even.
Assignee | ||
Comment 14•8 years ago
|
||
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.)
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8544175 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8540710 -
Attachment is obsolete: true
Comment 16•8 years ago
|
||
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+
Assignee | ||
Comment 17•8 years ago
|
||
FWIW I will clean up the name of the attribute later...
Comment 18•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/67b3ac5b553a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•