1350787 - Label runnables in dom/xhr

Status: RESOLVED FIXED

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

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This bug is to track and review how to apply labeling APIs (https://wiki.mozilla.org/Quantum/DOM) to dom/xhr.

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Sorry for a late response. I'm now switching to this bug.

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1350787 - DocGroup Labeling runnables in dom/xhr

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1350787 - DocGroup labeling runnables in dom/xhr

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8864443 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr

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

Hi Bevis,
Do you mind reviewing this labeling patch?
I search based on 'dispatch' keyword, since bug 1318506 already fixed Timer callback part.

Comment 5

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

Comment on attachment 8864443 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr

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

I'd like to revisit again after the comments are addressed.

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2834,5 @@
>      mSuspendedDoc = nullptr;
>    }
>  
>    if (mResumeTimeoutRunnable) {
> +    DispatchRunnable(GetOwnerGlobal(), mResumeTimeoutRunnable.forget());

Add MOZ_ASSERT(NS_IsMainThread()); at the beginning of XMLHttpRequestMainThread::UnsuppressEventHandlingAndResume().

@@ +2841,5 @@
>  }
>  
>  nsresult
>  XMLHttpRequestMainThread::SendInternal(const BodyExtractorBase* aBody)
>  {

MOZ_ASSERT(NS_IsMainThread());

@@ +3130,5 @@
> +nsresult
> +XMLHttpRequestMainThread::DispatchRunnable(nsIGlobalObject* aGlobal,
> +                                           already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> +  nsCOMPtr<nsIRunnable> runnable = aRunnable;

You don't need this but just call target->Dispatch(Move(aRunnable), ...) isntead.

@@ +3132,5 @@
> +                                           already_AddRefed<nsIRunnable>&& aRunnable)
> +{
> +  nsCOMPtr<nsIRunnable> runnable = aRunnable;
> +
> +  nsCOMPtr<nsIEventTarget> target = aGlobal->EventTargetFor(TaskCategory::Other);

Add MOZ_ASSERT(aGlobal); before accessing aGlobal if you think that aGlobal will never be nullptr when accessing.

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +579,5 @@
>    nsresult OnRedirectVerifyCallback(nsresult result);
>  
>    void SetTimerEventTarget(nsITimer* aTimer);
>  
> +  nsresult DispatchRunnable(nsIGlobalObject* aGlobal,

DispatchToMainThread sounds better.
Why don't we have GetOwnerGlobal() inside its implementation instead of having aGlobal as a parameter?

Will aGlobal be different from different call site?

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #5)
> Comment on attachment 8864443 [details] [diff] [review]
> Bug 1350787 - DocGroup labeling runnables in dom/xhr
> 
> Review of attachment 8864443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to revisit again after the comments are addressed.
> 
> ::: dom/xhr/XMLHttpRequestMainThread.cpp
> @@ +2834,5 @@
> >      mSuspendedDoc = nullptr;
> >    }
> >  
> >    if (mResumeTimeoutRunnable) {
> > +    DispatchRunnable(GetOwnerGlobal(), mResumeTimeoutRunnable.forget());
> 
> Add MOZ_ASSERT(NS_IsMainThread()); at the beginning of
> XMLHttpRequestMainThread::UnsuppressEventHandlingAndResume().
In general, in class XMLHttpRequestMainThread, all functions are missing MOZ_ASSERT(NS_IsMainThread()).
I thought it's on purpose not to add it. Anyway, I will add it.

> 
> @@ +3130,5 @@
> > +nsresult
> > +XMLHttpRequestMainThread::DispatchRunnable(nsIGlobalObject* aGlobal,
> > +                                           already_AddRefed<nsIRunnable>&& aRunnable)
> > +{
> > +  nsCOMPtr<nsIRunnable> runnable = aRunnable;
> 
> You don't need this but just call target->Dispatch(Move(aRunnable), ...)
> isntead.
> 
Good catch! I now learn new stuff this time.
> @@ +3132,5 @@
> > +                                           already_AddRefed<nsIRunnable>&& aRunnable)
> > +{
> > +  nsCOMPtr<nsIRunnable> runnable = aRunnable;
> > +
> > +  nsCOMPtr<nsIEventTarget> target = aGlobal->EventTargetFor(TaskCategory::Other);
> 
> Add MOZ_ASSERT(aGlobal); before accessing aGlobal if you think that aGlobal
> will never be nullptr when accessing.
okay.
> ::: dom/xhr/XMLHttpRequestMainThread.h
> @@ +579,5 @@
> >    nsresult OnRedirectVerifyCallback(nsresult result);
> >  
> >    void SetTimerEventTarget(nsITimer* aTimer);
> >  
> > +  nsresult DispatchRunnable(nsIGlobalObject* aGlobal,
> 
> DispatchToMainThread sounds better.
> Why don't we have GetOwnerGlobal() inside its implementation instead of
> having aGlobal as a parameter?
Okay.
> 
> Will aGlobal be different from different call site?
It should not.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1350787 - DocGroup labeling runnables in dom/xhr

Comment 8

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

Comment on attachment 8864762 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr

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

Per offline discussion, it will be great if we can cache EventTarget at ::Construct() time, then we don't have to make assertion on GetOwnerGlobal() when dispatching runnables.
If there is still any limitation on caching it, then we can replace with this patch instead.

Thank!

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> Comment on attachment 8864762 [details] [diff] [review]
> Bug 1350787 - DocGroup labeling runnables in dom/xhr
> 
> Review of attachment 8864762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Per offline discussion, it will be great if we can cache EventTarget at
> ::Construct() time, then we don't have to make assertion on GetOwnerGlobal()
> when dispatching runnables.
> If there is still any limitation on caching it, then we can replace with
> this patch instead.
> 
> Thank!

I will figure out that problem for sure.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> > Comment on attachment 8864762 [details] [diff] [review]
> > Bug 1350787 - DocGroup labeling runnables in dom/xhr
> > 
> > Review of attachment 8864762 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Per offline discussion, it will be great if we can cache EventTarget at
> > ::Construct() time, then we don't have to make assertion on GetOwnerGlobal()
> > when dispatching runnables.
> > If there is still any limitation on caching it, then we can replace with
> > this patch instead.
> > 
> > Thank!
> 
> I will figure out that problem for sure.

As discussed, we don't need to have mEventTarget, but just handle in |XMLHttpRequestMainThread::DispatchToMainThread|. In ::Construct(), aGlobalObject can be nullptr.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1350787 - DocGroup labeling runnables in dom/xhr

Comment 12

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

Comment on attachment 8864796 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr

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

Thanks!

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8864796 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +3028,5 @@
>        return NS_ERROR_DOM_NETWORK_ERR;
>      } else {
>        // Defer the actual sending of async events just in case listeners
>        // are attached after the send() method is called.
> +      DispatchToMainThread(NewRunnableMethod<ProgressEventType>(this,

return ispatchToMainThread(...

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1350787 - DocGroup labeling runnables in dom/xhr, r=baku, f=bevistseng

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c64c9f357fe1b1490ed8e04d02a658f4cd7cc8a&selectedJob=97536292

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4607916d474a
DocGroup labeling runnables in dom/xhr, r=baku, f=bevistseng

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/4607916d474a