Benoit Girard (:BenWa) Assignee
	Description • 10 years ago

Bug 1112332 introduced a regression where we would take the tiling 'FastPath' when we had progressive drawing enabled causing us to OOM. Bug 1121871 landed an incorrect fix for that OOM.

We need to revert the changes made in bug 1112332 and 1121871. At the same time the logic to disallow taking the fast path when using low-precision is really fragile and should be improved.

The aim of this bug is to clean this up.

	Benoit Girard (:BenWa) Assignee
	Comment 1 • 10 years ago

Try is closed, will push later.

	Benoit Girard (:BenWa) Assignee
	Comment 2 • 10 years ago

[Blocking Requested - why for this release]:
2.1 blocker since it's required to fix 2.1+ bug 1121871.

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

Comment on attachment 8566670 [details] [diff] [review]
patch

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

LGTM.

::: gfx/layers/client/ClientTiledPaintedLayer.cpp
@@ +277,3 @@
>  
>    // Only draw progressively when the resolution is unchanged, and we're not
>    // in a reftest scenario (that's what the HasShadowManager() check is for).

Move this bit of the comment about the reftests and HasShadowManager() up into UseProgressiveDraw(). I guess it's HasShadowTarget() now.

	Benoit Girard (:BenWa) Assignee
	Comment 5 • 10 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a591462aeb7a

	Benoit Girard (:BenWa) Assignee
	Comment 6 • 9 years ago

The patch had a git style commit message from the b2g environment. Fixed.

	Benoit Girard (:BenWa) Assignee
	Comment 7 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca73b950e612

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

Backed out for B2G crashtest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1af783edd1

https://treeherder.mozilla.org/logviewer.html#?job_id=6811373&repo=mozilla-inbound

	Benoit Girard (:BenWa) Assignee
	Comment 9 • 9 years ago

It looks like the failure of this patch is really a problem from the flip from bug 950934. This bug masked the issue that bug 950934 introduced.

This means that we should be able to land this patch on 2.1 without dealing with this issue.

This issue can be reproduce on mac with the following pref flips:
+pref("layers.low-precision-buffer", true);
+pref("layers.progressive-paint", true);
+pref("layers.async-pan-zoom.enabled", true);

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

I reproduced this and I think tracked down the root cause at [1] - the Intersect call returns bad results on values that are as large as the one in this test case:

aDirtyRect is (x=1073741344, y=1073741344, w=1073756696, h=1073819936) mScrollPort is (x=1073741820, y=1073741820, w=14400, h=77640)
dirtyRect is (x=1073741820, y=1073741820, w=1073756220, h=1073819460)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=54aff1390b16#2920

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

New try push including the patch from bug 1135677:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e1671117dfb

	Benoit Girard (:BenWa) Assignee
	Comment 12 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b06da9ddc08

	Benoit Girard (:BenWa) Assignee
	Comment 13 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d96b0a1376

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a33397c97370

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

That one still wasn't fast enough. Now with progressive-paint disabled also:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0f28536955

	Benoit Girard (:BenWa) Assignee
	Comment 16 • 9 years ago

Still waiting on a slave... I've up the test priority.

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

That last one has a passing C2, so things are looking good. This is the patch corresponding to that try push, which has progressive-painting turned into a live pref that is disabled on that test.

	Benoit Girard (:BenWa) Assignee
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc07fc31d234

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

https://hg.mozilla.org/mozilla-central/rev/dc07fc31d234

	Benoit Girard (:BenWa) Assignee
	Comment 20 • 9 years ago

Comment on attachment 8568935 [details] [diff] [review]
patch v4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1112332
User impact if declined: OOM and/or black glitches
Testing completed: local testing, we should let this bake on trunk a bit before the uplift.
Risk to taking this patch (and alternatives if risky): Medium/High, it's touching the core of the tiling draw logic.
String or UUID changes made by this patch: none

Uplift note: This should be uplifted bug 1135677. I expect that this may need rebasing.

	Benoit Girard (:BenWa) Assignee
	Comment 21 • 9 years ago

non trivial rebase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=935825b56f38

	Benoit Girard (:BenWa) Assignee
	Comment 22 • 9 years ago

Comment on attachment 8568935 [details] [diff] [review]
patch v4

Push rebased on b2g 2.2:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dc319dc9051

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

Try push in comment 22 was missing bug 1135677 hence the C2 failure. Here's a new push which includes that patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cabe2bf6e565

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

The 2.1 rebase is showing a M-12 failure, but it's really a crash on shutdown *after* all the tests are done running. I'm not able to reproduce this locally because when I run it locally the emulator window sticks around instead of shutting down (i.e. it's non-trivial to reproduce what try is doing on a local machine).

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

Oh, maybe this isn't a problem after all. It looks like the exact same test timeout and crash shows up on M-12 on 2.1 trunk, it's just that TreeHerder doesn't flag it as a failure. No idea why that is, but this is likely a treeherder problem rather than a problem with this patch.

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

Confirmed that the M-12 failure is not a real problem by pushing the b2g34 tip cset to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c86aa57a238

The M-12 which is green (albeit with errors) on b2g34 is red on try, and so pushing these patches to 2.1 should be fine as the error is pre-existing.

	bhavana bajaj [:bajaj]
	Comment 27 • 9 years ago

Given the medium high risk here, I really want this to get baked and need QA verification before uplift. So flaggin :njpark here to get around this and help as soon as he can.

	No-Jun Park [:njpark]
	Comment 28 • 9 years ago

tested the patch on 2.1 and 2.2 (in conjunction with verifying the patch for Bug 1137203) and did not see any OOM failures or black screen during the screen transition

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

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3aefa50bb4b0
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/c7f5ed22de57

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

RyanVM pointed out crashtest failures on 2.1 desktop platforms, which is because the progressive-paint pref wasn't in all.js in 2.1 (it was only added there as part of bug 1072093 which is 36 and up. I added it manually:

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/871071010b5b

	Vincent Liu[:vliu]
	Comment 31 • 9 years ago

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/c7f5ed22de57