Closed
Bug 1131368
Opened 10 years ago
Closed 9 years ago
Re-enable flash for desktopRT
Categories
(Firefox Graveyard :: Webapp Runtime, defect, P1)
Firefox Graveyard
Webapp Runtime
Tracking
(firefox42 fixed)
RESOLVED
WONTFIX
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: nick, Assigned: nick)
References
Details
(Whiteboard: DesktopWebRT2 [dependency: marketplace-partners])
Attachments
(4 files, 1 obsolete file)
We're looking to re-enable flash on the desktop runtime.
Comment 1•10 years ago
|
||
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•10 years ago
|
||
see also bug 768521
Comment 3•10 years ago
|
||
Top Must Have Apps blocked by this right now include: TuneIn, Zinio, Spotify. Every few days another big name adds to the list.
Assignee | ||
Comment 4•10 years ago
|
||
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?
Updated•10 years ago
|
Whiteboard: DesktopWebRT2
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8568038 -
Flags: review?(myk)
Assignee | ||
Comment 7•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8568038 -
Flags: review?(myk)
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
That didn't work, ni'd bsmedberg for some help.
Flags: needinfo?(benjamin)
Comment 11•10 years ago
|
||
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.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 12•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
(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
Assignee | ||
Comment 16•10 years ago
|
||
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
Assignee | ||
Comment 17•10 years ago
|
||
not ready yet, just un-bit-rotting the previous version
Attachment #8568038 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
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
Assignee | ||
Comment 20•10 years ago
|
||
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
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
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
Assignee | ||
Comment 23•10 years ago
|
||
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.
Assignee | ||
Comment 24•10 years ago
|
||
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`.
Assignee | ||
Comment 25•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8596684 -
Flags: review?(myk)
Assignee | ||
Comment 26•10 years ago
|
||
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•10 years ago
|
||
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.
Flags: needinfo?(nick)
Assignee | ||
Comment 28•10 years ago
|
||
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?
Flags: needinfo?(nick)
Comment 29•10 years ago
|
||
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?
Assignee | ||
Comment 30•10 years ago
|
||
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.
Updated•9 years ago
|
Whiteboard: DesktopWebRT2 → DesktopWebRT2 [dependency: marketplace-partners]
Comment 31•9 years ago
|
||
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 33•9 years ago
|
||
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•9 years ago
|
||
Comment 35•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 36•9 years ago
|
||
Comment on attachment 8596684 [details] [diff] [review]
patch v1
Cancelling this review pending investigation into blocklist tests.
Attachment #8596684 -
Flags: review?(myk)
Comment 37•9 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → WONTFIX
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•