Benjamin Smedberg Reporter
	Description • 8 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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 1 • 8 years ago

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?

	Benjamin Smedberg Reporter
	Comment 2 • 8 years ago

Yes, the situation has changed because there are multiple user-visible regressions.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 3 • 8 years ago

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?

	Benjamin Smedberg Reporter
	Comment 4 • 8 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.

	David Durst [:ddurst]
	Comment 5 • 8 years ago

Banding together bytesized and felipe for this (felipe mentoring, bytesized owning).

	:Felipe Gomes (needinfo for replies!)
	Comment 8 • 8 years ago

You don't need the uninstall part because we don't need to revert this pref.

	:Felipe Gomes (needinfo for replies!)
	Comment 9 • 8 years ago

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

	Benjamin Smedberg Reporter
	Comment 10 • 8 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)

	:Felipe Gomes (needinfo for replies!)
	Comment 13 • 8 years ago

(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.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 14 • 8 years ago

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?

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 15 • 8 years ago

Oops. Forgot to revert the version value in the addon after testing on Nightly.

	:Felipe Gomes (needinfo for replies!)
	Comment 16 • 8 years ago

(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)

	Benjamin Smedberg Reporter
	Comment 17 • 8 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.

	:Felipe Gomes (needinfo for replies!)
	Comment 18 • 8 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.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 19 • 8 years ago

Carrying forward r+

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 21 • 8 years ago

@jason Could I get this XPI signed please?

	Jason Thomas [:jason]
	Comment 22 • 8 years ago

Please see signed add-on.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 23 • 8 years ago

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

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 24 • 8 years ago

Removing request for uplift approval until issues are resolved.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 25 • 8 years ago

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.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 26 • 8 years ago

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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 27 • 8 years ago

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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 28 • 8 years ago

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?

	Cory Price [:ckprice] (bugmail disabled, NI me!)
	Comment 29 • 8 years ago

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.

	Osmose [:osmose, :mkelly]
	Comment 30 • 8 years ago

Kinda swamped at the moment, rhelmer or Standard8 should be able to help.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 31 • 8 years ago

We also need to make sure the code is landed on m-r (in case of dot release next week)

	Robert Helmer [:rhelmer]
	Comment 32 • 8 years ago

(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.

	:Felipe Gomes (needinfo for replies!)
	Comment 33 • 8 years ago

In this case, instead of landing the entire system add-on on mozilla-release, we can just land the pref flip

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 34 • 8 years ago

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.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 35 • 8 years ago

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

	Mark Banner (:standard8)
	Comment 36 • 8 years ago

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

	Benjamin Smedberg Reporter
	Comment 37 • 8 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."

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-release/rev/a5d31ad7e142

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 39 • 8 years ago

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

	:Felipe Gomes (needinfo for replies!)
	Comment 40 • 8 years ago

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).

	Yorgos
	Comment 41 • 8 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.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 42 • 8 years ago

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

	Yorgos
	Comment 43 • 8 years ago

ok now, fully fixed with "false". thanks.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 44 • 8 years ago

Yorgos' problem seems potentially very serious. It seems like we need to confirm this ASAP. What should our next steps be?

	mfm773
	Comment 45 • 8 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

	Vrato
	Comment 46 • 8 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.

	jojo314
	Comment 47 • 8 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, ...)

	Chris
	Comment 48 • 8 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.

	Benjamin Smedberg Reporter
	Comment 49 • 8 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.

	Vrato
	Comment 50 • 8 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.

	Benjamin Smedberg Reporter
	Comment 51 • 8 years ago

Please file a bug with precise steps to reproduce and NI? me.

	Chris
	Comment 52 • 8 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

	Benjamin Smedberg Reporter
	Comment 53 • 8 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.

	Yorgos
	Comment 54 • 8 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.

	Chris
	Comment 55 • 8 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.

	bosom01
	Comment 56 • 8 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

	Martin Durec
	Comment 57 • 8 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.

	Benjamin Smedberg Reporter
	Comment 58 • 8 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

	DEXTER
	Comment 59 • 8 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.

	Jeremy Noring
	Comment 60 • 8 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_.

	DEXTER
	Comment 61 • 8 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!

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 62 • 8 years ago

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.

	Jeremy Noring
	Comment 63 • 8 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.

	Paul Silaghi, QA [:pauly]
	Comment 64 • 8 years ago

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.

	DEXTER
	Comment 65 • 8 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.

	Jeremy Noring
	Comment 66 • 8 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.

	DEXTER
	Comment 67 • 8 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.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 68 • 8 years ago

(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.