Closed Bug 1092842 Opened 5 years ago Closed 5 years ago

Windowed Flash is improperly clipped by content on youtube

Categories

(Core :: Plug-ins, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 + verified
firefox35 + verified
firefox36 + verified

People

(Reporter: jimm, Assigned: roc)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image youtube.png
STR:

1) load the test case url
2) wait until video loads, then scroll up

result: the plugin window isn't properly clipped by the white content header.

Last good revision: 82df3654cd80 (2014-07-23)
First bad revision: a91ec42d6a9c (2014-07-24)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82df3654cd80&tochange=a91ec42d6a9c
Flash Version: 15.0.0.189
bug 1041530 fixed a similar issue in glass, that fix landed on 2014-07-31. Looks like this is a regression from bug 1022612.

Also, the work in bug 1022612 landed on aurora 2014-07-24 and this regression is in that release as well.
Blocks: 1022612
I can reproduce on 34-36 on Windows 7. I cannot reproduce on OSX. As noted, this is an issue related to plug-ins and, as such, does not reproduce with every YouTube video. 

Tracking as this is a regression, which I would like to fix. However, this issue is not severe enough to hold the 34 release.
Roc - You're listed as the owner of bug 1022612, which Jim expects caused this regression. Can you take this one?
Flags: needinfo?(roc)
Attached file testcase
I think this testcase covers what we're seeing with Youtube.

The nsDisplayTransform for the DIV is not marked as opaque, and so we don't subtract it (or the opaque regions of any of its contents) from the visible region for the plugin.

nsDisplayTransform only returns an opaque region if its entire contents are opaque, and that doesn't happen because the bottom-border part isn't known to be opaque. Actually the background color of the DIV nominally covers its entire border-box, but we clip it to the padding area because we know the opaque border is going to be painted outside it.

I have no idea how this was working before.
Assignee: nobody → roc
Flags: needinfo?(roc)
Attached patch fix (obsolete) — Splinter Review
I've been trying to think of a reason why we need the display item cliprect to have the exclude-borders optimization, but I can't think of one.
Attachment #8517223 - Flags: review?(tnikkel)
Attachment #8517223 - Flags: review?(tnikkel) → review+
Reftests fail with this...
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a5fb4d7fc918

These tests fail on all platforms:
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/border-radius/corner-1.html | image comparison (==), max difference: 98, number of differing pixels: 56
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/border-radius/clipping-3.html | image comparison (==), max difference: 95, number of differing pixels: 70
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/456219-2.html | image comparison (==), max difference: 38, number of differing pixels: 156 
Probably should just have more fuzz. They all already have fuzz and visually the results are fine.

Mac has a lot stuff like this:
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/layout/reftests/backgrounds/vector/tall--cover--nonpercent-width-omitted-height-viewbox.html | image comparison (==), max difference: 1, number of differing pixels: 4
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/layout/reftests/backgrounds/vector/tall--cover--nonpercent-width-percent-height-viewbox.html | image comparison (==), max difference: 1, number of differing pixels: 4
Looks like a difference where we stroke a CSS border rectangle compared to the same thing drawn with SVG. Not sure what's up with that.

Windows scores an unexpected pass:
REFTEST TEST-UNEXPECTED-PASS | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/border-radius/curved-border-background-nogap.html | image comparison (==)

Need to think on this a bit.
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/456219-2.html | image comparison (==), max difference: 38, number of differing pixels: 156 

This one doesn't have fuzz and moreover, the test shows a visual regression where stray pixels appear at the outside of rounded corners.
That gets a lot of weird off-by-1 errors on the corners of rectangles, on Mac only.
Blocks: 1095930
Bug 1097437 has a patch that should work around the Mac issues I saw.
Depends on: 1097437
https://hg.mozilla.org/mozilla-central/rev/31fb338f26bf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attached patch fix v3Splinter Review
I just realized I forgot to get this re-reviewed before landing...
Attachment #8517223 - Attachment is obsolete: true
Attachment #8521908 - Flags: review?(tnikkel)
Comment on attachment 8521908 [details] [diff] [review]
fix v3

Approval Request Comment
[Feature/regressing bug #]: 1022612
[User impact if declined]: plugin incorrectly rendered above certain page content on Windows and Linux
[Describe test coverage new/current, TBPL]: there are some automated tests for this.
[Risks and why]: It regresses some sites including Youtube, seriously enough that they might try to work around it or users might complain. The patch just disables an optimization in a narrow set of cases.
[String/UUID change made/needed]: none

Note, if we take this then we also need to take the patch in bug 1097437 or tests will fail. Fortunately that patch is also low-risk.
Attachment #8521908 - Flags: approval-mozilla-beta?
Attachment #8521908 - Flags: approval-mozilla-aurora?
Attachment #8521908 - Flags: review?(tnikkel) → review+
Comment on attachment 8521908 [details] [diff] [review]
fix v3

Taking this in 34 beta9 as it is a visible regression on YouTube. I have asked QE to verify. Beta+ Aurora+
Attachment #8521908 - Flags: approval-mozilla-beta?
Attachment #8521908 - Flags: approval-mozilla-beta+
Attachment #8521908 - Flags: approval-mozilla-aurora?
Attachment #8521908 - Flags: approval-mozilla-aurora+
I just landed the **reftest manifest changes only** from bug 1096181 on aurora and beta, because those changes actually belonged in either bug 1097437 or this bug, which were just landed in both places:

https://hg.mozilla.org/releases/mozilla-aurora/rev/187e9fa68b67
https://hg.mozilla.org/releases/mozilla-beta/rev/c486cd17bebb
Flags: qe-verify+
I was able to reproduce the issue with the YouTube URL on Windows 7 x64, with Firefox 34 Beta 8 (Flash Version: 15.0.0.189). The issue no longer reproduces with: 
- Firefox 34 Beta 9 - BuildID=20141113192814
- latest Firefox 36 Nightly - BuildID=20141113030201
Status: RESOLVED → VERIFIED
Also fixed on Windows 7 x64 with latest Firefox 35 Aurora (BuildID=20141114004002).
This patch broke bug 1099445.  can we get this backed out of 2.1 branch?  ni? roc for help.
Flags: needinfo?(roc)
We could, as an emergency measure, but I think it's important to figure out bug 1099445 because it might be a deeper bug that just got exposed.
Flags: needinfo?(roc)
You need to log in before you can comment on or make changes to this bug.