Closed
Bug 982101
Opened 11 years ago
Closed 11 years ago
Automatically activate plugins that are bundled inside of extensions
Categories
(Core Graveyard :: Plug-ins, defect, P2)
Core Graveyard
Plug-ins
Tracking
(firefox30+ verified, firefox31 verified, relnote-firefox 30+)
VERIFIED
FIXED
mozilla31
People
(Reporter: gfritzsche, Assigned: gfritzsche)
References
Details
(Whiteboard: p=3 s=it-31c-30a-29b.3 [qa!])
Attachments
(2 files, 6 obsolete files)
11.74 KB,
patch
|
gfritzsche
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
11.94 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
Per bug 982028, comment 9 and 10, we could use a module that allows you to set plugin permissions per site (and maybe globally as well) - possible implementation details can be found there.
Ideally that would handle recovering the previous state properly when the addon is disabled or uninstalled.
Assignee | ||
Updated•11 years ago
|
Blocks: click-to-play
Updated•11 years ago
|
Blocks: fxdesktoptriage
Comment 1•11 years ago
|
||
After a discussion with cweiner, I'm going to morph this a little bit. What we want to do for now is to automatically activate plugins that are bundled inside an addon. The user has already chosen to install the addon and so there's no need to provide an extra activation step in these cases.
Component: General → Plug-ins
Product: Add-on SDK → Core
Summary: Add module for plugin permissions → Automatically activate plugins that are bundled inside of extensions
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → georg.fritzsche
Assignee | ||
Comment 2•11 years ago
|
||
* we want a separate pref for the default plugin state when they are loaded from extensions
* we should be able to tell whether plugins are loaded from extensions by checking against extension directory lists (probably XRE_EXTENSIONS_DIR_LIST?) when state-checking or scanning plugins
* restartless addons may behave differently here
Assignee | ||
Updated•11 years ago
|
Priority: -- → P2
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•11 years ago
|
||
So, i think for test coverage we actually need to figure out how to build an extension as part of our build process.
Otherwise we would have to add an xpi to the tree that contains a test plugin for all the supported platform, which seems rather awkward.
Assignee | ||
Comment 4•11 years ago
|
||
This is still missing testing (still need to figure out how to do tests for extensions that are not restartless).
Apparently we never had support for plugins in restartless extensions, so we can ignore that.
Assignee | ||
Comment 5•11 years ago
|
||
Proper build integration for addons will be bug 988938, but that is probably too far off for this bug.
Comment 6•11 years ago
|
||
Oof, please file a followup for plugins in restartless addons: it should be possible and it would make the transition story better.
Assignee | ||
Comment 7•11 years ago
|
||
It looks like i have the testing nearly worked after a few bumps.
The last thing i need to clear up is why we end up with the plugin path prefixed with "/private", e.g. in xpcshell:
/private/var/folders/tp/mgx9wlds46j6q3ck3xhjzm2w0000gn/T/tmpfonkn2/extensions/test-plugin-from-xpi@tests.mozilla.org/plugins/Test.plugin
... and how to properly compare that path against the paths from XRE_EXTENSIONS_DIR_LIST.
Updated•11 years ago
|
Whiteboard: p=0
Updated•11 years ago
|
Whiteboard: p=0 → p=8
Updated•11 years ago
|
Whiteboard: p=8 → p=3 s=it-31c-30a-29b.2
Comment 8•11 years ago
|
||
Jorge, do you know if there are any real-world extensions we should be testing for this so we can verify the fix? Thanks very much!
Flags: needinfo?(jorge)
Whiteboard: p=3 s=it-31c-30a-29b.2 → p=3 s=it-31c-30a-29b.2 [qa+]
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Hi Paul, can you please take ownership of this?
QA Contact: paul.silaghi
Comment 10•11 years ago
|
||
(In reply to Liz Henry :lizzard from comment #8)
> Jorge, do you know if there are any real-world extensions we should be
> testing for this so we can verify the fix? Thanks very much!
Here are a few extensions that include plugins:
https://addons.mozilla.org/addon/ie-tab-2-ff-36/
https://addons.mozilla.org/addon/jazz-plugin/
https://addons.mozilla.org/addon/nplauncher3/
Flags: needinfo?(jorge)
Assignee | ||
Comment 11•11 years ago
|
||
Putting up this without the test for now to make sure that we don't disagree about the implementation.
I will not land this without the (xpcshell) test.
Attachment #8398754 -
Attachment is obsolete: true
Attachment #8405392 -
Flags: review?(benjamin)
Comment 12•11 years ago
|
||
Comment on attachment 8405392 [details] [diff] [review]
Activate plugins that are in extensions
>diff --git a/dom/plugins/base/nsPluginHost.cpp b/dom/plugins/base/nsPluginHost.cpp
>+bool
>+GetPluginIsFromExtension(const nsCOMPtr<nsIFile>& pluginFile,
>+ const nsCOMPtr<nsISimpleEnumerator>& extensionDirs)
>+{
>+ if (!extensionDirs) {
>+ return false;
>+ }
>+
>+ bool hasMore;
>+ while (NS_SUCCEEDED(extensionDirs->HasMoreElements(&hasMore)) && hasMore) {
>+ nsCOMPtr<nsISupports> supports;
>+ nsresult rv = extensionDirs->GetNext(getter_AddRefs(supports));
>+ if (NS_FAILED(rv)) {
>+ continue;
>+ }
>+
>+ nsCOMPtr<nsIFile> extDir(do_QueryInterface(supports, &rv));
>+ if (NS_FAILED(rv)) {
>+ continue;
>+ }
>+
>+ nsCOMPtr<nsIFile> dir;
>+ if (NS_FAILED(extDir->Clone(getter_AddRefs(dir)))) {
>+ continue;
>+ }
>+
>+ if (NS_FAILED(dir->Append(NS_LITERAL_STRING("plugins")))) {
>+ continue;
>+ }
>+
>+ bool exists;
>+ if (NS_FAILED(dir->Exists(&exists)) || !exists) {
>+ continue;
>+ }
>+
>+ bool isDirectory;
>+ if (NS_FAILED(dir->IsDirectory(&isDirectory)) || !isDirectory) {
>+ continue;
>+ }
>+
>+ bool contains;
>+ if (NS_SUCCEEDED(dir->Contains(pluginFile, false, &contains)) && contains) {
>+ return true;
>+ }
The append("plugins") bit and the Exists check are not necessary. Just do extDir->Contains(pluginFile, true)...
>+ nsCOMPtr<nsISimpleEnumerator> extensionDirs = GetExtensionDirectories();
>+ if (!extensionDirs) {
>+ PLUGIN_LOG(PLUGIN_LOG_ALWAYS, ("Could not get extension directories."));
>+ printf("************* Could not get extension directories.\n");
>+ }
nit leftover printf
r=me with these changes
Attachment #8405392 -
Flags: review?(benjamin) → review+
Updated•11 years ago
|
Whiteboard: p=3 s=it-31c-30a-29b.2 [qa+] → p=3 s=it-31c-30a-29b.3 [qa+]
Assignee | ||
Comment 13•11 years ago
|
||
The test itself is fine now - i'm simply circumventing the path issue by making sure that we don't have a plugin with a conflicting name around.
bsmedberg, uploading this WIP so you can take a look:
* i don't know how to get the plugin name without hard-coding
* when building this i run into the oh-so-descriptive:
> ...
> 0:35.09 find.xpt
> 0:35.12 fuel.xpt
> 0:35.24 Makefile:20: *** missing separator. Stop.
> 0:35.24 make[4]: *** [dom/plugins/test/export] Error 2
> 0:35.24 make[4]: *** Waiting for unfinished jobs....
Flags: needinfo?(benjamin)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(benjamin)
Assignee | ||
Comment 14•11 years ago
|
||
Previous issues fixed, now running into the problem that libs:: in Makefile.in is too early - <objdir>/dist/plugins/ isn't build yet when it's hit.
Attachment #8406240 -
Attachment is obsolete: true
Comment 15•11 years ago
|
||
When this rule is run, has the plugin been compiled yet at all? You might need to put this in a subdir after dom/plugins/test/testplugin is finished.
Also why bother with dist/plugins if you could copy it directly from objdir/dom/plugins/test/testplugin ?
Assignee | ||
Comment 16•11 years ago
|
||
This mostly comes down to OSX being special, it actually does build the bundle via an install target:
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/testplugin/testplugin.mk
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> This mostly comes down to OSX being special, it actually does build the
> bundle via an install target
... or at least that was my assumption, not knowing about how our build system really works.
mshal actually confirmed that the subdir approach works fine on OSX too, so i must have had broken Makefiles still lying around or something like that yesterday.
Comment 18•11 years ago
|
||
Georg, here's a potential build fix on top of your patch. It creates an .xpi on linux and should work on mac but I haven't tested it there.
Assignee | ||
Comment 19•11 years ago
|
||
Thanks for the help, there was some potential for confusion for me here.
Now looking good and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=abce49bdd818
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8406348 -
Attachment is obsolete: true
Attachment #8407130 -
Attachment is obsolete: true
Attachment #8407497 -
Flags: review?(benjamin)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8405392 -
Attachment is obsolete: true
Attachment #8407498 -
Flags: review+
Comment 22•11 years ago
|
||
Comment on attachment 8407497 [details] [diff] [review]
Test that a plugin from an addon is not click-to-play
>diff --git a/dom/plugins/test/testaddon/Makefile.in b/dom/plugins/test/testaddon/Makefile.in
>+# This is so hacky. Waiting on bug 988938.
>+addondir = $(srcdir)
Just use srcdir below?
>+testdir = $(abspath $(DEPTH)/_tests/xpcshell/dom/plugins/test/unit/)
>+pluginpath = $(abspath $(DEPTH)/dist/plugins/$(plugin_file_name))
>+stagedir = $(abspath $(DEPTH)/dom/plugins/test/testaddon/_staging)
This should just be $(abspath _staging).
>+libs::
>+ rm -rf $(stagedir)
>+ mkdir -p $(stagedir)/plugins
>+ cp $(addondir)/install.rdf $(stagedir)
>+ cp -r $(pluginpath) $(stagedir)/plugins/
>+ cd $(stagedir) && zip -qr $(testdir)/testaddon.xpi *
Is there a particular reason you chose to use a stagedir instead of zipping twice directly from srcdir and $(DIST)? It doesn't matter much to me except this seems more complicated.
But importantly, you do need to delete $(testdir)/testaddon.xpi before zipping it. Otherwise there maybe files left over in the .xpi file from previous runs.
r=me with at least the rm fixed
Attachment #8407497 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #22)
> Is there a particular reason you chose to use a stagedir instead of zipping
> twice directly from srcdir and $(DIST)? It doesn't matter much to me except
> this seems more complicated.
We do need a specific directory structure (install.rdf & plugins/$pluginfile).
I didn't find a way to directly add to the zip and get that directory structure, so i'm building it in the objdir and zipping it there.
Assignee | ||
Comment 24•11 years ago
|
||
With the changes done i'd prefer for someone else to have another look at this.
* adopt improvements & review comments from bsmedberg
* fix failures with test-packaging on unified OSX builds [1]:
* we need to provide separate test xpis or build unified test plugins
* unified test plugin is awkward in our build system, so this adopts
the xpi naming to include the XPCOM ABI
Looks good locally, need to wait on try push to confirm the proper test-packaging:
https://tbpl.mozilla.org/?tree=Try&rev=28adec55bb18
[1] https://tbpl.mozilla.org/php/getParsedLog.php?id=37945714&tree=Try&full=1#error0
Attachment #8407497 -
Attachment is obsolete: true
Attachment #8408327 -
Flags: review?(benjamin)
Comment 25•11 years ago
|
||
Comment on attachment 8408327 [details] [diff] [review]
Test that a plugin from an addon is not click-to-play
>diff --git a/dom/plugins/test/testaddon/Makefile.in b/dom/plugins/test/testaddon/Makefile.in
>+testdir = $(abspath $(DEPTH)/_tests/xpcshell/dom/plugins/test/unit/)
>+addonpath = $(testdir)/$(addon_file_name)
>+
>+libs::
>+ $(NSINSTALL) -D $(testdir)
>+ rm -f $(addonpath)
>+ cd $(srcdir) && zip -rD $(testdir)/$(addon_file_name) install.rdf
>+ cd $(DIST) && zip -rD $(addonpath) plugins/$(plugin_file_name)
This confused me at first because the first line uses $(testdir)/$(addon_file_name) and the second line uses $(addonpath) which is the same thing. Please use $(addonpath) in both places.
Attachment #8408327 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 26•11 years ago
|
||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/46b65a3ccf6f
https://hg.mozilla.org/mozilla-central/rev/6022ca67befb
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 28•11 years ago
|
||
Hi Paul, confirming if this resolved bug can be verified before the end of the desktop iteration on Monday April 28?
Flags: needinfo?(paul.silaghi)
Comment 29•11 years ago
|
||
(In reply to Marco Mucci [:MarcoM] from comment #28)
> Hi Paul, confirming if this resolved bug can be verified before the end of
> the desktop iteration on Monday April 28?
Yes, it can. I'll start ASAP.
Flags: needinfo?(paul.silaghi)
Comment 30•11 years ago
|
||
IE Tab Plug-in is initially set to 'Ask to Activate' - https://addons.mozilla.org/addon/ie-tab-2-ff-36/
It should be 'Always activate', right ?
Flags: needinfo?(georg.fritzsche)
Assignee | ||
Comment 31•11 years ago
|
||
If the plugin is contained in the xpi, it should be "always activate".
You can check that it is in the xpi at about:plugins, it should be in $extension_folder/plugins/.
Flags: needinfo?(georg.fritzsche)
Assignee | ||
Comment 32•11 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #31)
> in the xpi
"in the extension folder"
Comment 33•11 years ago
|
||
Looks like it is
Path: C:\Users\paul.silaghi\AppData\Roaming\Mozilla\Firefox\Profiles\jhkta1cy.21\extensions\{1BC9BA34-1EED-42ca-A505-6D2F1A935BBB}\plugins\npietab2.dll
Updated•11 years ago
|
status-firefox31:
--- → fixed
Comment 34•11 years ago
|
||
Comment on attachment 8407498 [details] [diff] [review]
Activate plugins that are in extensions
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Needed in order to flip the CtP pref for FF30, bug 992995
Testing completed (on m-c, etc.): landed, QA found one issue fixed in bug 999443 which will also need to be taken
Risk to taking this patch (and alternatives if risky): Fairly low risk.
String or IDL/UUID changes made by this patch: None
Attachment #8407498 -
Flags: approval-mozilla-aurora?
Comment 35•11 years ago
|
||
(In reply to Jorge Villalobos [:jorgev] from comment #10)
> (In reply to Liz Henry :lizzard from comment #8)
> > Jorge, do you know if there are any real-world extensions we should be
> > testing for this so we can verify the fix? Thanks very much!
>
> Here are a few extensions that include plugins:
>
> https://addons.mozilla.org/addon/ie-tab-2-ff-36/
> https://addons.mozilla.org/addon/jazz-plugin/
> https://addons.mozilla.org/addon/nplauncher3/
Also tried with:
http://mail.qq.com/zh_CN/xpi/TencentMailPlugin_1_0_1_54.xpi
http://player.cntv.cn/flashplayer/config/plugins/npCNTVLive2_Win_32_ver1.0.0.7.xpi?3
https://start.ingbusinessonline.pl/login/components/SignPluginInstallIng.xpi
https://net.dnb.pl/static/components/13045/SignPluginInstallNord.xpi
https://webchat.na.collabserv.com/stwebav/20140308.0834/STWebPlayerLL.xpi
The plugins are "Always activate" in Addons Manager.
Verified fixed FF 31.0a1 (2014-04-24) Win 7, Ubuntu 12.10, OS X 10.9.2
Status: RESOLVED → VERIFIED
Whiteboard: p=3 s=it-31c-30a-29b.3 [qa+] → p=3 s=it-31c-30a-29b.3 [qa!]
Updated•11 years ago
|
status-firefox30:
--- → affected
tracking-firefox30:
--- → +
Updated•11 years ago
|
Attachment #8407498 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
relnote-firefox:
--- → ?
Comment 36•11 years ago
|
||
Comment 38•11 years ago
|
||
Have added this to FF30 Desktop notes as a 'developer' note so as to help Add-on devs.
Updated•11 years ago
|
Comment 39•11 years ago
|
||
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Verified as fixed on Firefox 30 beta 5 (Build ID: 20140515140857) with the extensions from comment 10 and:
http://mail.qq.com/zh_CN/xpi/TencentMailPlugin_1_0_1_54.xpi
http://player.cntv.cn/flashplayer/config/plugins/npCNTVLive2_Win_32_ver1.0.0.7.xpi?3https://start.ingbusinessonline.pl/login/components/SignPluginInstallIng.xpi
https://net.dnb.pl/static/components/13045/SignPluginInstallNord.xpi
Keywords: verifyme
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•