Enable async rendering by default on 49 using a system addon

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: benjamin, Assigned: bytesized)

Tracking

(Depends on: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49blocking fixed, relnote-firefox 49+)

Details

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

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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)
(Reporter)

Comment 2

3 years ago
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?
status-firefox49: --- → affected
tracking-firefox49: --- → blocking
(Reporter)

Comment 4

3 years ago
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)

Updated

3 years ago
Assignee: nobody → ksteuber
Posted file async_rendering.xpi (obsolete) —
Attachment #8798227 - Flags: review?(felipc)
(Assignee)

Updated

3 years ago
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)
(Reporter)

Comment 10

3 years ago
mozreview-review
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)
Comment hidden (mozreview-request)
Posted 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?
Posted 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)
(Reporter)

Comment 17

3 years ago
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 18

3 years ago
mozreview-review
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+
Posted file async_rendering.xpi
Carrying forward r+
Attachment #8798499 - Attachment is obsolete: true
Attachment #8798499 - Flags: review?(felipc)
Attachment #8798519 - Flags: review+
Comment hidden (mozreview-request)
@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
(Assignee)

Updated

3 years ago
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
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)
(Reporter)

Comment 37

3 years ago
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)

Comment 38

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/a5d31ad7e142
status-firefox49: affected → fixed
relnote-firefox: ? → 49+
Comment on attachment 8800907 [details] [diff] [review]
pref_flip.patch

Felipe told me this has already landed on moz-release branch, see cset https://hg.mozilla.org/releases/mozilla-release/rev/a5d31ad7e142af09e09d2a2166744f4c4dffdf13
Attachment #8800907 - Flags: approval-mozilla-release?
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
Last Resolved: 2 years ago
Resolution: --- → FIXED

Comment 41

2 years ago
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|?

Comment 43

2 years ago
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)

Comment 45

2 years ago
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

Comment 46

2 years ago
(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.

Comment 47

2 years ago
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, ...)

Comment 48

2 years ago
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.
(Reporter)

Comment 49

2 years ago
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)

Comment 50

2 years ago
(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)
(Reporter)

Comment 51

2 years ago
Please file a bug with precise steps to reproduce and NI? me.
Flags: needinfo?(benjamin)

Comment 52

2 years ago
I already did that.

@Vrato: You could try experimenting with setting a different wmode, since this may only affect the use of wmode=window
(Reporter)

Comment 53

2 years ago
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.

Comment 54

2 years ago
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.

Comment 55

2 years ago
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.

Updated

2 years ago
Depends on: 1311524

Updated

2 years ago
No longer depends on: 1311524

Comment 56

2 years ago
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

Comment 57

2 years ago
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.
(Reporter)

Comment 58

2 years ago
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
(Reporter)

Updated

2 years ago
Blocks: 1312528

Comment 59

2 years ago
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.

Comment 60

2 years ago
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_.

Comment 61

2 years ago
(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)

Updated

2 years ago
Flags: needinfo?(jeremy.noring)

Comment 63

2 years ago
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.

Updated

2 years ago
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)

Comment 65

2 years ago
(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)

Comment 66

2 years ago
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)

Comment 67

2 years ago
(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]
Comment hidden (spam)
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.