Automatically activate plugins that are bundled inside of extensions

VERIFIED FIXED in Firefox 30

Status

()

defect
P2
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox30+ verified, firefox31 verified, relnote-firefox 30+)

Details

(Whiteboard: p=3 s=it-31c-30a-29b.3 [qa!])

Attachments

(2 attachments, 6 obsolete attachments)

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

5 years ago

Comment 1

5 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
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Assignee

Updated

5 years ago
Assignee: nobody → georg.fritzsche
Assignee

Comment 2

5 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

5 years ago
Priority: -- → P2
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Assignee

Comment 3

5 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

5 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

5 years ago
Proper build integration for addons will be bug 988938, but that is probably too far off for this bug.

Comment 6

5 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

Updated

5 years ago
Blocks: 989967
Assignee

Comment 7

5 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.
Whiteboard: p=0
Whiteboard: p=0 → p=8
Whiteboard: p=8 → p=3 s=it-31c-30a-29b.2
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+]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Hi Paul, can you please take ownership of this?
QA Contact: paul.silaghi
(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

5 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 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+
Whiteboard: p=3 s=it-31c-30a-29b.2 [qa+] → p=3 s=it-31c-30a-29b.3 [qa+]
Assignee

Comment 13

5 years ago
Posted patch Test, WIP (obsolete) — Splinter Review
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

5 years ago
Flags: needinfo?(benjamin)
Assignee

Comment 14

5 years ago
Posted patch Test, WIP (obsolete) — Splinter Review
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
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

5 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

5 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.
Posted patch plugin-extension-buildfix (obsolete) — Splinter Review
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

5 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

5 years ago
Attachment #8406348 - Attachment is obsolete: true
Attachment #8407130 - Attachment is obsolete: true
Attachment #8407497 - Flags: review?(benjamin)
Assignee

Comment 21

5 years ago
Attachment #8405392 - Attachment is obsolete: true
Attachment #8407498 - Flags: review+
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

5 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

5 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 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+
https://hg.mozilla.org/mozilla-central/rev/46b65a3ccf6f
https://hg.mozilla.org/mozilla-central/rev/6022ca67befb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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)
(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)
Depends on: 999400
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

5 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

5 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #31)
> in the xpi

"in the extension folder"
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
Depends on: 999443
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?
(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!]
Attachment #8407498 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flagging for verification in Firefox 30 (now in Beta).
Keywords: verifyme
Have added this to FF30 Desktop notes as a 'developer' note so as to help Add-on devs.
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
You need to log in before you can comment on or make changes to this bug.