Label runnables for platform-independent widget

RESOLVED FIXED in Firefox 55

Status

()

Core
Widget
P1
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: kechen, Assigned: vliu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: tpi:+[QDL][TDC-MVP][GFX])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 months ago
See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story.

The followings are current runnables that need to be labeled.

1. PuppetWidget.cpp:    NS_DispatchToCurrentThread(mPaintTask.get());
 -> https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/PuppetWidget.cpp#314

2. nsBaseAppShell.cpp: return NS_SUCCEEDED(aTarget->Dispatch(mDummyEvent, NS_DISPATCH_NORMAL));
  -> https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsBaseAppShell.cpp#313

3. nsBaseWidget.cpp
  mLongTapTimer->InitWithFuncCallback(OnLongTapTimerCallback, this,
                                      timeout,
                                      nsITimer::TYPE_REPEATING_SLACK);
  -> https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsBaseWidget.cpp#1983

4. nsIdleService.cpp
  (void)mTimer->InitWithFuncCallback(DailyCallback,
                                       this,
                                       SECONDS_PER_DAY * PR_MSEC_PER_SEC,
                                       nsITimer::TYPE_ONE_SHOT);
  -> https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#146
  (void)mTimer->InitWithFuncCallback(DailyCallback,
                                     this,
                                     milliSecLeftUntilDaily,
                                     nsITimer::TYPE_ONE_SHOT);
  -> https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#221

  (void)self->mTimer->InitWithFuncCallback(DailyCallback,
                                           self,
                                           delayTime / PR_USEC_PER_MSEC,
                                           nsITimer::TYPE_ONE_SHOT);
  -> https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#295

  mTimer->InitWithFuncCallback(StaticIdleTimerCallback,
                               this,
                               deltaTime.ToMilliseconds(),
                               nsITimer::TYPE_ONE_SHOT);
  -> https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#833

5. nsNativeTheme.cpp
  rv = mAnimatedContentTimer->InitWithCallback(this, timeout,
                                               nsITimer::TYPE_ONE_SHOT);
  -> https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsNativeTheme.cpp#650

Updated

8 months ago
Priority: -- → P3
Whiteboard: tpi:+
(Reporter)

Updated

7 months ago
Assignee: nobody → kechen
(Reporter)

Updated

7 months ago
Assignee: kechen → vliu
(Assignee)

Comment 1

7 months ago
Created attachment 8850278 [details] [diff] [review]
0001-Bug-1343475-Add-labeling-for-Dispatch-in-PuppetWidge.patch

Hi Bevis,
The attached patch intends to add labeling for Dispatch in PuppetWidget::Invalidate(). Could you please have a feedback to see if the patch is valid for labeling? Thanks.
Attachment #8850278 - Flags: feedback?(btseng)
Comment on attachment 8850278 [details] [diff] [review]
0001-Bug-1343475-Add-labeling-for-Dispatch-in-PuppetWidge.patch

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

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

::: widget/PuppetWidget.cpp
@@ +311,5 @@
>    mDirtyRegion.Or(mDirtyRegion, aRect);
>  
>    if (!mDirtyRegion.IsEmpty() && !mPaintTask.IsPending()) {
>      mPaintTask = new PaintTask(this);
> +    if (XRE_IsContentProcess() && NS_IsMainThread()) {

Add assertion here if mTabChild will never be nullptr when Invalidate() is called:
    MOZ_ASSERT(mTabChild);

@@ +312,5 @@
>  
>    if (!mDirtyRegion.IsEmpty() && !mPaintTask.IsPending()) {
>      mPaintTask = new PaintTask(this);
> +    if (XRE_IsContentProcess() && NS_IsMainThread()) {
> +      RefPtr<TabGroup> tabGroup = mTabChild->TabGroup();

As we checked yesterday, mTabChild->TabGroup() is infallible except in PuppetWidget's constructor. Hence, we can simplify this dispatching as followed:
    nsCOMPtr<nsIRunnable> event(mPaintTask.get());
    // PaintTask has been named in its constructor.
    mTabChild->TabGroup()->Dispatch(nullptr, TaskCategory::Other, event.forget());

@@ +316,5 @@
> +      RefPtr<TabGroup> tabGroup = mTabChild->TabGroup();
> +      nsCOMPtr<nsIRunnable> event(mPaintTask.get());
> +      tabGroup->Dispatch("PuppetWidget::Invalidate", TaskCategory::Other, event.forget());
> +    } else {
> +      NS_DispatchToCurrentThread(mPaintTask.get());

Can we double check if Invalidate() will be run off-main-thread?

If not, we could add MOZ_ASSERT(NS_IsMainThread()); at the beginning of this function to simplify the if-else statement as:
if (XRE_IsContentProcess()) {
  ... // TabGroup Dispatching
} else {
  NS_DispatchToMainThread(mPaintTask.get());
}
Attachment #8850278 - Flags: feedback?(btseng) → feedback+
(Assignee)

Comment 3

7 months ago
> 2. nsBaseAppShell.cpp: return NS_SUCCEEDED(aTarget->Dispatch(mDummyEvent,
> NS_DISPATCH_NORMAL));
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsBaseAppShell.cpp#313
> 

For the Dispatch() in nsBaseAppShell, I would not labeling it because it only used during shutdown phase.
(Assignee)

Comment 4

7 months ago
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #2)

> Add assertion here if mTabChild will never be nullptr when Invalidate() is
> called:
>     MOZ_ASSERT(mTabChild);
> 

Added


> 
> As we checked yesterday, mTabChild->TabGroup() is infallible except in
> PuppetWidget's constructor. Hence, we can simplify this dispatching as
> followed:
>     nsCOMPtr<nsIRunnable> event(mPaintTask.get());
>     // PaintTask has been named in its constructor.
>     mTabChild->TabGroup()->Dispatch(nullptr, TaskCategory::Other,
> event.forget());

Done with next patch.


> Can we double check if Invalidate() will be run off-main-thread?
> 
> If not, we could add MOZ_ASSERT(NS_IsMainThread()); at the beginning of this
> function to simplify the if-else statement as:
> if (XRE_IsContentProcess()) {
>   ... // TabGroup Dispatching
> } else {
>   NS_DispatchToMainThread(mPaintTask.get());
> }

Because I am not sure whether this function run only on main-thread or not, I will keep the original implementation.
(Assignee)

Comment 5

7 months ago
Created attachment 8851850 [details] [diff] [review]
0001-Bug-1343475-Add-labeling-for-Dispatch-in-PuppetWidge.patch

Hi Smaug,

Could you please have a review for the patch? Thanks
Attachment #8850278 - Attachment is obsolete: true
Attachment #8851850 - Flags: review?(bugs)
(Assignee)

Comment 6

7 months ago
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #0)
> See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story.
> 
> The followings are current runnables that need to be labeled.
> 
> 
> 4. nsIdleService.cpp
>   (void)mTimer->InitWithFuncCallback(DailyCallback,
>                                        this,
>                                        SECONDS_PER_DAY * PR_MSEC_PER_SEC,
>                                        nsITimer::TYPE_ONE_SHOT);
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#146
>   (void)mTimer->InitWithFuncCallback(DailyCallback,
>                                      this,
>                                      milliSecLeftUntilDaily,
>                                      nsITimer::TYPE_ONE_SHOT);
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#221
> 
>   (void)self->mTimer->InitWithFuncCallback(DailyCallback,
>                                            self,
>                                            delayTime / PR_USEC_PER_MSEC,
>                                            nsITimer::TYPE_ONE_SHOT);
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#295
> 
>   mTimer->InitWithFuncCallback(StaticIdleTimerCallback,
>                                this,
>                                deltaTime.ToMilliseconds(),
>                                nsITimer::TYPE_ONE_SHOT);
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#833
> 

Hi froydnj,

Recently I got into the work on dom labeling for Dispatch and TimerCallback. Please see [1] for details. From TimerCallback in nsIdleService.cpp, I also had a look for this. Since I am not sure if the above InitWithFuncCallback are run on main-thread in Content process, I would like to have your comment to figure it out. Really thanks.


[1]: https://wiki.mozilla.org/Quantum/DOM#Labeling
Flags: needinfo?(nfroyd)
(Assignee)

Comment 7

7 months ago
Created attachment 8851926 [details] [diff] [review]
0001-Add-labeling-for-InitWithCallback-in-nsNativeTheme-Q.patch

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #0)
> See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story.
> 
> The followings are current runnables that need to be labeled.
> 

> 
> 5. nsNativeTheme.cpp
>   rv = mAnimatedContentTimer->InitWithCallback(this, timeout,
>                                                nsITimer::TYPE_ONE_SHOT);
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsNativeTheme.cpp#650

The attached patch intends to add labeling for 5. Could you please have a review? Thanks
Attachment #8851926 - Flags: review?(bugs)
(In reply to Vincent Liu[:vliu] from comment #6)
> Recently I got into the work on dom labeling for Dispatch and TimerCallback.
> Please see [1] for details. From TimerCallback in nsIdleService.cpp, I also
> had a look for this. Since I am not sure if the above InitWithFuncCallback
> are run on main-thread in Content process, I would like to have your comment
> to figure it out. Really thanks.
> 
> [1]: https://wiki.mozilla.org/Quantum/DOM#Labeling

They are all run on the main thread.  I'm not sure whether they run in the main process, content process, or both.  I think that, given that the idle service can write prefs, and prefs have to be written by the main process (?), that the idle service would only run in the main process and not the content process.  But I'm not aware of anything that guarantees that.
Flags: needinfo?(nfroyd)
Whiteboard: tpi:+ → tpi:+[QDL][TDC-MVP][GFX]
(Assignee)

Comment 9

7 months ago
> 4. nsIdleService.cpp
>   (void)mTimer->InitWithFuncCallback(DailyCallback,
>                                        this,
>                                        SECONDS_PER_DAY * PR_MSEC_PER_SEC,
>                                        nsITimer::TYPE_ONE_SHOT);
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#146
>   (void)mTimer->InitWithFuncCallback(DailyCallback,
>                                      this,
>                                      milliSecLeftUntilDaily,
>                                      nsITimer::TYPE_ONE_SHOT);
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#221
> 
>   (void)self->mTimer->InitWithFuncCallback(DailyCallback,
>                                            self,
>                                            delayTime / PR_USEC_PER_MSEC,
>                                            nsITimer::TYPE_ONE_SHOT);
>   ->
> https://searchfox.org/mozilla-central/rev/
> 9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/widget/nsIdleService.cpp#295
> 

I won't label InitWithFuncCallback in nsIdleServiceDaily because it lives only in Parent process. See [1] to know the detail. Please correct me if I got anything wrong.

[1]: http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/widget/nsIdleService.cpp#402
Comment on attachment 8851850 [details] [diff] [review]
0001-Bug-1343475-Add-labeling-for-Dispatch-in-PuppetWidge.patch

>   if (!mDirtyRegion.IsEmpty() && !mPaintTask.IsPending()) {
>     mPaintTask = new PaintTask(this);
>-    NS_DispatchToCurrentThread(mPaintTask.get());
>+    MOZ_ASSERT(mTabChild);
>+    if (XRE_IsContentProcess() && NS_IsMainThread()) {
>+      nsCOMPtr<nsIRunnable> event(mPaintTask.get());
>+      mTabChild->TabGroup()->Dispatch("PuppetWidget::Invalidate", TaskCategory::Other, event.forget());
>+    } else {
>+      NS_DispatchToCurrentThread(mPaintTask.get());
>+    }
This doesn't look quite right. When does parent process have puppet widget, or mTabChild?

Always just dispatch using mTabChild and if that is not available, I think the dispatching can be skipped.
I could take a look at the new patch too.
Attachment #8851850 - Flags: review?(bugs) → review-
Attachment #8851926 - Flags: review?(bugs) → review+
(Assignee)

Updated

6 months ago
Priority: P3 → P1
(Assignee)

Comment 11

6 months ago
Created attachment 8856777 [details] [diff] [review]
0001-Bug-1343475-Add-labeling-for-Dispatch-in-PuppetWidge.patch

(In reply to Olli Pettay [:smaug] (review queue closed. Please ask reviews from other DOM peers) from comment #10)

> This doesn't look quite right. When does parent process have puppet widget,
> or mTabChild?
> 

Yes, you are right. puppet widget and mTabChild never have chance in parent process. 

> Always just dispatch using mTabChild and if that is not available, I think
> the dispatching can be skipped.
> I could take a look at the new patch too.

Could you please have a review? Thanks
Attachment #8851850 - Attachment is obsolete: true
Attachment #8856777 - Flags: review?(bugs)
Attachment #8856777 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

6 months ago
try result: https://treeherder.mozilla.org/#/jobs?repo=try&tochange=33c6ded24ac508bacf0d1527619f370a1d5aa80a&fromchange=0628d22e442567ce79bdb03e4cad6297496ab8b2&selectedJob=90723767

Comment 13

6 months ago
Pushed by vliu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf82d52f479
Add labeling for Dispatch in PuppetWidget::Invalidate(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e67191681118
Add labeling for InitWithCallback in nsNativeTheme::QueueAnimatedContentForRefresh(). r=smaug

Comment 14

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9bf82d52f479
https://hg.mozilla.org/mozilla-central/rev/e67191681118
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.