Closed Bug 1350787 Opened 4 years ago Closed 4 years ago

Label runnables in dom/xhr

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: hsinyi, Assigned: shawnjohnjr)

References

Details

(Whiteboard: [QDL][TDC-MVP][DOM])

Attachments

(1 file, 4 obsolete files)

This bug is to track and review how to apply labeling APIs (https://wiki.mozilla.org/Quantum/DOM) to dom/xhr.
Summary: Label runnables in xhr/ → Label runnables in dom/xhr
Whiteboard: [QDL][TDC-MVP][DOM]
Sorry for a late response. I'm now switching to this bug.
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.
Attachment #8864443 - Flags: review?(btseng)
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?
Attachment #8864443 - Flags: review?(btseng)
(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 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!
Attachment #8864762 - Flags: feedback?(btseng)
(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.
(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 on attachment 8864796 [details] [diff] [review]
Bug 1350787 - DocGroup labeling runnables in dom/xhr

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

Thanks!
Attachment #8864796 - Flags: feedback?(btseng) → feedback+
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(...
Attachment #8864796 - Flags: review?(amarchesini) → review+
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4607916d474a
DocGroup labeling runnables in dom/xhr, r=baku, f=bevistseng
https://hg.mozilla.org/mozilla-central/rev/4607916d474a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.