Replace instances of "Services.tm.idleDispatchToMainThread" with "ChromeUtils.idleDispatch"

REOPENED
Unassigned

Status

enhancement
P3
normal
REOPENED
5 months ago
2 months ago

People

(Reporter: whimboo, Unassigned, Mentored)

Tracking

({good-first-bug})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js])

User Story

Please follow this documentation in how to setup your development environment:

https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

Attachments

(1 obsolete attachment)

Currently we use `idleDispatchToMainThread()` to wait for the delayed startup finished state of the browser before initializing Marionette:

https://dxr.mozilla.org/mozilla-central/rev/237e4c0633fda8e227b2ab3ab57e417c980a2811/testing/marionette/components/marionette.js#428

I wonder if we should better change that code and rely on the `browser-delayed-startup-finished` observer notification. Would that make sense? Would that also include the wait for the startupRecorder promise to be resolved?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(florian)
(In reply to Henrik Skupin (:whimboo) from comment #0)

> I wonder if we should better change that code and rely on the
> `browser-delayed-startup-finished` observer notification. Would that make
> sense? Would that also include the wait for the startupRecorder promise to
> be resolved?

That would be way earlier, and cause startup tests to report marionette files that wouldn't be loaded in normal Firefox runs.

What problem are you actually trying to fix?
Flags: needinfo?(florian)
I noticed the log output from:

>    log.debug(`Waiting for delayed startup...`);
>    Services.tm.idleDispatchToMainThread(async () => {

So I wondered if those are related or not. But as it looks like it's not.

As such it means there is no other way as having to call `idleDispatchToMainThread()` twice, to get what we want performance wise?
(In reply to Henrik Skupin (:whimboo) from comment #2)
> I noticed the log output from:
> 
> >    log.debug(`Waiting for delayed startup...`);
> >    Services.tm.idleDispatchToMainThread(async () => {
> 
> So I wondered if those are related or not. But as it looks like it's not.

This looks like someone (me?) forgot to update the log.debug line. I guess it should say something like "Waiting until startupRecorder.js is done recording scripts loaded during startup".

> As such it means there is no other way as having to call
> `idleDispatchToMainThread()` twice, to get what we want performance wise?

I see only one idleDispatchToMainThread call in marionette.js so I'm not sure what you mean.

Comment 4

5 months ago
I'll let Florian and you sort this out, I don't think you need my input. :-)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Florian Quèze [:florian] from comment #3)
> > >    log.debug(`Waiting for delayed startup...`);
> > >    Services.tm.idleDispatchToMainThread(async () => {
> > 
> > So I wondered if those are related or not. But as it looks like it's not.
> 
> This looks like someone (me?) forgot to update the log.debug line. I guess
> it should say something like "Waiting until startupRecorder.js is done
> recording scripts loaded during startup".

It was actually me who added this line. :/ 

> > As such it means there is no other way as having to call
> > `idleDispatchToMainThread()` twice, to get what we want performance wise?
> 
> I see only one idleDispatchToMainThread call in marionette.js so I'm not
> sure what you mean.

You are right. It's only one. But what is the reason why we have to execute all the code in `init()` on the main thread? I would like to add this as comment too, so it will be clear in the future. Thanks!
Flags: needinfo?(florian)
(In reply to Henrik Skupin (:whimboo) from comment #5)

> But what is the reason why we have to execute
> all the code in `init()` on the main thread?

I don't understand the question. As you asking because of the "ToMainThread" part of the name in the "idleDispatchToMainThread" method? All JS code executes on the main thread (unless you are using a worker), what this method does is delay execution until the browser is no longer busy.

Maybe some day I'll mass replace Services.tm.idleDispatchToMainThread to ChromeUtils.idleDispatch to reduce the confusion.
Flags: needinfo?(florian)
Ok, thanks. So I actually will add a new `executeSoon()` method in Marionette, which will replace all the various invocations of  `idleDispatchToMainThread()` across its code. I will make sure that this method calls `ChromeUtils.idleDispatch()`.

Marking as dupe of bug 1504756.
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1504756
(In reply to Henrik Skupin (:whimboo) from comment #7)
> Ok, thanks. So I actually will add a new `executeSoon()` method in
> Marionette, which will replace all the various invocations of 
> `idleDispatchToMainThread()` across its code. I will make sure that this
> method calls `ChromeUtils.idleDispatch()`.

This sounds confusing, as everywhere else executeSoon is _not_ and idle dispatch. Typical executeSoon implementations use Services.tm.dispatchToMainThread (or a setTimeout of 0ms when 'Services' isn't accessible).

Can you just call ChromeUtils.idleDispatch directly?
Oh wow. I totally missed the `idle` part of `idleDispatchToMainThread()`. And yes, `executeSoon` is using `Services.tm.dispatchToMainThread` in our files. I will make sure to fix-up those two instances in the code, which can be also seen in the following try commit:

https://hg.mozilla.org/try/rev/abc6b2d87b74a016292400ff3149c57d5fd03d7a
So lets actually do that change here then.

The code which has to be searched for instances of `Services.tm.idleDispatchToMainThread` can be found here:

https://searchfox.org/mozilla-central/source/testing/marionette/

Then each instance has to be replaced with `ChromeUtils.idleDispatch`.
Mentor: hskupin
Status: RESOLVED → REOPENED
User Story: (updated)
Keywords: good-first-bug
Resolution: DUPLICATE → ---
Summary: Revise code for waiting for delayed startup finished → Replace instances of "Services.tm.idleDispatchToMainThread" with "ChromeUtils.idleDispatch"
Whiteboard: [lang=js]
Before the startuprecorder exposed a "done" flag and Marionette was
made to wait for a promise on that [1], florian introduced a double
idle dispatch in [2].  This clears up the confusion about the “double
dispatches” earlier in this thread.

  [1] https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/testing/marionette/components/marionette.js#433-439
  [2] https://hg.mozilla.org/mozilla-central/rev/a3f93870b233
Priority: -- → P3

Comment 13

3 months ago

Hi @whimboo,
I would like to take up this task. I read all the comments but could not figure anything out. As per my understanding, I need to do what you have mentioned in comment-10 right?

That is correct. But please have a look at the user story first and how to setup your local environment, and how to run the Marionette tests.

Comment 15

2 months ago

Please assign this bug to me

Please attach a patch and we will assign the bug.

Comment 17

2 months ago

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #16)
Can you please tell me where is the code?

Please attach a patch and we will assign the bug.

(In reply to shruti10gandotra from comment #17)

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #16)
Can you please tell me where is the code?

Sure, the code is linked to in comment #0 above. It is in the
mozilla-central repository, which you will find at
https://hg.mozilla.org/mozilla-central/.

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
for a guide to getting started contributing.

Comment 20

2 months ago

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #18)
I have added the patch for review.

(In reply to shruti10gandotra from comment #17)

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #16)
Can you please tell me where is the code?

Sure, the code is linked to in comment #0 above. It is in the
mozilla-central repository, which you will find at
https://hg.mozilla.org/mozilla-central/.

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
for a guide to getting started contributing.

Comment on attachment 9048594 [details]
Replace instances of "Services.tm.idleDispatchToMainThread" with "ChromeUtils.idleDispatch"

Please note that this is not a patch, but the full file. Check the documentation again in how to create a review request on Phabricator. 

https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Patches.html
Attachment #9048594 - Attachment is obsolete: true
Attachment #9048594 - Attachment is patch: false
Attachment #9048594 - Flags: review+
You need to log in before you can comment on or make changes to this bug.