Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 9 years ago

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=90046f7147bc

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 3 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 7 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 9 years ago
green try

Ok, got it working. https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdc83a680bd5

	Botond Ballo [:botond]
	Comment 10 • 9 years ago

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.)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 11 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 12 • 9 years ago

(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.

	Pulsebot
	Comment 13 • 9 years ago
landing

https://hg.mozilla.org/integration/mozilla-inbound/rev/0867630f82f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/3373b602df4b

	Nigel Babu [:nigelb]
	Comment 14 • 9 years ago

You may have caused this bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=10063673&repo=mozilla-inbound

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 9 years ago
backout

Backed out until I can investigate: https://hg.mozilla.org/integration/mozilla-inbound/rev/bee4bdb3624c

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 17 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 18 • 9 years ago

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.

	Pulsebot
	Comment 19 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee20787262f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d33e17744ddf

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/ee20787262f2
https://hg.mozilla.org/mozilla-central/rev/d33e17744ddf