HTML-simulated play button doesn't render properly

VERIFIED FIXED in Firefox 45

Status

()

defect
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: kats, Assigned: bas.schouten)

Tracking

({regression})

45 Branch
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 unaffected, firefox45+ verified, firefox46 verified)

Details

(Whiteboard: [testday-20160219])

Attachments

(2 attachments)

Posted file 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.
[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?
Flags: needinfo?(bugmail.mozilla)
Not at the moment since I'm in the middle of stuff, but I can do it later today if you want me to.
Flags: needinfo?(bugmail.mozilla)
That would be awesome, thanks!
Assignee

Updated

4 years ago
Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
Assignee

Comment 6

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

Comment 7

4 years ago
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.
Attachment #8691254 - Flags: review?(matt.woodrow)
Duplicate of this bug: 1224824
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?
Attachment #8691254 - Flags: review?(matt.woodrow) → review+
Assignee

Comment 10

4 years ago
(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.
Blocks: 1226255

Comment 12

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b0760c35953
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla45 → ---
Assignee

Comment 15

4 years ago
(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...
Flags: needinfo?(philringnalda)
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.
Flags: needinfo?(philringnalda)
Assignee

Comment 17

4 years ago
(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.
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.
Assignee

Comment 19

4 years ago
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 21

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee4446a2c02a
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Bas, could you fill an uplift request to aurora? Thanks
Flags: needinfo?(bas)
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]:
Flags: needinfo?(bas)
Attachment #8691254 - Flags: approval-mozilla-aurora?
Blocks: 1237561
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.
Attachment #8691254 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Duplicate of this bug: 1226255
Duplicate of this bug: 1227615
Duplicate of this bug: 1237561
QA Whiteboard: [good first verify]
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
QA Whiteboard: [good first verify] → [good first verify][testday-20160219]
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
Status: RESOLVED → VERIFIED
Whiteboard: [testday-20160219]
You need to log in before you can comment on or make changes to this bug.