Closed Bug 1146418 Opened 5 years ago Closed 4 years ago

Promise API entry points should use NS_ASSERT_OWNINGTHREAD.

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bobowen, Assigned: shawnjohnjr, Mentored)

Details

(Whiteboard: [tw-dom])

Attachments

(1 file, 1 obsolete file)

From bug 1087246 comment 29:
"a Promise is a single-threaded object, and you're only allowed to touch it from the thread it lives on.  We should probably add some NS_ASSERT_OWNINGTHREAD to API entrypoints to make that clearer to people."
Assignee: nobody → shuang
Is there much point in doing this while we're going to remove DOM Promises any moment now?
The C++ API still exists, basically unchanged, so why would that matter?
Flags: needinfo?(Ms2ger)
If so, I guess it doesn't.
Flags: needinfo?(Ms2ger)
Comment on attachment 8738522 [details] [diff] [review]
Bug 1146418 - Promise API entry points should use NS_ASSERT_OWNINGTHREAD

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

These assertions only need to go on the public functions.  A bunch of these are protected or private.  Also I think you missed Promise::All

::: dom/promise/Promise.cpp
@@ +497,5 @@
>  
>  Promise::~Promise()
>  {
> +  NS_ASSERT_OWNINGTHREAD(Promise);
> +

We don't need this in the destructor, the reference counting assertions will handle it.

@@ +825,5 @@
>  
>  void
>  Promise::HandleException(JSContext* aCx)
>  {
> +  NS_ASSERT_OWNINGTHREAD(Promise);

This is private.

@@ +872,5 @@
>  void
>  Promise::CreateWrapper(JS::Handle<JSObject*> aDesiredProto, ErrorResult& aRv)
>  {
> +  NS_ASSERT_OWNINGTHREAD(Promise);
> +

This is protected.

@@ +1194,5 @@
>  void
>  Promise::CallInitFunction(const GlobalObject& aGlobal,
>                            PromiseInit& aInit, ErrorResult& aRv)
>  {
> +  NS_ASSERT_OWNINGTHREAD(Promise);

This is protected.

@@ +2469,5 @@
>  Promise::AppendCallbacks(PromiseCallback* aResolveCallback,
>                           PromiseCallback* aRejectCallback)
>  {
> +  NS_ASSERT_OWNINGTHREAD(Promise);
> +

This is private
Attachment #8738522 - Flags: review?(khuey) → review-
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> Comment on attachment 8738522 [details] [diff] [review]
> Bug 1146418 - Promise API entry points should use NS_ASSERT_OWNINGTHREAD
> 
> Review of attachment 8738522 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These assertions only need to go on the public functions.  A bunch of these
> are protected or private.  Also I think you missed Promise::All
> 
Thanks for pointing out all the errors.
But it looks like |Promise::All| is static function[1]. Can we use NS_ASSERT_OWNINGTHREAD in static member functions?

[1]:https://dxr.mozilla.org/mozilla-central/source/dom/promise/Promise.h?from=dom%2Fpromise%2FPromise.h#200
[2]https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#119
Flags: needinfo?(khuey)
Ah yes, you're right of course.  We can't annotate the static entry points.  That's what I get for skimming :P
Flags: needinfo?(khuey)
Attachment #8738882 - Flags: review?(khuey) → review?(amarchesini)
Attachment #8738882 - Flags: review?(amarchesini) → review+
mozilla-inbound is CLOSED.
https://hg.mozilla.org/mozilla-central/rev/e96a7a1e83c8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.