Closed Bug 1307108 Opened 5 years ago Closed 5 years ago

Enable async rendering by default on 49 using a system addon

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox49 blocking fixed
relnote-firefox --- 49+

People

(Reporter: benjamin, Assigned: bytesized)

References

(Depends on 1 open bug)

Details

(Whiteboard: [fce-active-legacy][go-faster-system-addon])

Attachments

(4 files, 3 obsolete files)

We accidentally didn't turn on async rendering in 49 (beta or release). We fixed that for 50 in bug 1305135 by flipping the pref.

But because we landed bug 1283274 into 49, we have a regression where win64 users are having functional issues.

There is also a separate functional regression caused by Adobe in bug 1301486 which would be fixed by enabling async rendering.

My recommendation after discussing with Brad is that we wait until Tuesday/Wednesday to get feedback from the beta channel, and if there are no serious blocker bugs, roll out a fix to release this Thursday (6-Oct). We can develop/sign/test the addon now.

Liz NI? because this is a change in plans: I'd like your approval for this before we do the engineering work. NI?ddurst for an engineering owner.
Flags: needinfo?(lhenry)
Flags: needinfo?(ddurst)
We talked about doing something similar in https://bugzilla.mozilla.org/show_bug.cgi?id=1305135#c14 and the following comments. Is the difference now that we know the problems are affecting more users? Or do you feel differently about the risk and the chance of causing new regressions in 49?
Flags: needinfo?(lhenry) → needinfo?(benjamin)
Yes, the situation has changed because there are multiple user-visible regressions.
Flags: needinfo?(benjamin)
Sounds like this also affects scrolling for e10s users + Flash, in bug 1305526.  So, as I understand it, in bug 1305135 we thought the pref flip might improve performance and also stability for Flash plugin crashes. But now we are looking at scrolling issues, plus IME users' ability to input text. 

Let's go forward with the system add-on. It seems good that we can potentially fix this without a dot release. Should it be limited to only roll out to particular platforms?
Windows is the only OS that has the async drawing API. So I don't think it matters whether we roll out to just windows or all OSes: whichever is easiest to develop/deploy/QA.
Banding together bytesized and felipe for this (felipe mentoring, bytesized owning).
Flags: needinfo?(ddurst)
Whiteboard: [fce-active]
Assignee: nobody → ksteuber
Attached file async_rendering.xpi (obsolete) —
Attachment #8798227 - Flags: review?(felipc)
Attachment #8798229 - Flags: review?(felipc)
You don't need the uninstall part because we don't need to revert this pref.
And two other changes, which are not strictly necessary, but will make it better:

please add this line to the install.rdf file:
http://searchfox.org/mozilla-central/source/browser/extensions/e10srollout/install.rdf.in#16

And when we did pref-flipping hotfixes in the past, we do an extra call to force a flush of the preferences file to the disk:
http://hg.mozilla.org/releases/firefox-hotfixes/file/tip/v20160128.01/bootstrap.js#l35
Attachment #8798227 - Flags: review?(felipc)
Attachment #8798229 - Flags: review?(felipc)
Comment on attachment 8798229 [details]
Bug 1307108 - Added system addon to enable async plugin rendering

https://reviewboard.mozilla.org/r/83746/#review82466

::: browser/extensions/asyncrendering/bootstrap.js:14
(Diff revision 1)
> +
> +const PREF_ASYNC_DRAWING_ENABLED = "dom.ipc.plugins.asyncdrawing.enabled";
> +
> +
> +function install() {
> +  Preferences.set(PREF_ASYNC_DRAWING_ENABLED, true);

I don't think we should be setting a user pref here. I'd much rather set the *default* pref at each startup(). Then you don't have to worry about flushing prefs to disk.

(new Preferences({defaultBranch: true})).set(PREF_ASYNC_DRAWING_ENABLED, true)
Attached file async_rendering.xpi (obsolete) —
Attachment #8798227 - Attachment is obsolete: true
Attachment #8798486 - Flags: review?(felipc)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #10)
> Comment on attachment 8798229 [details]
> Bug 1307108 - Added system addon to enable async plugin rendering
> 
> https://reviewboard.mozilla.org/r/83746/#review82466
> 
> ::: browser/extensions/asyncrendering/bootstrap.js:14
> (Diff revision 1)
> > +
> > +const PREF_ASYNC_DRAWING_ENABLED = "dom.ipc.plugins.asyncdrawing.enabled";
> > +
> > +
> > +function install() {
> > +  Preferences.set(PREF_ASYNC_DRAWING_ENABLED, true);
> 
> I don't think we should be setting a user pref here. I'd much rather set the
> *default* pref at each startup(). Then you don't have to worry about
> flushing prefs to disk.
> 
> (new Preferences({defaultBranch: true})).set(PREF_ASYNC_DRAWING_ENABLED,
> true)

In general I agree, but in this case a user-pref looked better for me because:

1- this pref has been flipped to true on 50, so whenever a user upgrade to 50 the user-set value will become the default

2- if a user on 49.0 receives the system add-on and later upgrades to 49.0.1, they will temporarily revert into the bug until the system add-on update is received again. Depending on the seriousness of the problem, this might be worth considering.

Benjamin, what do you think? If you think point #2 is not a big concern than I'm fine with the default pref method.
Flags: needinfo?(benjamin)
I thought that setting |<em:maxVersion>49.*</em:maxVersion>| meant that no update would be needed in the case you mention in point (2). Is that not correct?
Attached file async_rendering.xpi (obsolete) —
Oops. Forgot to revert the version value in the addon after testing on Nightly.
Attachment #8798486 - Attachment is obsolete: true
Attachment #8798486 - Flags: review?(felipc)
Attachment #8798499 - Flags: review?(felipc)
(In reply to Kirk Steuber [:bytesized] from comment #14)
> I thought that setting |<em:maxVersion>49.*</em:maxVersion>| meant that no
> update would be needed in the case you mention in point (2). Is that not
> correct?

49.* is correct. The problem is that system add-ons always revert to the built-in versions after a Firefox upgrade (intentionally..). So there's a period of time that it will revert to the original set, before receiving the update ping again.

(Yeah, it's weird and we need to mitigate this issue. Bug 1193411 can make it better but I don't know if it will be enough)
I'm not particularly worried about the 49.0.1 issue in this case. There are few enough users in that pathway that it's not a huge concern.
Flags: needinfo?(benjamin)
Comment on attachment 8798229 [details]
Bug 1307108 - Added system addon to enable async plugin rendering

https://reviewboard.mozilla.org/r/83746/#review82546

Thanks! r+ with the comment below addressed.

Once you attach the new xpi, please needinfo :jason asking him to sign it

::: browser/extensions/asyncrendering/bootstrap.js:13
(Diff revision 2)
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +
> +const PREF_ASYNC_DRAWING_ENABLED = "dom.ipc.plugins.asyncdrawing.enabled";
> +
> +function install() {
> +  enableAsyncDrawing();

There's no need to call this on install() too if it's called on startup(). Both of these functions are called on first run.
Attachment #8798229 - Flags: review?(felipc) → review+
Attached file async_rendering.xpi
Carrying forward r+
Attachment #8798499 - Attachment is obsolete: true
Attachment #8798499 - Flags: review?(felipc)
Attachment #8798519 - Flags: review+
@jason Could I get this XPI signed please?
Flags: needinfo?(jthomas)
Please see signed add-on.
Flags: needinfo?(jthomas)
Comment on attachment 8798229 [details]
Bug 1307108 - Added system addon to enable async plugin rendering

Approval Request Comment
[Feature/regressing bug #]:
Bug 1301486.
Fix problems associated with Bug 1283274.

[User impact if declined]:
Continuing functional regressions for win64 users.
Continuing functional regression where window is displayed at wrong position when inputting some CCJK characters.

[Describe test coverage new/current, TreeHerder]:
Manual testing of add-on and update process.

[Risks and why]: 
Low Risk. Simply flips a pref that is already flipped in Firefox 50.

[String/UUID change made/needed]:
None
Attachment #8798229 - Flags: approval-mozilla-release?
Depends on: 1309037
No longer depends on: 1309037
Attachment #8798229 - Flags: approval-mozilla-release?
Removing request for uplift approval until issues are resolved.
Hi Kirk, we do want to ship this to release - while testing did uncover a few issues, changing the pref should fix some critical regressions. Can you put back the uplift request? Thanks.
Flags: needinfo?(ksteuber)
Comment on attachment 8798229 [details]
Bug 1307108 - Added system addon to enable async plugin rendering

Approval Request Comment
[Feature/regressing bug #]:
Bug 1301486.
Fix problems associated with Bug 1283274.

[User impact if declined]:
Continuing functional regressions for win64 users.
Continuing functional regression where window is displayed at wrong position when inputting some CCJK characters.

[Describe test coverage new/current, TreeHerder]:
Manual testing of add-on and update process.

[Risks and why]: 
Flips a pref that is already flipped in Firefox 50.
This pref change is known to cause some performance issues (see Bug 1309037). We are considering these issues to be acceptable given the critical regressions that occur without the pref change.

[String/UUID change made/needed]:
None
Flags: needinfo?(ksteuber)
Attachment #8798229 - Flags: approval-mozilla-release?
Comment on attachment 8798229 [details]
Bug 1307108 - Added system addon to enable async plugin rendering

We have uncovered one bug with manual testing but it isn't a blocker. Let's go ahead with the system add-on.
Attachment #8798229 - Flags: approval-mozilla-release? → approval-mozilla-release+
Release Note Request (optional, but appreciated)
[Why is this notable]: Noting system add-on changes as described in https://wiki.mozilla.org/Firefox/Go_Faster/Process
[Suggested wording]: UPDATE [datestamp-of-update-go-live] Async rendering enabled by default to improve scrolling, performance, and prevent plugin hangs and crashes

[Links (documentation, blog post, etc)]:

Benjamin, is there anything we should link to from the release note? Any suggestions for wording this note?
relnote-firefox: --- → ?
Flags: needinfo?(benjamin)
So it looks like this is ready to ship as a system add-on. NI a few folks who may be able to help get this out. My understanding from Liz is that it should go out ASAP.
Flags: needinfo?(standard8)
Flags: needinfo?(rhelmer)
Flags: needinfo?(mkelly)
Kinda swamped at the moment, rhelmer or Standard8 should be able to help.
Flags: needinfo?(mkelly)
We also need to make sure the code is landed on m-r (in case of dot release next week)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #31)
> We also need to make sure the code is landed on m-r (in case of dot release
> next week)

I can help with this part, I'll leave standard8 needinfo'd to help with comment 29 as I am not set up to make things live in Balrog at the moment.
Flags: needinfo?(rhelmer)
In this case, instead of landing the entire system add-on on mozilla-release, we can just land the pref flip
Attached patch pref_flip.patchSplinter Review
This patch flips the preference directly in mozilla-release (without using a system addon). Will only affect users when/if a dot release is made.
Attachment #8800907 - Flags: review?(felipc)
Attachment #8800907 - Flags: review?(felipc) → review+
Comment on attachment 8800907 [details] [diff] [review]
pref_flip.patch

Approval Request Comment
Same as last patch

[Feature/regressing bug #]:
Bug 1301486.
Fix problems associated with Bug 1283274.

[User impact if declined]:
Continuing functional regressions for win64 users.
Continuing functional regression where window is displayed at wrong position when inputting some CCJK characters.

[Describe test coverage new/current, TreeHerder]:
Manual testing of add-on and update process.

[Risks and why]: 
Flips a pref that is already flipped in Firefox 50.
This pref change is known to cause a performance issue (see Bug 1309037). We are considering this issue to be acceptable given the critical regressions that occur without the pref change.

[String/UUID change made/needed]:
None
Attachment #8800907 - Flags: approval-mozilla-release?
I've put this on the release-sysaddon channel along with the one from bug 1307108. They will be installed for 49.0 and 49.0.1 only. The complete set that should be installed with the update are:

- d3d9 1.0
- async plugin rendering 1.0
- Multi-process 1.3
Flags: needinfo?(standard8)
Suggested release note wording: "Asynchronous rendering of the Flash plugins is now enabled by default. This should improve performance and reduce crashes for sites that use the Flash plugin."
Flags: needinfo?(benjamin)
This has been shipped to 49.0 and 49.0.1 as a system add-on, and landed as a pref flip on 49.0.2 (see comment 39).
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
what did you do?
after installing 49.0.2 I see black window in Flash (full screen mode only).
49.0.1 Perfect.
@Yorgos Is it fixed in 49.0.2 by setting the pref |dom.ipc.plugins.asyncdrawing.enabled| to |false|?
ok now, fully fixed with "false". thanks.
Yorgos' problem seems potentially very serious. It seems like we need to confirm this ASAP. What should our next steps be?
Flags: needinfo?(rkothari)
Flags: needinfo?(felipc)
I noticed an issue with a kongregate game (the game, and even the firefox controls didn't respond well, like the scrollbar and window controls) after the firefox update.  After reading this thread, I tried this:

>@Yorgos Is it fixed in 49.0.2 by setting the pref |dom.ipc.plugins.asyncdrawing.enabled| to |false|?

It fixed the problem.

This is the site if you'd like to verify:
http://www.kongregate.com/games/cwwallis/obliterate-everything-3
(In reply to mfm773 from comment #45)
> I noticed an issue with a kongregate game (the game, and even the firefox
> controls didn't respond well, like the scrollbar and window controls) after
> the firefox update.  After reading this thread, I tried this:
> 
> >@Yorgos Is it fixed in 49.0.2 by setting the pref |dom.ipc.plugins.asyncdrawing.enabled| to |false|?
> 
> It fixed the problem.
> 
> This is the site if you'd like to verify:
> http://www.kongregate.com/games/cwwallis/obliterate-everything-3

How can be this fixed for all players we have from flash? Can we somehow set this from flash? Can we disable the async rendering?

Our players are not able to set this in Firefox. We are buying them from TV advertisements, Facebook, Google etc. and they come to game and it does not work. This is very serious for us.
Hi, another player on Kongregate here,

to me the problems are that the game keeps running, but after a minute or so, Firefox and the game both need seconds to about a minute to respond to clicks, keyboard events seem worse. Firefox gets responsive again after tab goes to background (took about a minute).

Even weirder, in one of the games some game elements keep animating, while other stop (wait for event handling?)

|dom.ipc.plugins.asyncdrawing.enabled| to |false| fixed the problem for me (had not expected to find the solution in the FF changelog, instead checked all the extensions, antivirus, firewall, ...)
Hey,

another game dev here. This performance fix has also massively decreased performance for our users as well, and the firefox user base is big in our game. According to the adobe thread (https://forums.adobe.com/thread/2224252), it should not even be enabled if the wmode is set to "window", but Firefox seems to ignore this.
We need separate bugs filed on specific behavior changes. Mozilla and Adobe are working together on this feature so that we will no longer support windowed-mode Flash, but should have good performance for accelerated rendering. Please file in Core:Plug-Ins and set NEEDINFO to me.
Flags: needinfo?(rkothari)
Flags: needinfo?(felipc)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #49)
> We need separate bugs filed on specific behavior changes. Mozilla and Adobe
> are working together on this feature so that we will no longer support
> windowed-mode Flash, but should have good performance for accelerated
> rendering. Please file in Core:Plug-Ins and set NEEDINFO to me.

We are developing flash games. After this change (async drawing) we have problem that our player clicks on button or stage and what should happen immediately, happens in some cases 10 seconds later. The game is not playable.

We need to disable the async rendering from flash somehow. How we can do that? Is that possible?

Thank you very much for help.
Flags: needinfo?(benjamin)
Please file a bug with precise steps to reproduce and NI? me.
Flags: needinfo?(benjamin)
I already did that.

@Vrato: You could try experimenting with setting a different wmode, since this may only affect the use of wmode=window
I found Chris filed bug 1311985. Please make sure bugs are filed to Core:Plug-Ins and/or set NEEDINFO so that I see them.
https://bugzilla.mozilla.org/show_bug.cgi?id=1307108#c42
I dont understand if this the final behaviour (set to "false") or if I have to wait for FF 49.0.3.
Sorry and Thank you Benjamin!

In the meantime, i have found out that setting the wmode to "gpu" improves overall performance (cpt obvious) and seems to fix the issue as well. This might be a viable solutions for other devs.
Depends on: 1311524
No longer depends on: 1311524
Hi,

With dom.ipc.plugins.asyncdrawing.enabled set to True, a page that uses Flash retains focus even when it is no longer the active tab. Is that to be expected?! It is wasting CPU, so I flipped back to false, which fixes it.

Adrian
Since 49.0.2, where you changed dom.ipc.plugins.asyncdrawing.enabled to true, many flash application are still active (also when tab is not active), it consumes much more CPU, and also about 2MB/sec of RAM, so in couple minutes / hours, the whole RAM is used, and windows shows error about NO RAM RESOURCES. Tried on many systems, with 4, 8 and 16 GB RAM, everyevery it crashed due to consuming whole RAM.

Setting back dom.ipc.plugins.asyncdrawing.enabled to false, or downgrade to 49.0.1 resolved this issue.
bosom01@yahoo.com and legionar@legitalk.sk, could you please make sure that these issues are filed as bugs? Please look through this dependency tree, and if the bug is not already filed please file a new bug in Core:Plug-Ins and set NEEDINFO to me?

https://bugzilla.mozilla.org/showdependencytree.cgi?id=1229961&hide_resolved=1
Blocks: 1312528
So yeah.. this completely ballsed up Forge of Empires game. The memory is consumed so fast I cannot even blink. Then windows forcefully closes plugin container process, and sometimes FF too. Also caused my FF to crash and restart without HW accel enabled, and I had to go to about:support to reset the HW accel. FFx64, windows 10 x64.
This completely breaks the camera/mic allow/deny dialog in flash for many, many sites.  Who's idea was it to release this on a minor release?  That is asinine.

We will be blocking access to our site through Firefox 49+.  This needs to be fixed _immediately_.
(In reply to Jeremy Noring from comment #60)
> This completely breaks the camera/mic allow/deny dialog in flash for many,
> many sites.  Who's idea was it to release this on a minor release?  That is
> asinine.
> 
> We will be blocking access to our site through Firefox 49+.  This needs to
> be fixed _immediately_.

It is not even minor, it is a micro release...

FF is successfully making people use another browser. Good job!
Whiteboard: [fce-active] → [fce-active][go-faster-system-addon]
Dexter, Thanks for reporting that issue. 
Jeremy, can you provide a specific example? I'd like to make sure we test and fix any issues with the camera/mic dialog and it would help to have a reproducible example.

Andrei, can your team try to reproduce the issues in comment 59 and comment 60, with async rendering enabled in nightly, and file new bugs for those issues? Thanks.
Flags: needinfo?(andrei.vaida)
Flags: needinfo?(jeremy.noring)
We're working around it in a different way--going to MediaRecorder, which comes with its own set of problems, but that's the only short-term option we have.  Currently Firefox is blocked; we'll unblock once we have a MediaRecorder solution.
Flags: needinfo?(jeremy.noring)
Dexter, Jeremy - could you please file new bug reports with specific steps to reproduce, for the issues you mentioned in comment 59 & 60?
Not sure exactly what to look for, this is the first time I'm playing, but Forge of Empires looks ok to me on FX 49.0.2 x64, Win 7 x64, dom.ipc.plugins.asyncdrawing.enabled=TRUE.
Flags: needinfo?(mydexterid)
Flags: needinfo?(jeremy.noring)
Flags: needinfo?(andrei.vaida)
(In reply to Paul Silaghi, QA [:pauly] from comment #64)
> Dexter, Jeremy - could you please file new bug reports with specific steps
> to reproduce, for the issues you mentioned in comment 59 & 60?
> Not sure exactly what to look for, this is the first time I'm playing, but
> Forge of Empires looks ok to me on FX 49.0.2 x64, Win 7 x64,
> dom.ipc.plugins.asyncdrawing.enabled=TRUE.

The issue was not with how it looked or not, but how fast it ate up my 16Gigs of ram. Sometime it ate up immediately, sometimes only after about half an hour and Windows 10 OOM killer (or some equivalent of it) killed the plugin container and FF processes.
Flags: needinfo?(mydexterid)
Paul, the issue is with the camera allow/deny dialog in flash.  To use the microphone/camera in flash, there's a permissions step that has to happen.  On many sites, that allow/deny dialog no longer shows.

I'm not filing any defect--we moved to media recorder.  Feel free to test/file on your own.
Flags: needinfo?(jeremy.noring)
(In reply to Paul Silaghi, QA [:pauly] from comment #64)
> Dexter, Jeremy - could you please file new bug reports with specific steps
> to reproduce, for the issues you mentioned in comment 59 & 60?
> Not sure exactly what to look for, this is the first time I'm playing, but
> Forge of Empires looks ok to me on FX 49.0.2 x64, Win 7 x64,
> dom.ipc.plugins.asyncdrawing.enabled=TRUE.

So now that I upgraded to FF 50, the FOE game does not even load. The screen is all black.
(In reply to DEXTER from comment #67)
> So now that I upgraded to FF 50, the FOE game does not even load. The screen
> is all black.

Please file a separate bug for this.
Whiteboard: [fce-active][go-faster-system-addon] → [fce-active-legacy][go-faster-system-addon]
You need to log in before you can comment on or make changes to this bug.