Closed Bug 1343475 Opened 3 years ago Closed 3 years ago

Label runnables for platform-independent widget

Categories

(Core :: Widget, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kechen, Assigned: vliu)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 2 obsolete files)

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
Priority: -- → P3
Whiteboard: tpi:+
Assignee: nobody → kechen
Assignee: kechen → vliu
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+
> 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.
(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.
Hi Smaug,

Could you please have a review for the patch? Thanks
Attachment #8850278 - Attachment is obsolete: true
Attachment #8851850 - Flags: review?(bugs)
(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)
(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]
> 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+
Priority: P3 → P1
(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+
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
https://hg.mozilla.org/mozilla-central/rev/9bf82d52f479
https://hg.mozilla.org/mozilla-central/rev/e67191681118
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.