1227210 - HTML-simulated play button doesn't render properly

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Test case

This appears to be a regression in Fx 45. The attached test page shows a "play" button simulated in HTML/CSS in Fx 44 and earlier. In 45 though it doesn't render properly. I tested in both desktop (OS X) and Fennec; in both cases it renders fine in the latest Aurora build (Nov 23) but doesn't in the latest Nightly.

Comment 1

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

This is a fairly recent regression; I see the bug in today's nightly but a Nov 5 nightly does not have the bug.

kats, are you hunting down the regression range?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Not at the moment since I'm in the middle of stuff, but I can do it later today if you want me to.

Comment 3

[Boris Zbarsky [:bzbarsky]]

That would be awesome, thanks!

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc473fe5dc512c450634506f68cbacfb40a06a23&tochange=3cc3b1968524248450c465c4ea2ee5596ffa65f2

Going through inbounds now...

Comment 5

[Kartikaya Gupta (email:[e-mail])]

mozregression found a winner!

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1e5f3d1151d60a1edd6424a35a2e38b5ab17adad&tochange=af5c9cf6898b0e2931a809774387cad5953363c3

Comment 6

[Bas Schouten (:bas.schouten)]

This somehow has to do with stacking of device offsets, I'm working on the solution.

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Ensure the mask and the surface are in the right space when being blended

This was being caused by GetMaskData returning a maskTransform that was from mask space to device space. While the PushGroupForBlendBack API assumes the maskTransform is in userspace.

While testing I found out this was also broken in a different way if non-operator OVER was used, this patch also corrects that case.

It would be nice if we had a reftest for this but I wasn't easily able to contrive one.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8691254 [details] [diff] [review]
Ensure the mask and the surface are in the right space when being blended

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

::: gfx/thebes/gfxContext.cpp
@@ +996,5 @@
>    CompositionOp oldOp = GetOp();
>    SetOp(CompositionOp::OP_OVER);
>  
>    if (mask) {
> +    if (false && !maskTransform.HasNonTranslation()) {

I assume this whole file is just debugging changes that aren't meant to be in this patch?

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> Comment on attachment 8691254 [details] [diff] [review]
> Ensure the mask and the surface are in the right space when being blended
> 
> Review of attachment 8691254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxContext.cpp
> @@ +996,5 @@
> >    CompositionOp oldOp = GetOp();
> >    SetOp(CompositionOp::OP_OVER);
> >  
> >    if (mask) {
> > +    if (false && !maskTransform.HasNonTranslation()) {
> 
> I assume this whole file is just debugging changes that aren't meant to be
> in this patch?

Heh, yeah, I was confirming all the 'fallback' codepaths worked correctly, thanks for noting.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0760c35953

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b0760c35953

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/52d0c4ad8be5 - judging by the frequency while I was retriggering looking for the cause, that gave us somewhere between a 10% and 40% shutdown crash on Android reftest-15 like https://treeherder.mozilla.org/logviewer.html#?job_id=17950621&repo=mozilla-inbound

Comment 14

[Phil Ringnalda (:philor)]

Or on the other flavor of Android, much less frequently, https://treeherder.mozilla.org/logviewer.html#?job_id=17941347&repo=mozilla-inbound

Comment 15

[Bas Schouten (:bas.schouten)]

(In reply to Phil Ringnalda (:philor) from comment #14)
> Or on the other flavor of Android, much less frequently,
> https://treeherder.mozilla.org/logviewer.html#?job_id=17941347&repo=mozilla-
> inbound

Are you really certain about this patch having been the cause? Those bits of code really aren't related.. it really seems quite unlikely that this patch was the cause...

Comment 16

[Phil Ringnalda (:philor)]

I don't know of any way to be really certain, but https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&tochange=1b0760c35953&filter-searchStr=082a9c22a7d2e185f28762c15229863611debf25&fromchange=b88dbbba1287 it happened 1-in-10 on your push, 0-in-50 just below it, it stopped on m-i when I backed you out but continued on other trees until the backout was merged around and has stopped on other trees since it was merged around.

Comment 17

[Bas Schouten (:bas.schouten)]

(In reply to Phil Ringnalda (:philor) from comment #16)
> I don't know of any way to be really certain, but
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&tochange=1b0760c35953&filter-
> searchStr=082a9c22a7d2e185f28762c15229863611debf25&fromchange=b88dbbba1287
> it happened 1-in-10 on your push, 0-in-50 just below it, it stopped on m-i
> when I backed you out but continued on other trees until the backout was
> merged around and has stopped on other trees since it was merged around.

That's bizar.. I'll ask Jamie to see if he can see it locally.

Comment 18

[Jamie Nicol [:jnicol]]

I have been unable to reproduce locally on Android 5.1 devices, and unfortunately was unable to get reftests to run at all locally on a gingerbread device.

Comment 19

[Bas Schouten (:bas.schouten)]

I've pushed this to try again and ran the test over 15 times and didn't ee a single failure.. Since in addition to that I also can't see any connection between this patch ad the aforementioned crash, I'm going to push again and hope for the best.

Comment 20

[Pulsebot]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Bas, could you fill an uplift request to aurora? Thanks

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 8691254 [details] [diff] [review]
Ensure the mask and the surface are in the right space when being blended

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: bad glitch on twitter.com
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low, simple patch, baked on central for a little while.
[String/UUID change made/needed]:

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8691254 [details] [diff] [review]
Ensure the mask and the surface are in the right space when being blended

Merci!
Obvious regression, taking it.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4a1b8b57f49

Comment 29

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 45.0a1 (Build ID: 20151123030237) on 
windows 8.1 64-bit with the instructions from comment 0.

Verified as fixed with latest Firefox beta 45.0b6 (Build ID: 20160215141016)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0

Verified as fixed with latest Firefox Aurora 46.0a2 (Build ID: 20160218004005)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0

Comment 30

[Sayed Mohammad Amir [:smamir]]

Reproduced the bug with Firefox Nightly 45.0a1 (20151123030237) on Ubuntu 14.04 LTS 64 Bit. 

Verified as fixed with latest Firefox Beta 45.0b7 (20160218171844)
UA: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0

Verified as fixed with latest Firefox Aurora 46.0a2 (20160219004008)
UA: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0