Convert NS_GetCurrentThread uses in dom (except for dom/media)

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1365096#c0.

This bug covers all uses in dom/ except for dom/media.
Posted patch patchSplinter Review
This conversion makes us operate on event targets instead of threads. The advantage of event targets is that there is only one main thread event target in the Quantum DOM world, while there are potentially many main threads.

I'll send out an email about this change to dev-platform tomorrow. However, I want to get some patches posted so people can see what the change will look like.
Attachment #8868326 - Flags: review?(bugs)
Also, bug 1361164 has some discussion about the API.
Well, there sure are several nsIEventTargets in main thread, like the one we use for window timers.
But ok.
Comment on attachment 8868326 [details] [diff] [review]
patch

>   IsOnOwningThread() const
>   {
>-    MOZ_ASSERT(mOwningThread);
>+    MOZ_ASSERT(mOwningEventTarget);
> 
>     bool current;
>-    return NS_SUCCEEDED(mOwningThread->IsOnCurrentThread(&current)) && current;
>+    return NS_SUCCEEDED(mOwningEventTarget->IsOnCurrentThread(&current)) && current;
This kind of code looks weird. Do we really want event target to have IsOnCurrentThread? I don't even understand what it means
conceptually. (I do understand what it probably does :) )

>   already_AddRefed<gfx::DataSourceSurface> GetDataSurfaceSafe() {
>-    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
>-    MOZ_ASSERT(mainThread);
>-    SyncRunnable::DispatchToThread(mainThread, this, false);
>+    RefPtr<ThreadEventTarget> mainTarget = GetMainThreadEventTarget();
>+    MOZ_ASSERT(mainTarget);
>+    SyncRunnable::DispatchToThread(mainTarget, this, false);
SyncRunnable::DispatchToThread(mainTarget doesn't make sense.
Either we dispatch to Thread or we dispatch to EventTarget

> StructuredCloneHolder::Write(JSContext* aCx,
>                              JS::Handle<JS::Value> aValue,
>                              JS::Handle<JS::Value> aTransfer,
>                              JS::CloneDataPolicy cloneDataPolicy,
>                              ErrorResult& aRv)
> {
>   MOZ_ASSERT_IF(mStructuredCloneScope == StructuredCloneScope::SameProcessSameThread,
>-                mCreationThread == NS_GetCurrentThread());
>+                mCreationEventTarget->IsOnCurrentThread());
Also here, IsOnCurrentThread. It is really odd. What does it mean nsIEventTarget to be on current thread.
Could IsOnCurrentThread be renamed to IsOnOwningThread or some such.

> StructuredCloneHolder::Read(nsISupports* aParent,
>                             JSContext* aCx,
>                             JS::MutableHandle<JS::Value> aValue,
>                             ErrorResult& aRv)
> {
>   MOZ_ASSERT_IF(mStructuredCloneScope == StructuredCloneScope::SameProcessSameThread,
>-                mCreationThread == NS_GetCurrentThread());
>+                mCreationEventTarget->IsOnCurrentThread());
ditto

StructuredCloneHolder::ReadFromBuffer(nsISupports* aParent,
>                                       JSContext* aCx,
>                                       JSStructuredCloneData& aBuffer,
>                                       uint32_t aAlgorithmVersion,
>                                       JS::MutableHandle<JS::Value> aValue,
>                                       ErrorResult& aRv)
> {
>   MOZ_ASSERT_IF(mStructuredCloneScope == StructuredCloneScope::SameProcessSameThread,
>-                mCreationThread == NS_GetCurrentThread());
>+                mCreationEventTarget->IsOnCurrentThread());
and here

>   virtual mozIStorageConnection*
>   GetConnection() const override
>   {
>-    MOZ_ASSERT(mTarget == NS_GetCurrentThread());
>+    MOZ_ASSERT(mTarget->IsOnCurrentThread());
here


>   SetConnection(mozIStorageConnection* aConn) override
>   {
>-    MOZ_ASSERT(mTarget == NS_GetCurrentThread());
>+    MOZ_ASSERT(mTarget->IsOnCurrentThread());
and here


and elsewhere.

I think nsIEventTarget::IsOnCurrentThread is conceptually wrong enough that I'd like to see a new patch.
Attachment #8868326 - Flags: review?(bugs) → review-
Comment on attachment 8868326 [details] [diff] [review]
patch

As we discussed on IRC, IsOnCurrentThread is admittedly not a great name, but it's been a method on nsIEventTarget for a long time. I'd rather not change it.
Attachment #8868326 - Flags: review- → review?(bugs)
Comment on attachment 8868326 [details] [diff] [review]
patch

>   already_AddRefed<gfx::DataSourceSurface> GetDataSurfaceSafe() {
>-    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
>-    MOZ_ASSERT(mainThread);
>-    SyncRunnable::DispatchToThread(mainThread, this, false);
>+    RefPtr<ThreadEventTarget> mainTarget = GetMainThreadEventTarget();
>+    MOZ_ASSERT(mainTarget);
>+    SyncRunnable::DispatchToThread(mainTarget, this, false);
So DispatchToThread needs to be renamed but apparently that is old code, so not in this bug.

> StructuredCloneHolder::Write(JSContext* aCx,
>                              JS::Handle<JS::Value> aValue,
>                              JS::Handle<JS::Value> aTransfer,
>                              JS::CloneDataPolicy cloneDataPolicy,
>                              ErrorResult& aRv)
> {
>   MOZ_ASSERT_IF(mStructuredCloneScope == StructuredCloneScope::SameProcessSameThread,
>-                mCreationThread == NS_GetCurrentThread());
>+                mCreationEventTarget->IsOnCurrentThread());
And IsOnCurrentThread() should be renamed too, in some other bug, or something



> UDPSocketParent::RecvConnect(const UDPAddressInfo& aAddressInfo)
> {
>-  nsCOMPtr<nsIEventTarget> thread(NS_GetCurrentThread());
>+  nsCOMPtr<nsIEventTarget> target;
>+  target = static_cast<nsIEventTarget*>(GetCurrentThreadEventTarget());
Why you need static_cast.
And why nsCOMPtr<nsIEventTarget> target; why not
nsCOMPtr<nsIEventTarget> target = ...
Attachment #8868326 - Flags: review?(bugs) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/351a4d47496e
Convert NS_GetCurrentThread uses in dom (except for dom/media) (r=smaug)
https://hg.mozilla.org/mozilla-central/rev/351a4d47496e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.