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

With the landing of APZ on webrender, the dom/canvas/test/webgl-mochitest/test_capture.html test started failing quite frequently on linux64-qr platforms. This spike in intermittents was lumped together with a pre-existing android failure in a different test_capture.html test (the one at dom/canvas/test/test_capture.html) into bug 1206887.

This bug is specifically about tracking down and fixing the "new" failures on the linux64-qr platform. Here is an orangefactor link for the bug tracking the intermittents; type "linux64-qr" into the search filter to see the new failures.

https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1206887&startday=2017-06-11&endday=2017-06-27&tree=all

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

I looked through the failing run output, the last bit of which is reproduced below:

TEST-PASS | dom/canvas/test/webgl-mochitest/test_capture.html | vmanual should still be red
WebGL: drawArrays(green)
Requesting frame from vmanual
Waiting for video vmanual to match [0,255,0,255] - green (should become green after call order test)
WebGL: clearColor(red)
Frame: 0 IsPixel ref=0,255,0,255 threshold=0 value=255,0,0,255
Buffered messages finished
TEST-UNEXPECTED-FAIL | dom/canvas/test/webgl-mochitest/test_capture.html | Test timed out.

I compared this to what the test is supposed to do and to a passing local run. The interesting thing here is the "WebGL: clearColor(red)" call in the middle of the output below - that's not supposed to happen, and that's what causes the green pixel to end up red and the test to time out while it waits for the expected green pixel to appear at [1].

I suspect the reason this clearColor(red) appears is because the the previous hunk of the test registered a requestAnimationFrame callback that does a red clearColor [2]. That previous hunk did clean up after itself by calling "stop" on the RAF looper code [3] but I think there's a bug in this code.

Specifically, the RAF looper code only uses the stop variable to stop *additional* RAF calls, but it still runs the provided callback function [4]. That means if a RAF call is queued up at the time stop() is called, the next frame will still run the callback function. And I suspect that's what happening in this case, allowing the "clearColor(red)" call to leak outside the hunk of the test that it's supposed to be contained in.

I can see two possible fixes: one is to expand the scope of the "if (stop)" block to include the callback function call. The other is to do a cancelAnimationFrame call when stop() is called. The first of these two approaches seems like the more conservative approach, because I'm not sure what other tests use this code and if they implicitly or explicitly assume there's going to be another animation frame coming. Cancelling the animation frame might cause those tests to hang if that's the case.

[1] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/dom/canvas/test/webgl-mochitest/test_capture.html#114
[2] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/dom/canvas/test/webgl-mochitest/test_capture.html#93
[3] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/dom/canvas/test/webgl-mochitest/test_capture.html#104
[4] http://searchfox.org/mozilla-central/rev/b425854d9bbd49d5caf9baef3686e49ec91c17ec/dom/canvas/test/captureStream_common.js#45-46

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

Marking this as blocking bug 1161913 also since that's the bug that introduced this code.

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

Try push is looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f453a7a45c22afe52a14d71e0be2a1ae2e9e18c

	Andreas Pehrson [:pehrsons]
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8881408 [details]
Bug 1376449 - Robustify startDrawing to prevent calling callback function after stop() has been called.

https://reviewboard.mozilla.org/r/152566/#review157690

nit: IMO `if (stop) { return; }` is cleaner.

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

Thanks, updated patch to do the early-return.

	Pulsebot
	Comment 8 • 7 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11e88910799f
Robustify startDrawing to prevent calling callback function after stop() has been called. r=pehrsons

	Carsten Book [:Tomcat]
	Comment 9 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/11e88910799f