1305135 - Flash 23 isn't using async rendering mode on release channels

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
important new flash feature doesn't appear to be working on release channels.

Flash 23 should be leveraging async rendering mode and replacing windowed instances with windowless instances. AFAICT this is working on dev channels (nightly/aurora) but fails on release channels (beta/release).

I've confirmed the code we landed to support this (bug 1283274) is working on beta channel. I've also confirmed that a windowed flash instance is converted to windowless in dev channels, but on release channels I still detect a native window for windowed plugins.

Comment 1

[Jim Mathies [:jimm]]

This is disabled on release channels via prefs - 

http://searchfox.org/mozilla-central/rev/c31ad35f39c6187b2e121aa6d3a39b7f67397010/modules/libpref/init/all.js#2823

Comment 2

[Jim Mathies [:jimm]]

Attachment: enable async drawing support for plugins

Comment 3

[Jim Mathies [:jimm]]

I think we're going to want this in a 49 point release.

Comment 4

[Benjamin Smedberg]

It seems like we haven't baked this with beta. I'm skeptical that rushing this to release without beta baking is a good strategy.

We even have the opportunity to do a controlled A/B experiment on beta to measure the effects (hopefully the wins).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43435bf2448b
Enable async drawing support on release channels. r=milan

Comment 6

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> It seems like we haven't baked this with beta. I'm skeptical that rushing
> this to release without beta baking is a good strategy.

Yes true. :(

> We even have the opportunity to do a controlled A/B experiment on beta to
> measure the effects (hopefully the wins).

Do we need to hassle with setting up an experiment? We'll be able to compare data between beta builds once this lands. I assume you're interested in plugin hang rates?

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8794387 [details] [diff] [review]
enable async drawing support for plugins

Approval Request Comment
[Feature/regressing bug #]:
Enables a new flash feature in release channels.
[User impact if declined]:
n/a
[Describe test coverage new/current, TreeHerder]:
Patch only impacts release channels, dev channels have had this enabled for a couple months.
[Risks and why]: 
n/a
[String/UUID change made/needed]:
none

Comment 8

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

Comment on attachment 8794387 [details] [diff] [review]
enable async drawing support for plugins

This seems like something we'd want to consider in a dot release, let's stabilize this on Beta channel before we do that. Beta50+

(Clearning aurora nom as 51 is marked unaffected)

Comment 9

[Jim Mathies [:jimm]]

(In reply to Ritu Kothari (:ritu) from comment #8)
> Comment on attachment 8794387 [details] [diff] [review]
> enable async drawing support for plugins
> 
> This seems like something we'd want to consider in a dot release, let's
> stabilize this on Beta channel before we do that. Beta50+
> 
> (Clearning aurora nom as 51 is marked unaffected)

FYI, it doesn't impact aurora because the code is ifdef'd to MOZ_RELEASE. I'll still need to land it there so 51 has the change.

Comment 10

[Jim Mathies [:jimm]]

s/MOZ_RELEASE/RELEASE_BUILD

Comment 11

[Jim Mathies [:jimm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1fee8de1111fdcc6fb5d95d4afef92c913fc87f2
https://hg.mozilla.org/releases/mozilla-beta/rev/2663d882602b065945d483a206bb3a39f46cc479

Comment 12

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/43435bf2448b

Comment 13

[Jim Mathies [:jimm]]

The next beta build with this will be out Monday, 9/26.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 49. Is this something we could check in beta this week, then if we think it safe, fix with a pref flip/hotfix on release?  Benjamin, how much bake time would you accept? Do we need an entire beta cycle?  What kinds of issues other the Flash crashing do you think might result (i.e. what should we be looking for in beta?)

Comment 15

[Benjamin Smedberg]

I would really prefer to put this on beta and ride to 50 release. We believe that this will improve Flash performance and crash rates, but this is not a small change. Possible issues could include:

* performance problems especially with games, video, or other "high-performance" Flash
* functional regressions, especially around IMEs, maybe accessibility
* new crash issues which could be specific to particular graphics cards/drivers/driver versions.

Although I'm excited about the possible benefits, I don't think the extra 5-6 weeks is a big deal.

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #15)
> * functional regressions, especially around IMEs, maybe accessibility

Note that the current 49 already regressed around IMEs. Firefox 49 does not allow users to input Japanese on windowed Flash apps.

Comment 17

[Masatoshi Kimura [:emk]]

Changing dom.ipc.plugins.asyncdrawing.enabled to true fixed the IME issue.

Comment 18

[Jim Mathies [:jimm]]

(In reply to Masatoshi Kimura [:emk] from comment #16)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #15)
> > * functional regressions, especially around IMEs, maybe accessibility
> 
> Note that the current 49 already regressed around IMEs. Firefox 49 does not
> allow users to input Japanese on windowed Flash apps.

Is there an open bug on this?

Comment 19

[Jim Mathies [:jimm]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Tracking for 49. Is this something we could check in beta this week, then if
> we think it safe, fix with a pref flip/hotfix on release?  

No, this needs a bit of bake time.

> Benjamin, how
> much bake time would you accept? Do we need an entire beta cycle?  What
> kinds of issues other the Flash crashing do you think might result (i.e.
> what should we be looking for in beta?)

We're going to get this into beta and see how things go. We'll make this decision later.

Comment 20

[Masatoshi Kimura [:emk]]

(In reply to Jim Mathies [:jimm] from comment #18)
> (In reply to Masatoshi Kimura [:emk] from comment #16)
> > (In reply to Benjamin Smedberg [:bsmedberg] from comment #15)
> > > * functional regressions, especially around IMEs, maybe accessibility
> > 
> > Note that the current 49 already regressed around IMEs. Firefox 49 does not
> > allow users to input Japanese on windowed Flash apps.
> 
> Is there an open bug on this?

This bug :) (Flipping dom.ipc.plugins.asyncdrawing.enabled will fix the issue.)

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #20)
> (In reply to Jim Mathies [:jimm] from comment #18)
> > (In reply to Masatoshi Kimura [:emk] from comment #16)
> > > (In reply to Benjamin Smedberg [:bsmedberg] from comment #15)
> > > > * functional regressions, especially around IMEs, maybe accessibility
> > > 
> > > Note that the current 49 already regressed around IMEs. Firefox 49 does not
> > > allow users to input Japanese on windowed Flash apps.
> > 
> > Is there an open bug on this?
> 
> This bug :) (Flipping dom.ipc.plugins.asyncdrawing.enabled will fix the
> issue.)

Filed bug 1305595 anyway.

Comment 22

[Makoto Kato [:m_kato]]

(In reply to Masatoshi Kimura [:emk] from comment #16)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #15)
> > * functional regressions, especially around IMEs, maybe accessibility
> 
> Note that the current 49 already regressed around IMEs. Firefox 49 does not
> allow users to input Japanese on windowed Flash apps.

It isn't Firefox issue.  That is bug 1301486.  I am waiting for Adobe's fix.

Comment 23

[Masatoshi Kimura [:emk]]

(In reply to Makoto Kato [:m_kato] from comment #22)
> (In reply to Masatoshi Kimura [:emk] from comment #16)
> > (In reply to Benjamin Smedberg [:bsmedberg] from comment #15)
> > > * functional regressions, especially around IMEs, maybe accessibility
> > 
> > Note that the current 49 already regressed around IMEs. Firefox 49 does not
> > allow users to input Japanese on windowed Flash apps.
> 
> It isn't Firefox issue.  That is bug 1301486.  I am waiting for Adobe's fix.

Ah, I forgot that 32-bit Firefox did not tighten the sandbox yet. On 64-bit Firefox, IMEs are not enabled at all. That's bug 1305595 I filed.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

The pref flip landed on release for 49.0.2 over in bug 1307108.

Comment 25

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

Based on the recent regressions that this pref change has caused on Release 49, the team has made a decision to disable this perf in Beta50. Reopening this bug to make sure we back out this patch before I gtb 50.0b10 this afternoon.

Comment 26

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

Here's the backout on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/ebded599175d

Comment 27

[Masatoshi Kimura [:emk]]

Per bug 1312528, async rendering should be enabled on Win64.

Comment 28

[Jim Mathies [:jimm]]

(In reply to Masatoshi Kimura [:emk] from comment #27)
> Per bug 1312528, async rendering should be enabled on Win64.

We're taking it out of 64-bit fx50, leaving it in 64-bit fx49 builds since we can't uplift the force windowless patch to release.

Comment 29

[Andrew Gurskiy]

Hi.

I installed async_rendering.signed.xpi (https://bugzilla.mozilla.org/show_bug.cgi?id=1312528) and default value for dom.ipc.plugins.asyncdrawing.enabled changed to false. But  I still have issue with game performance on non debug version of Flash player version 23.0.0.205.

After restarting browser and computer I have the same result.

You can check by yourself - https://apps.facebook.com/slotomania

My FF version is 49.0.2 (32 bit) platform Win 10(64 bit)

Best regards,
Slototmania TL 
Andrew Gurskiy

Comment 30

[Jim Mathies [:jimm]]

(In reply to Wes Kocher (:KWierso) from comment #26)
> Here's the backout on beta:
> https://hg.mozilla.org/releases/mozilla-beta/rev/ebded599175d

This same changeset needs to be backed out of aurora 51. We'll need a new patch for nightly since the defines we use here changed. I'll work up a nightly backout patch.

Comment 31

[Jim Mathies [:jimm]]

Attachment: nightly backout patch

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 8804818 [details] [diff] [review]
nightly backout patch

sorry, obsolete patch

Comment 33

[Jim Mathies [:jimm]]

Attachment: nightly backout patch

Comment 34

[Andrew Gurskiy]

(In reply to Jim Mathies [:jimm] from comment #33)
> Created attachment 8804820 [details] [diff] [review]
> nightly backout patch

I checked this parameter (dom.ipc.plugins.asyncdrawing.enabled) from my side and it was set to false by default.

My FF version is 49.0.2(32 bit), Flash player non debug 23.0.0.205 version, platform win10.

Bug with game performance still reproduce. Could you please use our application https://apps.facebook.com/slotomania for testing the fix?

Also other Playtika`s games are affected.

Best regards,
Playtika, Slotomania TL
Andrew Gurskiy

Comment 35

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

Ni?me to backout on aurora tomorrow when I'm off PTO.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

Not sure if this should be reopened or WONTFIX at this point. I'm assuming we'll track re-enabling in another bug.

Comment 37

[Jim Mathies [:jimm]]

(In reply to Andrew Gurskiy from comment #34)
> (In reply to Jim Mathies [:jimm] from comment #33)
> > Created attachment 8804820 [details] [diff] [review]
> > nightly backout patch
> 
> I checked this parameter (dom.ipc.plugins.asyncdrawing.enabled) from my side
> and it was set to false by default.
> 
> My FF version is 49.0.2(32 bit), Flash player non debug 23.0.0.205 version,
> platform win10.
> 
> Bug with game performance still reproduce. Could you please use our
> application https://apps.facebook.com/slotomania for testing the fix?
> 
> Also other Playtika`s games are affected.
> 
> Best regards,
> Playtika, Slotomania TL
> Andrew Gurskiy

Did you restart the browser after flipping that switch before testing? I don't see any issues with this slots game with the pref off.

Comment 38

[Andrew Gurskiy]

(In reply to Jim Mathies [:jimm] from comment #37)
> (In reply to Andrew Gurskiy from comment #34)
> > (In reply to Jim Mathies [:jimm] from comment #33)
> > > Created attachment 8804820 [details] [diff] [review]
> > > nightly backout patch
> > 
> > I checked this parameter (dom.ipc.plugins.asyncdrawing.enabled) from my side
> > and it was set to false by default.
> > 
> > My FF version is 49.0.2(32 bit), Flash player non debug 23.0.0.205 version,
> > platform win10.
> > 
> > Bug with game performance still reproduce. Could you please use our
> > application https://apps.facebook.com/slotomania for testing the fix?
> > 
> > Also other Playtika`s games are affected.
> > 
> > Best regards,
> > Playtika, Slotomania TL
> > Andrew Gurskiy
> 
> Did you restart the browser after flipping that switch before testing? I
> don't see any issues with this slots game with the pref off.

Of course.
We see that on good desktop issue does not reproduce. But a lot of users have weaker PC. I am tested on Lenovo laptop ThinkPad T440p, T450, T460. Issue reproduced every time.

Also we found that issue reproduces when CPU load of flash plugin container on maximum level. At this moment animation continue playing but browser has stuck.

Best regards,
Playtika, Slotomania TL
Andrew Gurskiy

Comment 39

[Jim Mathies [:jimm]]

Tracy, would you please test the site mentioned in comment 34 in 49.0.2 and flash 23 with the async pref turned off. See if you can reproduce some of these lasting issues Andrew is seeing.

Also, could you please file a fresh bug for the issues Andrew is seeing on this fb site, which should block bug 1229961.

Comment 40

[Jim Mathies [:jimm]]

The patch 'nightly backout patch' needs to land on mc, that's it.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da68ee13f25
Disable async rendering on Nightly. Required a modification due to changes in build defines. r=bsmedberg

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1da68ee13f25

Comment 43

[Tracy Walker [:tracy]]

(In reply to Jim Mathies [:jimm] from comment #39)
> Tracy, would you please test the site mentioned in comment 34 in 49.0.2 and
> flash 23 with the async pref turned off. See if you can reproduce some of
> these lasting issues Andrew is seeing.
> 
> Also, could you please file a fresh bug for the issues Andrew is seeing on
> this fb site, which should block bug 1229961.

filed bug 1315997 for increase memory usage in flash games while in asyncdrawing mode.