1073554 - Visible seams in webpages while panning

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Attachment: SC20140926-170240.jpg

Galaxy Tab 10.1, Android 3.2, current m-c, regression since about a month (sorry for not reporting earlier, but there was so much gfx breakage on Android that it was impossible to tell that this is a separate problem).

1) Visit http://www.pianoworld.com/forum/ubbthreads.php/forum_summary.html
2) Pan up and down the entire page a few times
3) Observe visible blue or yellow horizontal seams in the page (see screenshot)

I bisected this to:

The first bad revision is:
changeset:   202105:abcdfdab902b
user:        Matt Woodrow <[e-mail]>
date:        Thu Aug 28 15:36:07 2014 +1200
summary:     Bug 1051134 - Revert changes to padding out textures that will be resampled for mobile. r=Bas

Which, when looking what it patches, does indeed look like a very likely regressor!

Comment 1

[Matt Woodrow (:mattwoodrow)]

That change just backed out a change from a week or so earlier, it shouldn't have created anything new.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> That change just backed out a change from a week or so earlier, it shouldn't
> have created anything new.

What's the original bug it reverted? I notice in the bug comments "I couldn't reproduce the original problem, and Jeff fixed it another way", so it seems your fix *did* work and Jeff's didn't :-)

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Okay, I see the related bugs are bug 1048110 and bug 1023473.

Whatever "fix for mobile" was made there was not complete.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]:

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Snorp said he thought this regressed in 34. So NI to him to test and report back

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can't reproduce it there, but I can't reliably reproduce it on 35/36 either.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Still reproduces. Verified with a try build from blassey that backing out bug 1023473 does not fix this. 

Backing out bug 1051134 which is itself a backout did fix this when I originally reported it, but apparently regresses performance, which means we can have our blue seams faster now.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

I suspect the original fix for this issue was this or in the changesets right after:
changeset:   198480:a287654390b1
user:        Matt Woodrow <[e-mail]>
date:        Mon Aug 04 15:29:55 2014 +1200
summary:     Bug 1048110 - Expand complex visible regions if we're going to be resampling when using tiling. r=Bas

Unfortunately those were also builds that were broken (Java exception on launch) on my hardware where this reproduces. Sigh.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

So for the case where backing out the backout isn't possible because of performance issues, I went to the place where the code that was backed out was added, which is this:

changeset:   198480:a287654390b1
user:        Matt Woodrow <[e-mail]>
date:        Mon Aug 04 15:29:55 2014 +1200
summary:     Bug 1048110 - Expand complex visible regions if we're going to be resampling when using tiling. r=Bas

Now at this place, we are in a broken (with seams) state. I went back further then, trying to find the place where this once-upon-a-time worked. I was expecting to find something like "enable tiled rendering", but the actual bisection pointed to something more interesting:

The first bad revision is:
changeset:   195415:7a5b3439faa7
user:        Jeff Muizelaar <[e-mail]>
date:        Fri Jul 18 14:25:34 2014 -0400
summary:     Bug 1023473. Pad out tile contents by one pixel. r=BenWa

But that directly contradicts the conclusion in comment 7!

Comment 10

[Gian-Carlo Pascutto [:gcp]]

After investigation, we found that the backout-bug-1023473 try build didn't actually back out the entire patch. So that explains all the above.

Conclusion: this is caused by bug 1023473.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Does backing out the patch on bug 1023473 cause bug 1023473 to regress or is it handled by bug 1048110 now?

Comment 12

[Aaron Train [:aaronmt]]

Is this still reproducible?

Comment 13

[Gian-Carlo Pascutto [:gcp]]

As far as I know, yes, I don't think anything landed since comment 10 that changes this.

Needinfo myself to test https://tbpl.mozilla.org/?tree=Try&rev=4893d9de121b (which backs out 1023473) when I get back from Portland.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Confirming that the try build with bug 1023473 backed out works correctly.

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

What is the step forward here?

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Verifying that the backout doesn't regress bug 1023473 itself. (See comment 11)

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Next week is the last beta for 35.0 so if a clean backout of bug 1023473 is possible (and doesn't regress the issue further) we should get an uplift of the backout asap for the Jan 5th beta.

Comment 18

[Aaron Train [:aaronmt]]

Someone else will need to do it, he is away till the 5th.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

We talked about this in IRC and in order to not ship this new regression, we should get the backout on 35 asap and take care of checking for regressions to FxOS separately - since they have different shipping dates where one (Fennec) is next week and the other has more time for repair if needed.  I believe Blassey can do the backout so we're ready for this afternoon's final mobile beta gtb.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

The patch from the Try run doesn't apply cleanly to beta anymore. I'm going to punt on this for someone who can better ensure no accidental bustage on something this close to release.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Brad landed the backout on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/7033680d2fc4

I merged it to release:
https://hg.mozilla.org/releases/mozilla-release/rev/7033680d2fc4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Wonderful, the backout push has Android reftest failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=7033680d2fc4&filter-searchStr=Android%20reftest

Comment 23

[Ryan VanderMeulen [:RyanVM]]

> REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/transform-3d/perspective-origin-1b.html | image comparison (==), max difference: 130, number of differing pixels: 70 

random-if(Android&&AndroidVersion==17) == perspective-origin-1b.html perspective-origin-1a.html

I guess we made it "random" on more than just version 17 now.

> REFTEST TEST-UNEXPECTED-FAIL | http://10.26.133.19:30555/tests/layout/reftests/transform-3d/rotatex-transformorigin-1a.html | image comparison (==), max difference: 80, number of differing pixels: 6 

reftest.list says we shouldn't have any Android issues with that test.

I'm not going to back this out as it's past bedtime for me, but I can't say I'm thrilled with the idea of this shipping as-is either.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

All this means is that test started being random on the emulator (in addition to also being random on the panda). We should just drop the android version conditional on the random if

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Who's going to take care of changing the test expectations if that's all that's intended to be done here? Leaving things broken indefinitely on release branches isn't OK.

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #25)
> Who's going to take care of changing the test expectations if that's all
> that's intended to be done here? Leaving things broken indefinitely on
> release branches isn't OK.

I'm going to handle it as well as backing out on aurora/central

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

blassey and I both tested beta with the backout applied and couldn't reproduce this anymore. Does anyone else have a different experience?

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

Just to be clear - you backed out bug 1023473 on Beta and the problem is gone?

Since this is a backout of a backout of a backout bug, please confirm what's being backed out when "backing out" is in the comment :)

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Milan Sreckovic [:milan] from comment #28)
> Just to be clear - you backed out bug 1023473 on Beta and the problem is
> gone?
> 
> Since this is a backout of a backout of a backout bug, please confirm what's
> being backed out when "backing out" is in the comment :)

This is the backout that was pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/7033680d2fc4

This does appear more or less the patch that is in bug 1023473.

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Actually I can reproduce this still even with the backout. STR:

1) Go to planet.mozilla.org
2) Zoom in all the way
3) pan to the right about half way
4) quickly scroll down
5) Observe horizontal and vertical black (dark gray?) lines

Comment 31

[Gian-Carlo Pascutto [:gcp]]

Clarified with snorp that his issue is while scrolling, mine is static corruption that stays.

I *cannot* reproduce it:
a) With the try build from comment 22.
b) With Nightly.
c) With current Beta from the Google Play store.

snorp believes it's possible his Visit* crash fixes have also fixed this bug.

Comment 32

[Jeff Muizelaar [:jrmuizel]]

I've filed a new issue about the seams around low res stuff when scrolling at bug 1118858.

I expect we should backout the backout of bug 1023473 as it doesn't seem to make a difference anymore and as it's more likely that snorps fix to the Visit* crash fixed this bug.

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

RyanVM is going to backout the backout on beta and release.

Comment 34

[James Willcox (:snorp) ([e-mail]) (he/him)]

Marking this fixed, per comments #32 and #33

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Err, that would be comments #31 and #32

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Backed out on beta. Will merge to release in a bit.
https://hg.mozilla.org/releases/mozilla-beta/rev/400119b1311a

Comment 37

[Benoit Girard (:BenWa)]

See also bug 1110229 for a similar (but likely with a different root cause) seeming bug described in Comment 30.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

And release:
https://hg.mozilla.org/releases/mozilla-release/rev/400119b1311a

Comment 39

[u421692]

Verified as fixed on Firefox 35 RC and Firefox 35 Beta 11 on Acer Iconia A500 (Android 3.2.1) with the STR from comment 0.

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Mihai, you changed the status flag for 36. I guess that is still fixed but did you verify it?

Comment 41

[u421692]

Verified as fixed on Firefox 36 Beta 1 and Aurora 37.0a2(2015-01-14) on Samsung Galaxy Tab 10.1(Android 4.0.4)