1131368 - Re-enable flash for desktopRT

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Description

[(not reading bugmail) Nick Desaulniers [:\n]]

We're looking to re-enable flash on the desktop runtime.

Comment 1

[Bill Walker [:bwalker] [@wfwalker]]

Note: The decision in bug 1040996 to disable Flash for desktop apps was taken without consulting our business development team. Some of our partners are interested in publishing desktop open web apps that temporarily rely on flash while they build out non-flash mobile web apps. We'd like to support the business development team in bringing those important apps to Firefox Marketplace

Comment 2

[Bill Walker [:bwalker] [@wfwalker]]

see also bug 768521

Comment 3

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

Top Must Have Apps blocked by this right now include: TuneIn, Zinio, Spotify. Every few days another big name adds to the list.

Comment 4

[(not reading bugmail) Nick Desaulniers [:\n]]

Thoughts:

What happens when:
* A user does not have flash installed?
* A user has a deprecated version of flash, that we block by default?
* A user has flash set up to be blocked by default?

Comment 5

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

To further add to Comment 3 — I just met with IVI Movies (think Hulu) and IVI Music (think Vevo) who are the out and out leaders in Russia.
http://www.ivi.ru/
http://music.ivi.ru/

They too would love to submit for Desktop Marketplace but solve for DRM with Flash today (without other alternatives)

Comment 6

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: patch.txt

Comment 7

[(not reading bugmail) Nick Desaulniers [:\n]]

I wonder if there is a simpler site to test?  This doesn't seem to be working with TuneIn, which is published but not listed until this bug is fixed. https://marketplace.firefox.com/app/tunein-radio/

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8568038 [details] [diff] [review]
patch.txt

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

This worked before we changed it; unsure why it isn't working now.  I've added a "Hello World" Flash applet to Mykzilla <http://www.mykzilla.org/app/> to help test, and it works fine in the browser, but it doesn't work in the runtime.

I notice that the browser prefs file contains a variety of other prefs referencing Flash and plugins, but I don't see any that should affect the behavior in the runtime.  It may be that a change to "click to play" in the last six months has regressed this.  We need to consult a plugins expert.

Comment 9

[(not reading bugmail) Nick Desaulniers [:\n]]

trying a build now with `plugins.click_to_play` pref set to false.  I think with the whitelist of available plugins, it's ok to turn this on by default.

Comment 10

[(not reading bugmail) Nick Desaulniers [:\n]]

That didn't work, ni'd bsmedberg for some help.

Comment 11

[Benjamin Smedberg]

I have two concerns here.

#1 is the basic request: we are actively trying to phase Flash out of the web, to the point of deploying shumway for advertising. This is not something we want to allow or encourage. I believe it would be better not to have a desktop app at all, than to have it depending on Flash. Bill, if there is strong disagreement we should talk about our Flash strategy and what problems you're trying to solve. Otherwise we should WONTFIX this.

#2 is the quality requirements surrounding Flash. We must honor the blocklist and use the CtA Flash UI if Flash is known-vulnerable (or disable it altogether). This is going to mean a significant amount of work porting the CtA UI into the webapp runtime.

As for the technical question, plugins.click_to_play doesn't affect the CtA state of a plugin, it only changes how the prefs appear in the addon manager. The plugin.state.* prefs control the actual activation.

Comment 12

[(not reading bugmail) Nick Desaulniers [:\n]]

plugin.state.flash defaults to 2, which looks like always active, (line number relevant, up the irons Earthdogs!)[0].  Setting it to 1 is ask to activate, and 0 is never activate.  We don't change it in webapprt/prefs.js, so I'm not sure that's the reason we simply see a dark grey box.

[0] http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#666

Comment 13

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> I have two concerns here.
> 
> #1 is the basic request: we are actively trying to phase Flash out of the
> web, to the point of deploying shumway for advertising. This is not
> something we want to allow or encourage. I believe it would be better not to
> have a desktop app at all, than to have it depending on Flash. Bill, if
> there is strong disagreement we should talk about our Flash strategy and
> what problems you're trying to solve. Otherwise we should WONTFIX this.

Hi Benjamin,

No disagreement from me or the runtime team about phasing out Flash. Indeed, that's why we disabled Flash for desktop apps last year. However, we now have a handful of critical partner apps we can bring to Firefox OS if those partners can promote their currently Flash-based desktop web apps in our Marketplace. I've made it clear this would only be a temporary change.

I asked Nick to look into this because I assumed he merely needed to reverse the one line change we made in bug 1040996. I think you're saying there's no way around supporting the CtA in the Web Runtime, is that right?

Comment 14

[Benjamin Smedberg]

I'm saying that if we enable Flash we need the blocklist to work correctly. The specifics of how it should work don't have to involve click-to-play UI, but could be straight-up blocking or something.

We should also quickly investigate whether we can render the SWF for those partners using Shumway. Can you talk to cpeterson about that?

Comment 15

[Chris Peterson [:cpeterson]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #14)
> We should also quickly investigate whether we can render the SWF for those
> partners using Shumway. Can you talk to cpeterson about that?

Hi Bill, Shumway does not currently support Spotify (bug 1066196) and it is unlikely that other complicated Flash-based sites like TuneIn or Zinio will work yet. The Shumway team is currently focused on Flash ads, but we have had success targeting specific media players on Amazon, IMDb, and Facebook. If your team would like to add support for specific partner content, the Shumway team can guide you.

Nick asked me about RTMP streaming. Shumway has preliminary RTMP support, extracted into a standalone library (but requires moz- prefixed APIs): https://github.com/yurydelendik/rtmp.js

Comment 16

[(not reading bugmail) Nick Desaulniers [:\n]]

Bug 755551 added nsPluginHost::IsTypeWhitelisted which is called exclusively from nsPluginTag::InitMime [0]. When I attach gdb to a running instance of a web app, InitMime is called 3 or 4 times, but the last argument, aVariantCount is always 0, so we skip the entire loop that even checks the whitelist [1]!

[0] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginTags.cpp#265
[1] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginTags.cpp#254

Comment 17

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: patch v1

not ready yet, just un-bit-rotting the previous version

Comment 18

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: backtrace.txt

Comment 19

[(not reading bugmail) Nick Desaulniers [:\n]]

The mimetypecount is always 0 [0].  This leads to aVariantCount always being 0 [1][2], which skips checking the whitelist.

Thoughts on what could be going wrong.  Either:
* nsPluginManifestLineReader is doing something wrong.
* we're doing something that makes nsPluginManifestLineReader not happy.
* we should be checking the whitelist sooner.
* ?

[0] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#2902
[1] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#2953
[2] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginTags.cpp#199

Comment 20

[(not reading bugmail) Nick Desaulniers [:\n]]

Or, something could be messed up with the NSPR file [0].  `mPath.get()` [1] is `"/Users/Nicholas/Library/Application Support/mykzillaallizkym-362b12c70d0556c124908a3c125d3d02/Profiles/aa1zea78.default/pluginreg.dat"`

[0] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#2724
[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#397

Comment 21

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: pluginreg.dat

Comment 22

[(not reading bugmail) Nick Desaulniers [:\n]]

Or we could be writing the pluginreg.dat file incorrectly [0].

[0] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#2574

Comment 23

[(not reading bugmail) Nick Desaulniers [:\n]]

I'm not sure when nsPluginHost::WritePluginInfo is called.  I thought FF might call it when installing the app, but running `./mach run --debug` and setting a breakpoint at nsPluginHost::WritePluginInfo in lldb did not get caught.  On a fresh install of mykzilla, running `gdb /Applications/Mykzilla\ –\ allizkyM.app/Contents/MacOS/webapprt` and setting a breakpoint at nsPluginHost::WritePluginInfo in gdb also did not get caught.  Based on what is happening in nsPluginHost::ReadPluginInfo and my pluginreg.dat that I uploaded, I think that pluginreg.dat is not being written correctly in nsPluginHost::WritePluginInfo, but I can't seem to get a breakpoint or backtrace to who or where it's being called from.

Comment 24

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: pluginreg.dat Firefox

Compare pluginreg.dat from an instance of a running desktopRT app to this one from Firefox.  Something is definitely wrong with the plugin registry list.  For instance, this one from Firefox lists the number of MIME types the plugin is responsible for, followed by the MIME types themselves.  For the plugin registry for webappRT, it's 0 and empty!  This was captured from `obj-x86_64-apple-darwin14.1.0/tmp/scratch_user/pluginreg.dat`.

Comment 25

[(not reading bugmail) Nick Desaulniers [:\n]]

Upon installation of an app, the directory:

~/Library/Application\ Support/<app name>-<uuid>/Profiles/<8 random chars>.default/

is created but only contains:
└── times.json

0 directories, 1 file

running: `watch -n1 tree <that long dir name>` I see the pluginreg.dat get created upon first launch.

A breakpoint set at nsPluginHost::WritePluginInfo does get hit (so it looks like the plugin registry is written once on first launch of an app, not touched since (even after `touch`ing the pluginreg.dat)).  The plugin registry persists through uninstalls and reinstalls of the app (maybe a separate bug should be filed for this; `Application Support for wabappRT not removed on uninstall`)!  This makes debugging slightly more tedious since I can only catch this once per install, then I have to:

rm -rf /Applications/Mykzilla\ –\ allizkyM.app
rm -rf ~/Library/Application\ Support/my<tab complete>
./mach run
<reinstall app>
gdb /Applications/Mykzilla\ –\ allizkyM.app/Contents/MacOS/webapprt
b nsPluginHost::WritePluginInfo
r

tag->mMimeTypes.Length() [0] is 0 for each plugin tag [1], so nothing is getting written to the plugin registry.

Strangely, I just fired up gdb again, and tag->mMimeTypes.Length() is suddenly 2!  And it's all working (Flash is running in mykzilla).  Going to go make sure I'm not losing my mind...

...ok back. I wonder if this is because I cleared my Application Support dir?  Yeah, it works.  WTF.  Will file a follow up bug to see if we can hook into app uninstall to remove the application support dir.

[0] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#2614
[1] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#2576
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsMimeTypeArray.h#59

Comment 26

[(not reading bugmail) Nick Desaulniers [:\n]]

So the current patch is ready for review.  If you're testing with a previously installed app, make sure you remove the Application Support dir for it on OSX.  Whether or not this should be enabled is maybe another discussion.

rm -rf ~/Library/Application\ Support/<beginning of app name><tab complete>

Comment 27

[Benjamin Smedberg]

There's a patch on this bug, but I don't think this patch or any of the comments here address the core problem of having the blocklist continue to work (and dealing with the UI or lack of UI in that case). What is the status of that? This must not proceed without the security issue resolved.

Comment 28

[(not reading bugmail) Nick Desaulniers [:\n]]

I think we should outright block the plugin if it's outdated.  Myk mentions the case that someone downloads Firefox just to install an app, and never runs FF again; they might not be able to upgrade Flash (since they aren't running FF to get a prompt), but that doesn't need to be solved in the immediate future to turn on Flash for the majority of users.

Shouldn't the existing plugin code block the plugin if it's outdated?

Comment 29

[Benjamin Smedberg]

I don't know whether the blocklist code runs or works at all in webapprt. Do you have any automated test coverage for webapprt in general or the blocklist in particular?

Comment 30

[(not reading bugmail) Nick Desaulniers [:\n]]

The webappRT has general test coverage.  It doesn't look like tests for the whitelist were added in 755551 or 755554, yet Flash was turned on for webappRT.  See also 763783 & 773685.

Comment 31

[Myk Melez [:myk] [@mykmelez]]

Over in bug 1183372, I've added tests for the plugin.allowed_types pref.  These are not blocklist tests, just a preliminary step to ensure testing for the platform code on which the runtime depends.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/122f07d8abbb

Comment 33

[Myk Melez [:myk] [@mykmelez]]

Erm, comment 32 is actually the commit for the change in bug 1183372.  It does not fix this bug, and this bug should not be resolved after that commit is uplifted.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/122f07d8abbb

Comment 35

[Matthew N. [:MattN]]

https://hg.mozilla.org/mozilla-central/rev/122f07d8abbb

Comment 36

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8596684 [details] [diff] [review]
patch v1

Cancelling this review pending investigation into blocklist tests.

Comment 37

[Myk Melez [:myk] [@mykmelez]]

Per bug 1238079, we're going to disable the desktop web runtime and remove it from the codebase, so we won't fix these bugs in it.