Closed Bug 1227210 Opened 5 years ago Closed 5 years ago

HTML-simulated play button doesn't render properly

Categories

(Core :: Layout, defect)

45 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox44 --- unaffected
firefox45 + verified
firefox46 --- verified

People

(Reporter: kats, Assigned: bas.schouten)

References

Details

(Keywords: regression, Whiteboard: [testday-20160219])

Attachments

(2 files)

Attached 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: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
This somehow has to do with stacking of device offsets, I'm working on the solution.
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+
(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: 1227615
https://hg.mozilla.org/mozilla-central/rev/1b0760c35953
Status: ASSIGNED → RESOLVED
Closed: 5 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 → ---
(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)
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/ee4446a2c02a
Status: REOPENED → RESOLVED
Closed: 5 years ago5 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+
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.