Closed Bug 1046292 Opened 7 years ago Closed 7 years ago

Make TaskThrottler more robust in the face of missing TaskComplete calls

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(2 files, 2 obsolete files)

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).
Attached patch Patch (obsolete) — Splinter Review
Attached patch PatchSplinter Review
Attachment #8464961 - Flags: review?(botond)
Attached patch Tests (obsolete) — Splinter Review
Attachment #8464898 - Attachment is obsolete: true
Attachment #8464962 - Flags: review?(botond)
Attachment #8464961 - Flags: review?(botond) → review+
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.)
Attachment #8464962 - Flags: review?(botond) → review+
Attached patch Tests (v2)Splinter Review
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.
Attachment #8464962 - Attachment is obsolete: true
Attachment #8465120 - Flags: review?(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.
Attachment #8465120 - Flags: review?(botond) → review+
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
https://hg.mozilla.org/mozilla-central/rev/49f9f9140f88
https://hg.mozilla.org/mozilla-central/rev/bd81690c70e7
https://hg.mozilla.org/mozilla-central/rev/9eeb976a6ca5
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.