1046292 - Make TaskThrottler more robust in the face of missing TaskComplete calls

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

The APZ code uses TaskThrottler instances to throttle repaint requests sent to content. However there are various error conditions that might arise while trying to process a repaint request that might result in no repaint actually happening. In fact there are probably legitimate conditions in which layout can decide nothing has changed and so no repaint is needed. This can result in the TaskThrottler getting stuck and not sending out new repaint requests until layout does a repaint for some other reason.

Although no cases of this have been spotted in the wild, I would like to robustify TaskThrottler against this. (Mostly so I don't have to cargo-cult a bad error-handling approach to Fennec).

Comment 1

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

Attachment: Patch

Comment 2

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

Attachment: Patch

Comment 3

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

Attachment: Tests

Comment 4

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

https://tbpl.mozilla.org/?tree=Try&rev=12971e166e7c

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8464962 [details] [diff] [review]
Tests

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

Nice tests!

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1753,5 @@
> +
> +private:
> +    // These are static because the Task is stored in a UniquePtr and
> +    // destroyed right after running, and otherwise we would have no way
> +    // of getting information out of them.

I can think of ways of getting that information that don't involve static variables :)

For example, the MockTask constructor could take a reference to a Metrics object, and increment fields contained within it. Calling code would maintain a Metrics object, and pass it in when constructing MockTasks.

That said, I assume that gtests don't run in parallel, and that these static variables aren't the first thing that would break if they did, so they're fine.

@@ +1771,5 @@
> +  }
> +
> +  virtual void TearDown()
> +  {
> +    delete throttler;

According to https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/destructor_of_the_test_fixture_or_t, gtest calls SetUp() immediately after the constructor of the test fixture, and TearDown() immediately before the destructor, so the constructor and destructor are also suitable for doing setup/teardown. (In fact, they recommend using the constructor/destructor except in some cases (such as the teardown throwing an exception), that don't apply here.)

If we use the constructor/destructor, we can make 'throttler' a UniquePtr<TaskThrottler> and avoid the manual new/delete.

@@ +1783,5 @@
> +
> +  UniquePtr<CancelableTask> NewTask()
> +  {
> +    // compiler doesn't seem to be able to deduce the right constructor,
> +    // so doing it manually in this helper to avoid polluting test code.

If you mean that the compiler won't accept a CancelableTask* (or MockTask*) as an argument for a UniquePtr<CancelableTask> parameter, this is intentional. Wrapping a pointer into a UniquePtr entails taking ownership, and UniquePtr enforces being explicit about that.

The idiomatic way to create an object and wrap it into a UniquePtr right away is 'MakeUnique<T>(args)' - in this case, 'MakeUnique<MockTask>()'. (You can still wrap that into NewTask() if you prefer.)

Comment 6

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

Attachment: Tests (v2)

I liked the metrics idea instead of static variables so I did that, and ended up implementing the rest of your suggestions as well. I also fixed the indentation since I accidentally used 4-space for most it; it's all 2-space now. Re-requesting review since a bunch of the stuff changed.

Comment 7

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

https://tbpl.mozilla.org/?tree=Try&rev=d942ac8bd104

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8465120 [details] [diff] [review]
Tests (v2)

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

For good measure let's #include <mozilla/UniquePtr.h>.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1767,5 @@
> +    : mMetrics(aMetrics)
> +  {}
> +
> +  virtual void Run() {
> +    if (mMetrics) {

Is there any point to these checks? I'd remove them and make mMetrics a reference unless we have a use case for not having a metrics.

Comment 9

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

Made changes and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/49f9f9140f88
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd81690c70e7

Comment 10

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

Bustage fix, apparently I ran |mach build .| in the wrong directory after I made the above changes...

https://hg.mozilla.org/integration/mozilla-inbound/rev/9eeb976a6ca5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49f9f9140f88
https://hg.mozilla.org/mozilla-central/rev/bd81690c70e7
https://hg.mozilla.org/mozilla-central/rev/9eeb976a6ca5