Closed Bug 1305135 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Plug-ins, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 + fixed
firefox50 + wontfix
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Keywords: flashplayer, inputmethod, jp-critical)

Attachments

(2 files, 1 obsolete file)

[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.
Assignee: nobody → jmathies
Attachment #8794387 - Flags: review?(milan)
I think we're going to want this in a 49 point release.
Flags: needinfo?(lhenry)
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).
Attachment #8794387 - Flags: review?(milan) → review+
Pushed by jmathies@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43435bf2448b
Enable async drawing support on release channels. r=milan
(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 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
Attachment #8794387 - Flags: approval-mozilla-beta?
Attachment #8794387 - Flags: approval-mozilla-aurora?
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)
Attachment #8794387 - Flags: approval-mozilla-beta?
Attachment #8794387 - Flags: approval-mozilla-beta+
Attachment #8794387 - Flags: approval-mozilla-aurora?
(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.
s/MOZ_RELEASE/RELEASE_BUILD
https://hg.mozilla.org/mozilla-central/rev/43435bf2448b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
The next beta build with this will be out Monday, 9/26.
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?)
Flags: needinfo?(lhenry)
Flags: needinfo?(jmathies)
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
(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.
Changing dom.ipc.plugins.asyncdrawing.enabled to true fixed the IME issue.
(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?
Flags: needinfo?(jmathies) → needinfo?(VYV03354)
(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.
(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.)
Flags: needinfo?(VYV03354)
Blocks: 1305595
(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.
(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.
(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.
Blocks: 1305526
The pref flip landed on release for 49.0.2 over in bug 1307108.
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(wkocher)
Flags: needinfo?(jmathies)
Resolution: FIXED → ---
Per bug 1312528, async rendering should be enabled on Win64.
(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.
Flags: needinfo?(jmathies)
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
(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.
Attached patch nightly backout patch (obsolete) — Splinter Review
Attachment #8804818 - Flags: review?(benjamin)
Comment on attachment 8804818 [details] [diff] [review]
nightly backout patch

sorry, obsolete patch
Attachment #8804818 - Flags: review?(benjamin)
Attachment #8804818 - Attachment is obsolete: true
Attachment #8804820 - Flags: review?(benjamin)
(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
Ni?me to backout on aurora tomorrow when I'm off PTO.
Flags: needinfo?(wkocher)
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.
Flags: needinfo?(wkocher)
Keywords: leave-open
(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.
(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
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.
Flags: needinfo?(twalker)
Attachment #8804820 - Flags: review?(benjamin) → review+
Attachment #8794387 - Flags: checkin+
The patch 'nightly backout patch' needs to land on mc, that's it.
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1da68ee13f25
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1315997
(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.
Flags: needinfo?(twalker)
Depends on: 1323413
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.