1343760 - Label runnables in dom/events

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P2)

Description

[Ming-Chou Shih [:stone]]

Label runnables in the following classes
AsyncEventDispatcher
IMEContentObserver
EventListenerService

Comment 1

[Ming-Chou Shih [:stone]]

DataTransferItem

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Ming-Chou Shih [:stone] from comment #0)
> Label runnables in the following classes
> AsyncEventDispatcher
I think AsyncEventDispatcher has been labeled by :billm in bug 1318506 if you're talking about AsyncEventDispatcher::PostDOMEvent().
> IMEContentObserver
> EventListenerService

Comment 3

[Ming-Chou Shih [:stone]]

Attachment: Label runnables in dom/events

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8849879 [details] [diff] [review]
Label runnables in dom/events

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

Some use cases could be simplified.
Please see my comments below.

Thanks!

::: dom/events/EventListenerService.cpp
@@ +365,5 @@
>    }
>  
>    if (!mPendingListenerChanges) {
>      mPendingListenerChanges = nsArrayBase::Create();
> +    nsCOMPtr<nsIRunnable> runnable =

The name can be given by the other overrided NewRunnableMethod(char * aName, ...).

So we can modify these as followed:
    nsCOMPtr<nsIRunnable> runnable =
      NewRunnableMethod("EventListenerService::NotifyAboutMainThreadListenerChangeInternal",
        this, &EventListenerService::NotifyPendingChanges);
    if (nsCOMPtr<nsIGlobalObject> global = aTarget->GetOwnerGlobal()) {
      global->Dispatch(nullptr, TaskCategory::Other, runnable.forget());
    } else if (nsCOMPtr<nsINode> node = do_QueryInterface(aTarget)) {
      node->OwnerDoc()->Dispatch(nullptr,
                                 TaskCategory::Other, runnable.forget());
    } else {
      NS_DispatchToMainThread(runnable);
    }

::: dom/events/IMEContentObserver.cpp
@@ +1415,5 @@
>    // it may kick runnable event immediately after DOM tree is changed but
>    // the selection range isn't modified yet.
>    mQueuedSender = new IMENotificationSender(this);
> +  MOZ_ASSERT(mDocShell);
> +  if (nsCOMPtr<nsPIDOMWindowOuter> win = mDocShell->GetWindow()) {

we can simplify these as followed:
  if (nsCOMPtr<nsIScriptGlobalObject> globalObject = mDocShell->GetGetScriptGlobalObject()) {
    RefPtr<IMENotificationSender> queuedSender = mQueuedSender;
    globalObject->Dispatch("IMEContentObserver::FlushMergeableNotifications", 
                           TaskCategory::Other, queuedSender.forget());
  } else {
     NS_DispatchToCurrentThread(mQueuedSender);
  }

@@ +1548,5 @@
>        mIMEContentObserver->mQueuedSender =
>          new IMENotificationSender(mIMEContentObserver);
> +      MOZ_ASSERT(mIMEContentObserver->mDocShell);
> +      if (nsCOMPtr<nsPIDOMWindowOuter> win =
> +            mIMEContentObserver->mDocShell->GetWindow()) {

ditto

@@ +1620,5 @@
>           "posting IMENotificationSender to current thread", this));
>        mIMEContentObserver->mQueuedSender =
>          new IMENotificationSender(mIMEContentObserver);
> +      MOZ_ASSERT(mIMEContentObserver->mDocShell);
> +      if (nsCOMPtr<nsPIDOMWindowOuter> win =

ditto

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Label runnables in dom/events (v3)

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8850259 [details] [diff] [review]
Label runnables in dom/events (v3)

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

f=me, after the following comment are addressed.

::: dom/events/EventListenerService.cpp
@@ +366,5 @@
>  
>    if (!mPendingListenerChanges) {
>      mPendingListenerChanges = nsArrayBase::Create();
> +    nsCOMPtr<nsIRunnable> runnable =
> +      NewRunnableMethod("EventListenerService::NotifyAboutMainThreadListenerChangeInternal",

Please follow the naming convention in 
https://wiki.mozilla.org/Quantum/DOM#Runnable_naming_convention
NewRunnableMethod("EventListenerService::NotifyPendingChanges", ...);

::: dom/events/IMEContentObserver.cpp
@@ +1417,5 @@
> +  MOZ_ASSERT(mDocShell);
> +  if (nsCOMPtr<nsIScriptGlobalObject> globalObject =
> +        mDocShell->GetScriptGlobalObject()) {
> +    RefPtr<IMENotificationSender> queuedSender = mQueuedSender;
> +    globalObject->Dispatch("IMEContentObserver::FlushMergeableNotifications",

Please following the convention of runnable name in 
https://wiki.mozilla.org/Quantum/DOM#Runnable_naming_convention
The runnable's name in the case is "IMENotificationSender".

@@ +1551,5 @@
> +      if (nsCOMPtr<nsIScriptGlobalObject> globalObject =
> +            mIMEContentObserver->mDocShell->GetScriptGlobalObject()) {
> +        RefPtr<IMENotificationSender> queuedSender =
> +          mIMEContentObserver->mQueuedSender;
> +        globalObject->Dispatch("IMENotificationSender::Run",

ditto

@@ +1624,5 @@
> +      if (nsCOMPtr<nsIScriptGlobalObject> globalObject =
> +            mIMEContentObserver->mDocShell->GetScriptGlobalObject()) {
> +        RefPtr<IMENotificationSender> queuedSender =
> +          mIMEContentObserver->mQueuedSender;
> +        globalObject->Dispatch("IMENotificationSender::Run",

ditto

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Label runnables in dom/events (v4)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8850436 [details] [diff] [review]
Label runnables in dom/events (v4)


>+  nsCOMPtr<nsINode> parentNode =
>+    do_QueryInterface(mDataTransfer->GetParentObject());
>+  MOZ_ASSERT(parentNode);
>   RefPtr<GASRunnable> runnable = new GASRunnable(aCallback, stringData);
>-  rv = NS_DispatchToMainThread(runnable);
>+  if (nsIDocument* doc = parentNode->OwnerDoc()) {
>+    rv = doc->Dispatch("GASRunnable", TaskCategory::Other,
>+                       runnable.forget());
>+  } else {
>+    rv = NS_DispatchToMainThread(runnable);
>+  }

Huh, the GASRunnable code is really weirdly commented. Comments hint that there is code running on non-main-thread, but that isn't the case, as far as I see
So, looks ok to me

>   // If contents in selection range is modified, the selection range still
>   // has removed node from the tree.  In such case, nsContentIterator won't
>   // work well.  Therefore, we shouldn't use AddScriptRunnder() here since
>   // it may kick runnable event immediately after DOM tree is changed but
>   // the selection range isn't modified yet.
>   mQueuedSender = new IMENotificationSender(this);
>-  NS_DispatchToCurrentThread(mQueuedSender);
>-
>+  MOZ_ASSERT(mDocShell);
>+  if (nsCOMPtr<nsIScriptGlobalObject> globalObject =
>+        mDocShell->GetScriptGlobalObject()) {
>+    RefPtr<IMENotificationSender> queuedSender = mQueuedSender;
>+    globalObject->Dispatch("IMENotificationSender",
>+                           TaskCategory::Other, queuedSender.forget());
Could you use nsIScriptGlobalObject* here, not nsCOMPtr. nsCOMPtr does "slow" AddRef/Release and I don't see reason to explicitly have strong
variable here.


>@@ -1535,17 +1542,26 @@ IMEContentObserver::IMENotificationSende
>     // destroyed, mNeedsToNotifyIMEOfFocusSet is never set true again.
>     if (mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet) {
>       MOZ_ASSERT(!mIMEContentObserver->mIMEHasFocus);
>       MOZ_LOG(sIMECOLog, LogLevel::Debug,
>         ("0x%p IMEContentObserver::IMENotificationSender::Run(), "
>          "posting IMENotificationSender to current thread", this));
>       mIMEContentObserver->mQueuedSender =
>         new IMENotificationSender(mIMEContentObserver);
>-      NS_DispatchToCurrentThread(mIMEContentObserver->mQueuedSender);
>+      MOZ_ASSERT(mIMEContentObserver->mDocShell);
Useless assertion. We crash anyhow if mDocShell is null.
Just add a null check inside the 'if'

>+      if (nsCOMPtr<nsIScriptGlobalObject> globalObject =
>+            mIMEContentObserver->mDocShell->GetScriptGlobalObject()) {
>+        RefPtr<IMENotificationSender> queuedSender =
>+          mIMEContentObserver->mQueuedSender;
>+        globalObject->Dispatch("IMENotificationSender",
>+                               TaskCategory::Other, queuedSender.forget());
>+      } else {
>+        NS_DispatchToCurrentThread(mIMEContentObserver->mQueuedSender);
>+      }
Same here, no need for nsCOMPtr for the global object


>       MOZ_LOG(sIMECOLog, LogLevel::Debug,
>         ("0x%p IMEContentObserver::IMENotificationSender::Run(), "
>          "posting IMENotificationSender to current thread", this));
>       mIMEContentObserver->mQueuedSender =
>         new IMENotificationSender(mIMEContentObserver);
>-      NS_DispatchToCurrentThread(mIMEContentObserver->mQueuedSender);
>+      MOZ_ASSERT(mIMEContentObserver->mDocShell);
again, useless assertion. Just add check inside 'if' (or prove that mDocShell isn't ever null).

>+      if (nsCOMPtr<nsIScriptGlobalObject> globalObject =
>+            mIMEContentObserver->mDocShell->GetScriptGlobalObject()) {
No need for nsCOMPtr


Those fixed, r+

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Label runnables in dom/events (v5)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e061bb501b40ca58dc7e88a9e73ea2052335a6dd

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce25d866bd65
Label runnables in dom/events. f=bevistseng. r=smaug.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce25d866bd65

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out at smaug's request for causing bug 1351328.

https://hg.mozilla.org/mozilla-central/rev/0e0eb96528a1

Comment 13

[Ming-Chou Shih [:stone]]

Attachment: Label runnables in dom/events (v6)

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8854654 [details] [diff] [review]
Label runnables in dom/events (v6)

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

f=me after the comments below are addressed, thanks!

::: dom/events/EventListenerService.cpp
@@ +361,2 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());

The aTarget will never be nullptr according to its call sites.
Please add assertion to aTarget here if nullptr check on aTarget is not necessary:
  MOZ_ASSERT(aTarget);

::: dom/events/IMEContentObserver.cpp
@@ +1418,5 @@
> +                                        mDocShell->GetScriptGlobalObject() :
> +                                        nullptr;
> +  if (globalObject) {
> +    RefPtr<IMENotificationSender> queuedSender = mQueuedSender;
> +    globalObject->Dispatch("IMENotificationSender",

The IMENotificationSender was named in its constructor.
We can just pass nullptr here without naming it again in Dispatcher:
http://searchfox.org/mozilla-central/source/xpcom/threads/Dispatcher.cpp#124-128

@@ +1553,5 @@
> +        mIMEContentObserver->mDocShell->GetScriptGlobalObject() : nullptr;
> +      if (globalObject) {
> +        RefPtr<IMENotificationSender> queuedSender =
> +          mIMEContentObserver->mQueuedSender;
> +        globalObject->Dispatch("IMENotificationSender",

ditto

@@ +1627,5 @@
> +        mIMEContentObserver->mDocShell->GetScriptGlobalObject() : nullptr;
> +      if (globalObject) {
> +        RefPtr<IMENotificationSender> queuedSender =
> +          mIMEContentObserver->mQueuedSender;
> +        globalObject->Dispatch("IMENotificationSender",

ditto

Comment 15

[Ming-Chou Shih [:stone]]

Attachment: Label runnables in dom/events (v7)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8856436 [details] [diff] [review]
Label runnables in dom/events (v7)

yeah, DataTransfer is rather annoying.

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Label runnables in dom/events (v8)

Updated the patch summary.

The tried results are in https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbfc7aab13b4f4ac6e78657f55b776fc030165b

Looks like there are some infra problems on Windows 10 x64 asan

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52e34fa7a0b
Label runnables in dom/events. f=bevistseng, r=smaug

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f52e34fa7a0b