1635001 - Stop the APZ code from using ipc's MessageLoop and ipc::base::Thread

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Jean-Yves Avenard [:jya]]

Similar to bug 1634253;

APZ uses a MessageLoop and Google's base::Thread.

Should be modified to run of nsIThread instead.

Comment 1

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

If you are implementing this please proceed with extreme caution. APZ sits in a vortex of many threads which change depending on platform and runtime configuration, and making everything work properly with the right interleavings of things is not for the faint of heart. I'm not opposed to attempting this, but please request feedback early and often to make sure you're not going down a path that will cause architectural problems. If you have an outline of what you plan to do I'm happy to provide feedback on it.

Comment 2

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

Some relevant documentation: https://firefox-source-docs.mozilla.org/gfx/AsyncPanZoom.html#threading-locking-overview

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1635001 - P1. Make threading model clearer when dispatching tasks on the controller thread. r?kats

RemoteContentController::PostDelayedTask must be called on the controller thread; and all the 3 implementations are doing is dispatching the task on the current MessageLoop.

The naming and that the method was pure virtual made it a bit confusing as it gave the impression we would dispatch the task on the controller's internal thread, which wasn't the case.

So we make a generic implementation and assert to the documented use.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1635001 - P2. Don't use MessageLoop threads with APZ. r?kats

It is unclear on why MessageLoop was ever used with this code.

Depends on D73828

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1635001 - P3. Fix APZ controller thread-safety access. r?kats

The APZ was keeping a raw pointer to the controller thread. This was a dangerous exercise.
This was okay on desktop, as the controller thread was the main thread and would have outlived everything else. On Android however it's the UI thread and it could get deleted before we received a last input event.

So we use a strong pointer instead to prevent the thread from being deleted and as such, we now needs to explicitly clear it on shutdown.

This requires the various methods in APZThreadUtils to be made thread-safe so that the controller thread can be shutdown mid-air.

Depends on D73829

Comment 6

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

If there is a dependency between this bug and bug 1634253 please indicate it with the blocks/depends on. It seems like conceptually they are kind of independent but in terms of implementation there might be a dependency.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3cb0a05be7fc
P1. Make threading model clearer when dispatching tasks on the controller thread. r=kats
https://hg.mozilla.org/integration/autoland/rev/6cea251e5910
P2. Don't use MessageLoop threads with APZ. r=kats,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/c3c27cb46db6
P3. Fix APZ controller thread-safety access. r=kats

Comment 8

[Jean-Yves Avenard [:jya]]

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

If there is a dependency between this bug and bug 1634253 please indicate it with the blocks/depends on. It seems like conceptually they are kind of independent but in terms of implementation there might be a dependency.

There's no dependency to bug 1634253 ; could be done independently, it's all part of getting bug 1260828 done.

Comment 9

[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 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26daca5f7dab
P1. Make threading model clearer when dispatching tasks on the controller thread. r=kats
https://hg.mozilla.org/integration/autoland/rev/bc73609fc638
P2. Don't use MessageLoop threads with APZ. r=kats,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/62d98215d177
P3. Fix APZ controller thread-safety access. r=kats

Comment 11

[Narcis Beleuzu [:NarcisB]]

Backed out for causing bustages on RemoteContentController.cpp

(Conflicted with the backout of Bug 1634253)

Backout link: https://hg.mozilla.org/integration/autoland/rev/5d4199425f57cf5657ace9e868924164286e0f43
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=301457424&repo=autoland&lineNumber=28926

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bd4793e6d09
P1. Make threading model clearer when dispatching tasks on the controller thread. r=kats
https://hg.mozilla.org/integration/autoland/rev/40c6722e7cde
P2. Don't use MessageLoop threads with APZ. r=kats,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/49df0c8504b8
P3. Fix APZ controller thread-safety access. r=kats

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3bd4793e6d09
https://hg.mozilla.org/mozilla-central/rev/40c6722e7cde
https://hg.mozilla.org/mozilla-central/rev/49df0c8504b8