Reftest harness should trigger and wait for pending repaints from APZ

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

I wrote some new tests in bug 1151617 for testing the async positioning of scrollbars when APZ is enabled. However while running the tests locally on Linux using |mach reftest-ipc| I noticed that they would intermittently fail. In the failing cases, there was another repaint happening after reftest-content.js moved to the "completed" state, and so the repaint was getting "lost" in that the parent process reftest harness code was never notified and never updated the snapshot.

It turned out that the extra repaint was coming from the APZ code; my test was doing a scrollTo which sends a layer transaction to the compositor. The APZ code then updated the displayport margins and sent a repaint request back to the content side, and that was triggering the repaint.

For testing purposes we need a way to flush these sorts of pending repaint requests and allow test harnesses to wait until they have been processed. I have patches that do this.
Comment on attachment 8604353 [details] [diff] [review]
Part 1 - Add an API to flush APZ repaints

Most of this patch is just passing around the messages. The flush request entry point is in DOMWindowUtils and goes through to APZCTM. That flushes all the APZCs for the process and then sends a completion notification back out on the same path that repaint requests take. Therefore the completion notification is guaranteed to be processed after the repaint requests.

dvander, let me know if this works for the dom mochitests you mentioned as well.
Attachment #8604353 - Flags: review?(botond)
Attachment #8604353 - Flags: feedback?(dvander)
Comment on attachment 8604353 [details] [diff] [review]
Part 1 - Add an API to flush APZ repaints

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +559,5 @@
> +{
> +  MonitorAutoLock lock(mTreeLock);
> +  nsRefPtr<AsyncPanZoomController> root = FlushPendingRepaintRecursively(mRootNode, aLayersId);
> +  MOZ_ASSERT(root);
> +  root->NotifyFlushComplete();

Oh, apparently there's legitimate scenarios where root can be null here... need to change this bit slightly.
Attachment #8604353 - Flags: review?(botond)
Attachment #8604353 - Flags: feedback?(dvander)
Specifically, I was seeing this reftest fail on linux (try and locally):

mach reftest --e10s layout/reftests/reftest-sanity/test-async.xul

This test creates a XUL window as the root in the child process, and therefore we were getting no APZCs in the child process. I don't believe any of our production environments do this so it might not have impacted real-world cases but handling this actually makes the code a bit simpler.
Attachment #8604353 - Attachment is obsolete: true
Attachment #8604682 - Flags: review?(botond)
Attachment #8604682 - Flags: feedback?(dvander)
Attachment #8604682 - Flags: feedback?(dvander) → feedback+
Comment on attachment 8604682 [details] [diff] [review]
Part 1 - Add an API to flush APZ repaints (v2)

Ugh, found another issue. Dropping review until I sort it out.
Attachment #8604682 - Flags: review?(botond)
Comment on attachment 8605808 [details] [diff] [review]
Part 1 - Add an API to flush APZ repaints (v3)

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

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1436,5 @@
> +   * Do a round-trip to the compositor to ensure any pending APZ repaint requests
> +   * get flushed to the main thread. If the function returns true, the flush was
> +   * triggered and an "apz-repaints-flushed" notification will be dispatched via
> +   * the observer service once the flush is complete. If the function returns
> +   * false, no flush is required.

I find this comment a bit misleading. "No flush is required" sounds like "we checked for pending APZ repaint requests, and there are none", but the function only returns false in some error conditions (something is null) or if APZ is disabled.

@@ +1438,5 @@
> +   * triggered and an "apz-repaints-flushed" notification will be dispatched via
> +   * the observer service once the flush is complete. If the function returns
> +   * false, no flush is required.
> +   */
> +  bool flushApzRepaints();

Please mention that this is intended to be used by test code only.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +563,5 @@
> +  }
> +  const CompositorParent::LayerTreeState* state = CompositorParent::GetIndirectShadowTree(aLayersId);
> +  MOZ_ASSERT(state && state->mController);
> +  NS_DispatchToMainThread(NS_NewRunnableMethod(
> +    state->mController.get(), &GeckoContentController::NotifyFlushComplete));

Might it make more sense to have the NotifyFlushComplete bit in CompositorParent? (I suppose it doesn't really matter.)
Attachment #8605808 - Flags: review?(botond) → review+
Comment on attachment 8605809 [details] [diff] [review]
Part 2 - Update the reftest harness to wait for APZ repaints (v2)

This doesn't quite seem to be using the normal pattern (e.g., of having the observer set a boolean and having the progress advancement actually happen in MakeProgress), but I guess it's ok.

Maybe at least add a LogInfo() in the new case so that we know if we're getting MakeProgress calls from other sources while in that state?

Do you think the order of this happening before the MozPaintWait and MozPaintWaitFinished event checks is correct?

r=dbaron
Attachment #8605809 - Flags: review?(dbaron) → review+
(In reply to Botond Ballo [:botond] from comment #10)
> I find this comment a bit misleading. "No flush is required" sounds like "we
> checked for pending APZ repaint requests, and there are none", but the
> function only returns false in some error conditions (something is null) or
> if APZ is disabled.

Updated the comment

> > +  bool flushApzRepaints();
> 
> Please mention that this is intended to be used by test code only.

Done

> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> > +  NS_DispatchToMainThread(NS_NewRunnableMethod(
> > +    state->mController.get(), &GeckoContentController::NotifyFlushComplete));
> 
> Might it make more sense to have the NotifyFlushComplete bit in
> CompositorParent? (I suppose it doesn't really matter.)

It would work there but I prefer to keep it more encapsulated inside the APZ code.


(In reply to David Baron [:dbaron] ⏰UTC-4 from comment #11)
> Maybe at least add a LogInfo() in the new case so that we know if we're
> getting MakeProgress calls from other sources while in that state?

Added

> Do you think the order of this happening before the MozPaintWait and
> MozPaintWaitFinished event checks is correct?

Yeah, I'm pretty sure it's correct because the flush can trigger a paint and so we still want to wait for that paint to finish after doing the flush.
Filed bug 1167578 for the try push in comment 8 not actually running everything I expected it to.

I can sort of reproduce an issue locally on my emulator build but debugging it is going to be slow.
Looks like I need this to fix a frequently-occurring race in my new test in bug 1139155, so I'm going to look into getting this relanded.
Blocks: 1139155
The problem turned out to be just a silly mistake with the layers id I was using. I fixed it and the try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd331fbe349a. Will reland shortly, carrying previous reviews.
https://hg.mozilla.org/mozilla-central/rev/ee20787262f2
https://hg.mozilla.org/mozilla-central/rev/d33e17744ddf
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.