1634253 - Stop the CompositorThreadHolder from using ipc's MessageLoop and ipc::base::Thread

Status: RESOLVED FIXED

(Core :: Graphics, enhancement, P3)

Description

[Jean-Yves Avenard [:jya]]

There's really no good reason to use a MessageLoop or ipc::base::Thread; it's internally creating a thread via the same methods as your typical nsThread.

It brings no benefit and requires unnecessarily the ipc::MessageLoop::SerialEventTarget() whenever we need to work with MozPromise.

It's also get in the way of properly fixing bug 1592488

Comment 1

[Nathan Froyd [:froydnj]]

Dup of bug 1493790?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

More so a dupe of bug 1259450

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P1. Fix constness. r?froydnj

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P2. Dissociate running a BackgroundHangMonitor from main thread. r?froydnj

This will allow to have other threads to use one such as the compositor thread.

Depends on D73819

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P3. Don't cause assertion when delay is 0. r?froydnj

already_AddRefed destructor assert that it's nullptr.
DelayedDispatch check that the value passed isn't 0 and return an error code, leaving the already_AddRefed untouched.

Depends on D73820

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P4. Have NS_NewNamedThread take a already_Refed<nsIRunnable. r?froydnj

Depends on D73821

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P5. Make threading model clearer. r?kats

CompositorBridgeParent::ScheduleTask is always called from the compositor thread ; so make it explicit that we are dispatching the task to the compositor thread.

We make the method private and non-virtual as it's how it's actually used.

Depends on D73823

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P6. Remove MessageLoop use from gfx. r=kats!,mattwoodrow!

Depends on D73824

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P7. Re-enable BackgroundHangMonitor on Compositor thread. r?froydnj

We re-enable the option to have a BackgroundHangMonitor on the compositor thread that was removed earlier..

Depends on D73825

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P8. Remove use of MessageLoop in Canvas. r?mattwoodrow

MessagePool brings no benefit over the traditional nsIThread.

Additonally, replace some incorrect use of RefPtr for xpcom objects.

Depends on D73826

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)

More so a dupe of bug 1259450

I only see this comment now.

This list of patch pretty much covers everything listed in bug 1259450 comment 1.

There's no longer a MessageLoop running in the compositor thread (if you exclude the nsThread code that always creates a MessageLoop anyway).
You can no longer dispatch tasks to it.

The CompositorThread is now a nsISerialEventTarget (nsThread internally)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fac93b596c2
P1. Fix constness. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/8865de9ae0ef
P2. Dissociate running a BackgroundHangMonitor from main thread. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/6e47af64396a
P3. Don't cause assertion when delay is 0. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/dc7e17f772be
P4. Have NS_NewNamedThread take a already_Refed<nsIRunnable. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/5c0b7aa99406
P5. Make threading model clearer. r=kats
https://hg.mozilla.org/integration/autoland/rev/e3acd9db7065
P6. Remove MessageLoop use from gfx. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/53fd00dcf95c
P7. Re-enable BackgroundHangMonitor on Compositor thread. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/1cbb2866a3ad
P8. Remove use of MessageLoop in Canvas. r=mattwoodrow

Comment 14

[Dorel Luca [:dluca]]

Backed out 11 changesets (bug 1635001, bug 1634253) for Browser-chrome failures in browser_bug295977_autoscroll_overflow.js

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=301148817&repo=autoland&lineNumber=42290

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=1cbb2866a3ad7823ef1b45bdbf191721e449a80e

Backout:
https://hg.mozilla.org/integration/autoland/rev/281fa69cb1d3b3162e71c75f0cd7aac8bb46cd50

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634253 - P9. Disable intermittently failing test on win64 debug and asan. r?Gijs

The test is a permafail without any of those changes on all my windows machine so I've been unable to diagnostic what could happen.

According to :kats, the test was never rewritten to use the new APZ. Disabling on the platform where there's high intermittent failure (about 70%)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/139e7035e736
P1. Fix constness. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/fa3e7588e7d6
P2. Dissociate running a BackgroundHangMonitor from main thread. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ea62cb1ec472
P3. Don't cause assertion when delay is 0. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9f01acdf4367
P4. Have NS_NewNamedThread take a already_Refed<nsIRunnable. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/855e222ceb14
P5. Make threading model clearer. r=kats
https://hg.mozilla.org/integration/autoland/rev/e98212a74709
P6. Remove MessageLoop use from gfx. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/43eda078b405
P7. Re-enable BackgroundHangMonitor on Compositor thread. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/5f8a1ee17b81
P8. Remove use of MessageLoop in Canvas. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/d41b75c1f7ec
P9. Disable intermittently failing test on win64 debug. r=Gijs

Comment 17

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on browser_bug295977_autoscroll_overflow.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/4cfa902d0415ad8b01f6c11de76ebc8c8820ab5a
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=301450419&repo=autoland&lineNumber=6050

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/802f45438017
P1. Fix constness. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/0a19ecda120c
P2. Dissociate running a BackgroundHangMonitor from main thread. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/dff108315979
P3. Don't cause assertion when delay is 0. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/366e40b91676
P4. Have NS_NewNamedThread take a already_Refed<nsIRunnable. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/eef89c57b57a
P5. Make threading model clearer. r=kats
https://hg.mozilla.org/integration/autoland/rev/728ba655194f
P6. Remove MessageLoop use from gfx. r=kats,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/091a01b12001
P7. Re-enable BackgroundHangMonitor on Compositor thread. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/426e7df37812
P8. Remove use of MessageLoop in Canvas. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/33d3d4104e46
P9. Disable intermittently failing test on win64 debug and asan. r=Gijs

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/802f45438017
https://hg.mozilla.org/mozilla-central/rev/0a19ecda120c
https://hg.mozilla.org/mozilla-central/rev/dff108315979
https://hg.mozilla.org/mozilla-central/rev/366e40b91676
https://hg.mozilla.org/mozilla-central/rev/eef89c57b57a
https://hg.mozilla.org/mozilla-central/rev/728ba655194f
https://hg.mozilla.org/mozilla-central/rev/091a01b12001
https://hg.mozilla.org/mozilla-central/rev/426e7df37812
https://hg.mozilla.org/mozilla-central/rev/33d3d4104e46

Comment 20

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/autoland/rev/728ba655194f dropped a call to ClearAllCaches was that intentional?