769721 - Force-enable OOPP for Flash users on Windows Vista+

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

We have evidence that some set of our users have the pref "dom.ipc.plugins.enabled" customized off, even though there isn't any exposed UI for this pref. This may be from instructions in the early days of 3.6.4 or even beta testing, where there were lots of unresolved performance issue. I believe that most of these issues are resolved, and even more importantly Adobe is not testing the new versions of Flash with the in-process configuration, which means that we're seeing crashes which are specific to in-process.

I think that we should force Flash to be out-of-process. The simplest way to do this is by renaming the pref to dom.ipc.plugins.disabled, but we could try something more Flash-specific if necessary.

I also tried to search the addons MXR for "dom.ipc.plugins" but it timed out... I'd like to see if there are any addons which are disabling IPC plugins in general, and fix that. Note that it may be a valid technique in some cases to turn off IPC plugins for specific addon-specific plugins (a binary loading technique like ctypes).

Comment 1

[Benjamin Smedberg]

Attachment: Flip the pref name from "enabled" to "disabled", rev. 1

akeybl isn't sure about taking this in 14 because it may mask other issues, but I think I'd like to take it on trunk in any case.

Comment 2

[Benjamin Smedberg]

Actually I'm not sure I like this version. I decided to change *all* the prefs from "enable" to "disable", instead of just the main pref, but that will affect extensions which use NPAPI plugins as a form of binary interface. I think maybe we should continue to honor dom.ipc.plugins.<pluginname>.enabled, or something like that...

Comment 3

[Benjamin Smedberg]

Attachment: Flash on Vista+ forced into OOPP mode, rev. 1

Here's the minimal version for possible branch inclusion.

Comment 4

[Josh Aas]

Comment on attachment 638822 [details] [diff] [review]
Flip the pref name from "enabled" to "disabled", rev. 1

Review of attachment 638822 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +351,5 @@
>  
>    // Java plugins include a number of different file names,
>    // so use the mime type (mIsJavaPlugin) and a special pref.
>    if (aPluginTag->mIsJavaPlugin &&
> +      !Preferences::GetBool("dom.ipc.plugins.java.disabled", false)) {

Did you mean to remove the "!" here? Seems like you changed the default value but you're still reversing the value of the pref value itself, so if "disabled == true" you'll return false.

@@ +404,3 @@
>  #endif
>  #else
> +    Preferences::GetBool("dom.ipc.plugins.disabled", true);

Are you sure you want the default here (plus the three above) to be true? I don't remember why it was false, but it seems like we want to default to OOP now (disabled == false). I guess to be conservative we could keep the old behavior in any emergency fix, but try defaulting to OOPP on trunk.

Comment 5

[Benjamin Smedberg]

Comment on attachment 638827 [details] [diff] [review]
Flash on Vista+ forced into OOPP mode, rev. 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Flash 11.3 stability
User impact if declined: More crashy Firefox when running with Flash protected mode *and* when Flash upgrades (even if protected mode is disabled)
Testing completed (on m-c, etc.): none!
Risk to taking this patch (and alternatives if risky): The actual code risk of this appears very low to me. The risk is mainly user perception for users who have set the hidden pref and it is no longer honored.

Comment 6

[Benjamin Smedberg]

Morphing this bug for branch tracking, I'll file the original bug again separately.

Comment 7

[Benjamin Smedberg]

https://hg.mozilla.org/mozilla-central/rev/b39f4007be5a

Comment 8

[Benjamin Smedberg]

I cannot stay around to push this to beta today, you'll have to find somebody else if it's important.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 638827 [details] [diff] [review]
Flash on Vista+ forced into OOPP mode, rev. 1

[Triage Comment]
Approved for both Aurora 15 and Beta 14. We'll make sure to get some QA around this bug this week.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b9fedd50fd59
https://hg.mozilla.org/releases/mozilla-beta/rev/d1452658046f

Comment 11

[Scoobidiver (away)]

Will other plugins such as Java run in OOPP on Vista and above? In that case, why not on XP?

Comment 12

[Alex Keybl [:akeybl]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> https://hg.mozilla.org/mozilla-central/rev/b39f4007be5a

I'm not sure we have a good bug to test this against - will QA be able to find the pref outputted on the console during plugin load or somewhere similar?

Comment 13

[Benjamin Smedberg]

This bug only affects Flash; Java will continue to default to in-process on all versions of Windows.

The best way for QA to verify the fix is:
* set dom.ipc.plugins.enabled to false and restart the browser
* load a Flash page (youtube)
* check the task manager and see if there is a plugin-container process running (there should be on Windows Vista+, and there should not be on Windows XP)

Comment 14

[Paul Silaghi, QA [:pauly]]

Verified fixed using STR in comment 13 on FF 14b11. The plugin-container process is seen on Win Vista but not on Win XP. Removing qa wanted.

Comment 15

[Terrell Kelley]

I do not like this at all. You are intentionally introducing option differences in about:config--something I don't believe I've seen before. If you don't want to accidentally enable the addon in Windows XP, have it check the old preference and then use that info to set the new one. Having two different ways to do the exact same thing in two different OSes is just bad.

Comment 16

[Gordon Shumway]

Attachment: Stack traces from plugin-container.exe.

TID 1704 and 5392 eventually exited, although after 30 seconds to 2 minutes.

I set dom.ipc.plugins.timeoutSecs & processLaunchTimeoutSecs to 600 in order to get the stack logs, and was done in well under 10 minutes.

Comment 17

[Gordon Shumway]

This is a real problem for me. Flash hangs on my system, I have no idea why, but I can't disable the OOP plugins to even see if that's the issue. It seems odd that a process that's specifically there to prevent problems in a plugin from affecting Firefox could hang the browser though, so my initial instinct (which may be entirely wrong) is that the problem lies in plugin-container.exe, or at least it doesn't handle some situation it should.

If I should file a separate bug (I don't know if you guys reopen yours or not), please let me know, and also please let me know if there's any additional helpful information I can give.

Comment 18

[Benjamin Smedberg]

Gordon, you should definitely file a new bug, it's a different issue from this bug. The stack traces aren't really helpful because they don't contain symbols, but if you'd like to attach minidumps to your new bug, we can turn those into more useful stacks. Please also indicate which version of Flash you're testing with, and whether this has been a problem with older versions of Flash or is new with Flash 11.3. cc me, although I'll be away until 7-August so it may not receive attention until then.

Comment 19

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi [QA] from comment #14)
> Verified fixed using STR in comment 13 on FF 14b11. The plugin-container
> process is seen on Win Vista but not on Win XP. Removing qa wanted.

Verified fixed on FF 15b2.

Comment 20

[alexander :surkov (:asurkov)]

This ruins down the screen reader user experience as performance impact. I think we can detect whether a11y or certain screen readers are running to decide whether we should or shouldn't honor user preferences.

Comment 21

[Robert Kaiser]

IMHO, if we need special user prefs like that to make a11y work, then something is wrong from the beginning. If a11y needs OOP disabled, or needs it for at least some plugins, that's on one hand something we should try to work on fixing in the long term, but on the other, this should be done automatically.

Comment 22

[Rich Schwerdtfeger]

Just weighing in here. 

Firefox has introduced a modification in Firefox 14 that essentially locks down the Flash player into a separate process and removed a vehicle to disable this feature. Having built screen readers  I can tell you that this creates real problems for screen reader users.

1. When the screen reader hooks into the browser it hooks in process to be able to be performant. When Flash runs in a separate process it means that the screen reader needs to access the Flash process out of process make the user experience incredibly slow as it introduces a context switch. Context switching is subject to OS scheduling and memory mapping overhead that persists regardless of how fast your processor runs. This change negatively impacted the performance of both JAWS and NVDA.
2. By sticking Flash in a separate process you make it a separate model entry. Screen Readers do not merge virtual buffers and interactions across processes. Think trying to merge an MSWord document view with Firefox. It is a usability disaster as you are mixing two different applications. Yet, unlike Word, Flash should be integrated with seemlessly with the web page yet now you are forcing the screen reader to separate two parts of the Web experience - a usability disaster. I am sure NVDA is also not happy about this.

While I understand the perceived security problem I strongly urge you to ask Mozilla to put feature back into 14 that allows the screen reader to have Firefox put Flash in the same process.

Comment 23

[Scoobidiver (away)]

Rich, I think you should file a new bug and mark it as a regression from this one.

Comment 24

[Rich Schwerdtfeger]

I filed a regression defect as you requested: https://bugzilla.mozilla.org/show_bug.cgi?id=785047

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Robert Kaiser (:[e-mail]) from comment #21)
> IMHO, if we need special user prefs like that to make a11y work, then
> something is wrong from the beginning. If a11y needs OOP disabled, or needs
> it for at least some plugins, that's on one hand something we should try to
> work on fixing in the long term,

true, not sure where to start from though

> but on the other, this should be done
> automatically.

the problem here why I wouldn't disable OOP automatically when a11y is enabled is the a11y can be started due to numerous reasons not really related with accessibility. Whitelisting it seems doesn't work well here too since we don't know all consumers.

Comment 26

[Terrell Kelley]

This bug does not seem to have actually been resolved. Yes, plugin-container.exe is now running even with dom.ipc.plugins.enabled set to false, which was your only testing criteria. But setting dom.ipc.plugins.disabled to true does not seem to work. Flash continues to load in a new process.

This causes a performance hit, as apparently firefox.exe still has some stuff to do while plugin-container.exe is running the plugin. (It usually takes up about half again what plugin-container.exe is running.) The combined total is higher than if the plugin is run in-process. This is a problem on single-core netbooks where running videos* taxes the processor almost to the max as is.

*Except on YouTube, which has a more efficient Flash video player. But I lose the ability to watch 720p videos on there.

Please fix this. It's the only thing keeping me from running Windows 8 on my netbook.

Comment 27

[Terrell Kelley]

Forgot to mention: setting dom.ipc.plugins.enabled.npswf32.dll to false doesn't work either. Again, this was supposed to still work for individual plugins.

And, BTW, this didn't work as far back as the beta for 14.0.

Comment 28

[Gordon Shumway]

This worked for me:

http://forums.adobe.com/thread/1018071#TemporaryWorkaround

This is a workaround, that as far as I know negates the additional security from out-of-process flash completely, but for those of us that this issue is causing Firefox to be effectively unusable, it's something until plugin-container is no longer able to hang the browser.

Comment 29

[Scoobidiver (away)]

Terrell, this bug is fixed since Firefox 14 and you described the behavior expected by this bug (and Adobe): Flash is an OOPP on Vista and above and there's no way to disable it with a Firefox preference.

Comment 30

[Terrell Kelley]

You are incorrect. I refer you to comment 0, comment 1, comment 2, all of which outline a process for allowing the user to opt to keep plugins in-process. Seeing as comment 2 was even written by the guy who submitted the bug fix, and comment 3 is where it was submitted, it is clear to anyone who can read that the intent was to allow in-process Flash to be enabled.

Furthermore, such behavior would be in contradiction of the very core principles of Firefox, which is that we do not take the choice away from the user. The bug here as submitted is not to force users to do something they don't want to do, but to fix a setting they might have accidentally set.

Adobe has 0 expectations on whether Firefox can or cannot disable a feature. It only has the expectation that most users will run Flash out of process. They have even provided a method of dealing with crashes that they think happen because they designed Flash to run out of process. Adobe adhered to the core principles of Mozilla more than Firefox is currently doing.

Furthermore, that fix is actually necessary when you run Flash out-of-process. So Adobe actually just screwed up. It is not right for you to fix Adobe's bug for them. You are not Adobe Firefox.

I can't help it that, despite copying the code from Google Chrome, Chrome can successfully pull oft out-of-process plugins without significant overhead, but you can't. Firefox's main EXE is still heavily active when Flash is running out-of-process. Heck, that's another thing mentioned in comment 0--that you were doing this because there was no longer any performance issues. That is incorrect.

I was being nice before. You guys have screwed up. There is a bug in the submission that is going beyond the intended effect. Or you gave false information about what you were doing, thus not allowing other people to chime in and object. 

I do not appreciate being given false information. Either you are telling me something untrue, or Smedberg gave false information. You are in direct contradiction.

Comment 31

[Terrell Kelley]

Mr. Shumway: I appreciate the help, but that does not fix the problem. I wish it did. It just disables the sandbox. Flash still runs out of process, and still has significant performance issues. The exact same issues it has on Windows XP when run out-of-process, with the same issue of Firefox running half again as much processing time as plugin-container. The exact same issue that is resolved if I use a version of Firefox before this fix was put in.

Comment 32

[John Schoenick [:johns]]

(In reply to Terrell Kelley from comment #31)
> Mr. Shumway: I appreciate the help, but that does not fix the problem. I
> wish it did. It just disables the sandbox. Flash still runs out of process,
> and still has significant performance issues. The exact same issues it has
> on Windows XP when run out-of-process, with the same issue of Firefox
> running half again as much processing time as plugin-container. The exact
> same issue that is resolved if I use a version of Firefox before this fix
> was put in.

If you are having specific performance issues with OOP plugins, you should file a new bug with steps to reproduce as well as information about your hardware profile (including the text in about:support would be useful) so it can be investigated. Reactivating an unsupported and untested codepath with known issues is not a good solution.

Comment 33

[Steven Penny]

Terrell said it perfectly

> Furthermore, such behavior would be in contradiction of the very core principles of Firefox, 
> which is that we do not take the choice away from the user. The bug here as submitted is not
> to force users to do something they don't want to do, but to fix a setting they might have
> accidentally set.

this "bug" is based on false pretense that we or anyone should be "forcing" the browser to act a certain way, and removing all choice from the user? What is the point of the about:config setting if it does nothing? A glaringly simple fix would be to simply edit "prefs.js" with each Firefox upgrade, not go mucking with the code that isnt broken, then ignoring reasoned objections.

Comment 34

[Steven Penny]

> To add the variable: right click on
> My Computer, Properties, Advanced tab, Environment Variables
> Select New in the lower pane (System Variables, which are global) and enter
> MOZ_DISABLE_OOP_PLUGINS in the name field and 1 in the value field.

http://support.mozilla.org/en-US/questions/932561

Comment 35

[Chris]

This bug is just plain sad to read, it starts off with a silly reason to remove the pref "we dont like users choosing how to operate their own browser" and then a even sillier reason to ignore objective reasons to reinstate it.

My firefox I cannot get to reuse the plugin container now LOL.

I await someone to tell me to make a NEW bug as it seems devs here are incapable of reverting bad design choices without new bug reports.