1290505 - [e10s] Can't see video on Citytv.com due to overlay

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Adam Stevenson [:adamopenweb]]

Attachment: citytv.jpg

This bug is originally from webcompat.com https://webcompat.com/issues/2861

STR:
0) Enable flash and use a Canadian ip address
1) Open Firefox Dev Edition 49 or Nightly 50 with multi-process enabled
2) Navigate to http://www.citytv.com/toronto/?video=5049399533001

Expected behavior:
Video starts playing

Actual behavior:
Video plays in background, can hear audio

There is an overlay covering up the video window. If I open this website in a non-e10s window, the reported issue reproduces. But if I first disable multi-process in the preferences menu, the issue does not reproduce.

When I uncheck this property in dev tools:

.bcc-dialog .bcc-dialog-body{
   position: relative;
}

The overlay disappears and you can see the video.

Comment 1

[Barbara Bermes [:barbara] - NI please!]

Dev Edition 49
-------------------
with e10s: video doesn't play
without e10: video plays

GA 47: video plays.

I'm worried this is an e10s issue?

Comment 2

[Daniel Holbert [:dholbert]]

CC'ing Canadian layout-hackers botond, kats, & tnikkel.

(In reply to Adam Stevenson from comment #0)
> 1) Open Firefox Dev Edition 49 or Nightly 50 with multi-process enabled

Do we know yet whether this is a regression? (i.e. will users who get Firefox-48-with-e10s next week be affected by this bug?) (I'm not sure whether it's easy to configure/ensure e10s enabledness in Firefox 48 beta, but you can grab & test a late 48 DevEdition build here to see if 48 is affected: https://archive.mozilla.org/pub/firefox/nightly/2016/06/2016-06-05-00-40-13-mozilla-aurora/ )

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I can reproduce this on Nightly. Downloading beta, on which I'll force enable e10s via browser.tabs.remote.force-enable, and test there.

Comment 4

[Timothy Nikkel (:tnikkel)]

I have reproduced on 2015-08-01 nightly (with e10s) so far. Going further back...

Comment 5

[Adam Stevenson [:adamopenweb]]

Daniel, I tested with firefox-48.0a2.en-US.mac.dmg and I'm able to reproduce the issue.

With e10s - doesn't play
without e10s - plays

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I have reproduced this on 49 Beta 7.

This seems like kind of a big deal. :(

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Requesting tracking, since video playback in Flash appears to be broken for this particular case in e10s mode.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

I put in the wrong version for comment 6.

I've reproduced this on 48 beta 7.

Comment 9

[Timothy Nikkel (:tnikkel)]

Regression range
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=34e2d2bd7ec4&tochange=a6bbabebed2f

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Jim, you've got some plugin related patches in that range. Bob, you've got some NPAPI sandbox patches

Comment 11

[Timothy Nikkel (:tnikkel)]

I tested on Windows and I didn't see the bug, so this is likely mac only.

Reverting the patch for bug 1121811 (even though the code has changed a little since then) fixes the bug for me.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

sounds like bob is off the hook then

Comment 13

[Benoit Girard (:BenWa)]

If I had to guess we might be setting the plugin window size slightly differently in e10s and the plugin is way to strict (otherwise we'd like have seen this show up before).

Let me have a look.

Comment 14

[Benoit Girard (:BenWa)]

(In reply to Timothy Nikkel (:tnikkel) from comment #11)
> Reverting the patch for bug 1121811 (even though the code has changed a
> little since then) fixes the bug for me.

Do you have the patch that you used for reference? It's far more than a trivial rebase now.

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: what i used to revert patch

This is what I used, it's hacky.

Comment 16

[Benoit Girard (:BenWa)]

Alright I was looking at the other patch. I'm still trying to understand why this specific case only under e10s is so strict.

Comment 17

[Benoit Girard (:BenWa)]

Alright some progress:
That patch just spams SetWindow against the plugin. Certainly not a patch we want to take.

Basically we end up this:
set plugin window 149, -9858, 675, 381
set plugin window 149, -9858, 675, 381
set plugin window 149, 141, 675, 381 <-- on screen
set plugin window 149, 141, 675, 381

I'm not sure what the -9858 is about, could be the page doing something.

The problem is without spamming the window set we don't set the window when the value changes to 141 properly:
set plugin window 0, 0, 675, 381
set plugin window 149, -9858, 675, 381
set plugin window 0, 0, 675, 381
set plugin window 149, -9858, 675, 381 <-- Last set window is wrong

We probably don't trigger an window update when we should. It's probably something e10s specific as well.

Comment 18

[Benoit Girard (:BenWa)]

I've found the problem and I have a candidate solution. Verifying a few things before I post the patch.

Comment 19

[Benoit Girard (:BenWa)]

Attachment: Bug 1290505 - Remove E10S NPAPI plugin windows special case.

Review commit: https://reviewboard.mozilla.org/r/68026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68026/

Comment 20

[Benoit Girard (:BenWa)]

This removes an optimization that rode along with bug 1009148 that I initially wrote.

We might be able to find a better fix to preserve this optimization but AFAIK this optimization never produced any measurable win. It also was causing our plugin code to behave differently with e10s and non-e10s. In the interest of minimizing e10s risk this is probably the best change. But I'm open to exploring how to preserve this optimization properly.

We will want to make sure we get some good coverage of this across platforms. We could limit this chance to OSX only but this change was intended as an OSX optimization so it feels odd to leave it in for other platforms.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 8776121 [details]
Bug 1290505 - Remove E10S NPAPI plugin windows special case.

https://reviewboard.mozilla.org/r/68026/#review65246

Comment 22

[Jim Mathies [:jimm]]

(In reply to Benoit Girard (:BenWa) from comment #20)
> This removes an optimization that rode along with bug 1009148 that I
> initially wrote.
> 
> We might be able to find a better fix to preserve this optimization but
> AFAIK this optimization never produced any measurable win. It also was
> causing our plugin code to behave differently with e10s and non-e10s. In the
> interest of minimizing e10s risk this is probably the best change. But I'm
> open to exploring how to preserve this optimization properly.
> 
> We will want to make sure we get some good coverage of this across
> platforms. We could limit this chance to OSX only but this change was
> intended as an OSX optimization so it feels odd to leave it in for other
> platforms.

Originally plugin window clipRect was mac only [1], but it looks like we started working with it in our gtk test plugin at some point. It isn't in use on Windows.

Regardless.. this particular area of nsPluginInstanceOwner where we modify it (FixUpPluginWindow [2]) is ifdef'd to mac only so we should be fine on other platforms.

[1] http://searchfox.org/mozilla-central/rev/1112b7a5222b71a3b5b68bd531f50ded6bcbc770/dom/plugins/ipc/PluginInstanceParent.cpp#1363
[2] http://searchfox.org/mozilla-central/rev/1112b7a5222b71a3b5b68bd531f50ded6bcbc770/dom/plugins/base/nsPluginInstanceOwner.cpp#3448

Comment 23

[Benoit Girard (:BenWa)]

Great, let's run it through try and flag it for qa-wanted once it's on nightly to be ready for an uplift.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Mike considers that it is a big deal, tracking.

Now, we are going to ship 48 tomorrow. I don't think it is worth delaying the 48 release.
However, if this becomes something big, we can do a dot release with a fix before increasing the number of people having e10s = on.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/192c7f9edb1c
Remove E10S NPAPI plugin windows special case. r=jimm

Comment 26

[Mike Taylor [:miketaylr]]

(adding regression keyword so this appears on the Platform triage radar.)

Comment 27

[Barbara Bermes [:barbara] - NI please!]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Mike considers that it is a big deal, tracking.
> 
> Now, we are going to ship 48 tomorrow. I don't think it is worth delaying
> the 48 release.
> However, if this becomes something big, we can do a dot release with a fix
> before increasing the number of people having e10s = on.

From a product manager perspective, I agree, this is a big deal but not worth delaying 48. However we should get this fixed asap.

NI Erin so she is in the loop of this.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Snorp, I am putting back the affected because we want to keep that on our radar. If this bug affects many other websites, we might want to do a dot release before we enable e10s to more users.

Clearing the ni to Erin, I already mentioned this bug to her (cf comment 25.5)

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/192c7f9edb1c

Comment 30

[Benoit Girard (:BenWa)]

qawanted. It's a non-trivial change to the plug-in code and we might want to uplift this. Testing plugin on each platforms with e10s pages such at this one [1] http://www.citytv.com/toronto/?video=5049399533001 and Youtube and making sure the video is visible and that mouse events are sent correctly to the plugin (you can use the video scrubber/timeline control).

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Andrei, can you help with this one? Thanks
Benoit, could you fill the uplift request so that we are ready in case we want to uplift that fast? Thanks

Comment 32

[Benoit Girard (:BenWa)]

Comment on attachment 8776121 [details]
Bug 1290505 - Remove E10S NPAPI plugin windows special case.

Approval Request Comment
[Feature/regressing bug #]: bug 1009148
[User impact if declined]: Some broken Flash video
[Describe test coverage new/current, TreeHerder]: Limited flash video test coverage. Has been on nightly for a few days
[Risks and why]: Medium to high. We're patching the plug-in code which is a finicky beast but at least we're doing in a few limited area. I've flagged qa-wanted. This is something we want to keep an eye on.
[String/UUID change made/needed]: none

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776121 [details]
Bug 1290505 - Remove E10S NPAPI plugin windows special case.

E10s and Flash video playback, let's uplift to Aurora50 and stabilize before uplifting to Beta49/Release48. Aurora50+

Comment 34

[Ciprian Georgiu, Desktop QA]

I was able to reproduce this bug on 50.0a1 (2016-08-01) using Mac OS X 10.11 "El Capitan", with an Intel Iris Pro GPU (1536 MB) and flash enabled.

Unfortunately, I'm still seeing the bug on 51.0a1 (2016-08-04):
- it's reproducible _with and without e10s_ on my end
- it's reproducible even if I change the value of the "position" property for .bcc-dialog .bcc-dialog-body

Benoit, what's your take on this? Am I missing something?

Comment 35

[Carsten Book [:Tomcat]]

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

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8776121 [details]
Bug 1290505 - Remove E10S NPAPI plugin windows special case.

As comment #34 suggests it is not actually fixed, not taking it for beta for now.
Please resubmit when ready.

Comment 37

[Adam Stevenson [:adamopenweb]]

This is fixed for me using 50.0a2 (2016-08-07) and 51.0a1 (2016-08-07) on OSX. I have flash set as always activate when testing, ask to activate doesn't present a prompt.

Ciprian - the content is only available in Canada. Could that be the reason?

Comment 38

[Mike Taylor [:miketaylr]]

Attachment: Working in Nightly 51

FWIW, I happen to be in Toronto today and the video is working with Flash enabled for me in Nightly.

Note the old URL shows a message like: This video is no longer available. 
I tested with http://www.citytv.com/toronto/?video=5068122152001

Comment 39

[Ciprian Georgiu, Desktop QA]

Attachment: stuck on buffering.png

Comment 40

[Ciprian Georgiu, Desktop QA]

(In reply to Adam Stevenson from comment #37)
> This is fixed for me using 50.0a2 (2016-08-07) and 51.0a1 (2016-08-07) on
> OSX. I have flash set as always activate when testing, ask to activate
> doesn't present a prompt.
> 
> Ciprian - the content is only available in Canada. Could that be the reason?

I do not think this is the reason. I've used a proxy from Canada when I first reproduce the bug, but after the bug was fixed the video or sound is not playing anymore. (it's stuck on buffering)
Also the Flash is set to "Always Activate".

Any ideas Adam? Am I missing something?

Comment 41

[Daniel Holbert [:dholbert]]

Given Ciprian's caveats in comment 34 & comment 40 (the issue being buffering-related, the site being broken with/without e10s, and broken with/without the overlay hidden), I suspect he's seeing a *different bug*.  (and one which doesn't reproduce for people actually in Canada yet -- possibly related to some flash traffic bypassing his proxy, maybe?)

Ciprian: do you see the same broken behavior with the *previous* release of Firefox? (47)  If you get similarly-broken results in Firefox 47, then we should definitely consider your experience to be a separate bug.

Comment 42

[Ciprian Georgiu, Desktop QA]

(In reply to Daniel Holbert [:dholbert] 
> Ciprian: do you see the same broken behavior with the *previous* release of
> Firefox? (47)  If you get similarly-broken results in Firefox 47, then we
> should definitely consider your experience to be a separate bug.

Yes I see the same behavior on Firefox 47. Should I log a separate bug for this problem, Daniel?

Comment 43

[Daniel Holbert [:dholbert]]

Probably best to file a separate bug for that, then -- yeah.  (Though depending on your proxy setup, your issue might not be a Firefox-specific problem at all, but rather the proxy being insufficient.)

In any case, it seems we've established that the issue Ciprian is hitting is *not* a (recent) regression and is distinct from the issue reported/fixed in this bug.  So let's consider this VERIFIED after all per comment 38, and let's get this uplifted to beta.

(In reply to Sylvestre Ledru [:sylvestre] from comment #36)
> Comment on attachment 8776121 [details]
> As comment #34 suggests it is not actually fixed, not taking it for beta for now.

(Please retriage, sylvestre, since we've determined that Ciprian's "not-fixed" report was a distinct, non-regression, possibly-proxy-dependent bug.)

Comment 44

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8776121 [details]
Bug 1290505 - Remove E10S NPAPI plugin windows special case.

ok, let's do it!

Comment 45

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

https://hg.mozilla.org/releases/mozilla-beta/rev/39ad687dbbae

Comment 46

[Sylvestre Ledru [:Sylvestre]]

Too risky for 48, let it ride the train from 49

Comment 47

[Ciprian Georgiu, Desktop QA]

I verified again this issue on 49.0-build2 (2016-09-07) and 50.0a2 latest Aurora (2016-09-12), using Mac OS X 10.11 with a different proxy plugin.

I can confirm that this issue is verified fixed, on the Firefox builds mentioned above. Changing the flags accordingly.