1092842 - Windowed Flash is improperly clipped by content on youtube

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Attachment: 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

Comment 1

[Jim Mathies [:jimm]]

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.

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

Roc - You're listed as the owner of bug 1022612, which Jim expects caused this regression. Can you take this one?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Different approach: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8001bce5a078

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That gets a lot of weird off-by-1 errors on the corners of rectangles, on Mac only.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 1097437 has a patch that should work around the Mac issues I saw.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31fb338f26bf

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/31fb338f26bf

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

I just realized I forgot to get this re-reviewed before landing...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7155bda88517
https://hg.mozilla.org/releases/mozilla-beta/rev/19296c34b1ca

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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

Comment 19

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Also fixed on Windows 7 x64 with latest Firefox 35 Aurora (BuildID=20141114004002).

Comment 20

[Tony Chung [:tchung]]

This patch broke bug 1099445.  can we get this backed out of 2.1 branch?  ni? roc for help.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.