Test plugin isn't getting picked up by the browser for elm builds

RESOLVED FIXED in mozilla20

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
mozilla20
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [metro-it2])

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

6 years ago
Off the top of my head I don't see the issue, but I suspect this has something to do with app vs. gre locations.

INFO | runtests.py | Running tests: end.
mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_addon.js | status - Got ERROR, expected VALID
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_addon.js | markup - Got VVVVVVVVVVVVVVEEEEEEEEEEEEEEEEEEEE, expected VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_addon.js | status - Got ERROR, expected VALID
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_addon.js | markup - Got VVVVVVVVVVVVVEEEEEEEEEEEEEEEEEEEE, expected VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_addon.js | arg.name.value - Got undefined, expected Test Plug-in
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_addon.js | arg.name.status - Got ERROR, expected VALID
(Assignee)

Comment 1

6 years ago
Moving this, the root gre plugins dir isn't getting picked up by the browser. We landed a fix for xpcshell for this so all of our plugin tests succeed. But browser chrome tests that leverage this will fail. I guess the dev toolbar was the first to rely on it.
Blocks: 755724
Component: Developer Tools → Plug-ins
Product: Firefox → Core
(Assignee)

Updated

6 years ago
Summary: browser_cmd_addon.js fails on elm due to test plugin → Test plugin isn'
(Assignee)

Updated

6 years ago
Summary: Test plugin isn' → Test plugin isn't getting picked up by the browser for elm builds
(Assignee)

Comment 2

6 years ago
Created attachment 688258 [details] [diff] [review]
fix v.1

Currently on mc nsAppFileLocationProvider adds the common app / gre dir when nsPluginHost polls for NS_APP_PLUGINS_DIR_LIST. However on elm we lose the gre path which is where we keep the test plugin.

Alternatively we could probably move the test plugin, but we would have to copy it out to multiple app dirs, which seems like a pain compared to this fix.
Assignee: nobody → jmathies
Attachment #688258 - Flags: review?(mh+mozilla)
Comment on attachment 688258 [details] [diff] [review]
fix v.1

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

::: toolkit/xre/nsXREDirProvider.cpp
@@ +737,5 @@
> +    // nsAppFileLocationProvider will provide the application plugins folder,
> +    // we provide the gre path in case the two locations are different.
> +    LoadDirIntoArray(mGREDir,
> +                     kAppendPlugins,
> +                     directories);

I'm afraid this will make us go through all plugins in that directory twice in the currently common case where the two directories are equal, and possibly initialize plugins twice.

But maybe I'm wrong, so punting to someone who probably knows.
Attachment #688258 - Flags: review?(mh+mozilla) → review?(benjamin)
(Assignee)

Comment 4

6 years ago
(In reply to Mike Hommey [:glandium] from comment #3)
> Comment on attachment 688258 [details] [diff] [review]
> fix v.1
> 
> Review of attachment 688258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/xre/nsXREDirProvider.cpp
> @@ +737,5 @@
> > +    // nsAppFileLocationProvider will provide the application plugins folder,
> > +    // we provide the gre path in case the two locations are different.
> > +    LoadDirIntoArray(mGREDir,
> > +                     kAppendPlugins,
> > +                     directories);
> 
> I'm afraid this will make us go through all plugins in that directory twice
> in the currently common case where the two directories are equal, and
> possibly initialize plugins twice.
> 
> But maybe I'm wrong, so punting to someone who probably knows.

I can test for that. If this is the case I think we could just update LoadDirIntoArray such that it doesn't add duplicates.
(Assignee)

Comment 5

6 years ago
(In reply to Jim Mathies [:jimm] from comment #4)
> > I'm afraid this will make us go through all plugins in that directory twice
> > in the currently common case where the two directories are equal, and
> > possibly initialize plugins twice.
> > 
> > But maybe I'm wrong, so punting to someone who probably knows.
> 
> I can test for that. If this is the case I think we could just update
> LoadDirIntoArray such that it doesn't add duplicates.

oh, nm, 'directories' is local.
(Assignee)

Comment 6

6 years ago
Comment on attachment 688258 [details] [diff] [review]
fix v.1

Yes duplicates the directories. It doesn't look like nsAppFileLocationProvider has access to the gre path to do filtering. It does have the bin directory (NS_XPCOM_CURRENT_PROCESS_DIR) but from my understanding that can be the app or gre.
Attachment #688258 - Flags: review?(benjamin)
I assume this is only broken running the tests from the objdir, because the test plugin is installed to $somewhere/plugins, and that's not where the plugin host is looking?

An alternate fix here would be to simply make the in-tree testsuite targets (mochitest-* etc) use the same strategy as the packaged test targets: pass --extra-profile-file=/path/to/plugins, which causes the test plugin to get copied to $profile/plugins and loaded from there. We could stage the test plugin at a known location ($(DIST)/_tests/plugins or something) to avoid the app vs. GRE confusion.)
(Assignee)

Comment 8

6 years ago
We can fixup tests, but how about the browser? Don't we want (install location)/plugins scanned? I would think 3rd parties might install plugins there, but maybe we've restricted them from doing so.
(Assignee)

Comment 9

6 years ago
This also breaks loading the test plugin in local builds, which is manually fixable but kind of a pain.

Comment 10

6 years ago
Realistically I don't want to scan any of gre/plugins, appdir/plugins, installdir/plugins or profile/plugins. But we rely on profile/plugins currently, so let's keep that and ditch the others, if we can.

If we can't, let's just do the most expedient thing.
(Assignee)

Updated

6 years ago
Attachment #688258 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [metro-it2]
(Assignee)

Comment 11

5 years ago
Created attachment 689667 [details] [diff] [review]
test fix v.1

This does a few things:

- moves the test plugins to $(dist)/plugins
- adds the extra profile path for various mochitest targets
- adds a new '--test-plugin-path' param for xpcshell for specifying the new location.
- provides test-plugin-path for various xpcshell targets
- fixups some plugin test logic which was assuming gre/plugins for test plugins

Pushed to try to look for issues.
(Assignee)

Comment 12

5 years ago
Created attachment 689706 [details] [diff] [review]
test fix v.1

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=69d0052ad184

Still finishing up, but it looks like a clean run. I also did a few spot checks in locally run tests that rely on the test plugin.
Attachment #689667 - Attachment is obsolete: true
Attachment #689706 - Flags: review?(ted)
Comment on attachment 689706 [details] [diff] [review]
test fix v.1

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

::: dom/plugins/base/Makefile.in
@@ +124,3 @@
>  	$(NSINSTALL) -D $@
>  
> +export:: $(DIST)/plugins

If you use INSTALL_TARGETS you shouldn't need these anymore. Mind cleaning that up while you're here?

::: dom/plugins/test/testplugin/testplugin.mk
@@ +84,2 @@
>  else
> +	$(INSTALL) $(SHARED_LIBRARY) $(DIST)/plugins

You should be able to replace these all with INSTALL_TARGETS now:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1519

::: js/xpconnect/shell/xpcshell.cpp
@@ -99,1 @@
>      bool SetGREDir(const char *dir);

You'll need an XPCOM peer to sign off on xpcshell changes.

@@ +2157,5 @@
> +        // unpacking test zips.
> +        if (mGREDir) {
> +          mGREDir->Clone(getter_AddRefs(file));
> +          file->AppendNative(NS_LITERAL_CSTRING("plugins"));
> +          dirs.AppendObject(file);

You should probably check whether this directory exists here and not bother if it doesn't.

::: testing/xpcshell/runxpcshelltests.py
@@ +625,5 @@
>        test directories to run.
>      |testdirs|, if provided, is a list of absolute paths of test directories.
>        No-manifest only option.
>      |testPath|, if provided, indicates a single path and/or test to run.
> +    |pluginsPath|, if provided, plugins directoryto be  returned from the dir svc provider.

nit: spacing
Attachment #689706 - Flags: review?(ted) → review+
(Assignee)

Comment 14

5 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> Comment on attachment 689706 [details] [diff] [review]
> test fix v.1
> 
> Review of attachment 689706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/Makefile.in
> @@ +124,3 @@
> >  	$(NSINSTALL) -D $@
> >  
> > +export:: $(DIST)/plugins
> 
> If you use INSTALL_TARGETS you shouldn't need these anymore. Mind cleaning
> that up while you're here?

I think we need this logic in base. The stage zip logic relies on dist/plugins getting created even if we don't build the test plug, and buildbot relies on the zip operation succeeding so that there's a plugins folder in the tests zip during xpcshell test setup. Remocving these directives in dom/plguins/base results in a build error on android - 

https://tbpl.mozilla.org/php/getParsedLog.php?id=17707738&tree=Try&full=1#error0

I've updated testplugin.mk to use INSTALL_TARGETS.
(Assignee)

Comment 15

5 years ago
Created attachment 689763 [details] [diff] [review]
test fix v.2

r? bsmedberg -> xpshell test related changes
Attachment #689763 - Flags: review?(benjamin)
(Assignee)

Comment 16

5 years ago
Comment on attachment 689763 [details] [diff] [review]
test fix v.2

This is failing on mac, needs a little work.
Attachment #689763 - Flags: review?(benjamin)
(Assignee)

Comment 17

5 years ago
Created attachment 689804 [details] [diff] [review]
test fix v.2

This should work but I'll wait until the mac try build finishes to be sure. I made the mistake of thinking that mac plist file and the shared lib both had to be copied to two different locations, and I assumed $(category)_DIST supported multiple paths. So turns out the plist goes in one dir and the shared lib in another. This patch fixes that. I alo kept the change I made to rules.mk that allows multiple paths in $(category)_DIST which might be useful.
Attachment #689763 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
Created attachment 689928 [details] [diff] [review]
test fix v.3
Attachment #689804 - Attachment is obsolete: true
Attachment #689928 - Flags: review?(benjamin)
(Assignee)

Comment 19

5 years ago
Created attachment 690849 [details] [diff] [review]
mozapps patch

There's another copy of get_test_plugin() over in mozapps that needs updating as well.
Comment on attachment 689928 [details] [diff] [review]
test fix v.3

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

::: config/rules.mk
@@ +1547,5 @@
>  	$$(call install_cmd,$(4) $$< $${@D})
>  endef
>  $(foreach category,$(INSTALL_TARGETS),\
>    $(if $($(category)_DEST),,$(error Missing $(category)_DEST))\
> +  $(foreach dest,$($(category)_DEST),\

Why do you need this change? afaics, nothing in this patch requires it.
(Assignee)

Comment 22

5 years ago
(In reply to Mike Hommey [:glandium] from comment #20)
> Comment on attachment 689928 [details] [diff] [review]
> test fix v.3
> 
> Review of attachment 689928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/rules.mk
> @@ +1547,5 @@
> >  	$$(call install_cmd,$(4) $$< $${@D})
> >  endef
> >  $(foreach category,$(INSTALL_TARGETS),\
> >    $(if $($(category)_DEST),,$(error Missing $(category)_DEST))\
> > +  $(foreach dest,$($(category)_DEST),\
> 
> Why do you need this change? afaics, nothing in this patch requires it.

I added that but ended up not needing it. Seemed like it might be useful so I left it in. I can take it out of the final patch if need be.
(In reply to Jim Mathies [:jimm] from comment #22)
> I added that but ended up not needing it. Seemed like it might be useful so
> I left it in. I can take it out of the final patch if need be.

I'd prefer build system changes not to land hidden in a seemingly unrelated changeset.
(Assignee)

Comment 24

5 years ago
We still have one problem on elm with an extension test that uses the test plugin. The pertinent part of the test is here - 

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_plugins.js#99

We've updated these checks once already to deal with breakage. Moving the test plugin breaks them again. The scope results here come from a bit of code in the PluginProvider.jsm - 

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#286

The results are different depending on the os, and for unix based systems depending on where the obj dir resides.

I'm working up a patch that touches up this testper platform. I'm tempted to just delete this part of the test entirely though since the results can vary based on whether the test is run via automation and for local test runs, how the user has their build set up. Anyone else have an opinion on the need for these scope checks?
(Assignee)

Updated

5 years ago
Attachment #689706 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
Created attachment 691532 [details] [diff] [review]
test_plugins.js fix
Attachment #690849 - Attachment is obsolete: true
Attachment #691532 - Flags: review?(benjamin)
(Assignee)

Comment 26

5 years ago
Comment on attachment 691532 [details] [diff] [review]
test_plugins.js fix

Mossop suggests just killing this part of the test off.
Attachment #691532 - Flags: review?(benjamin)
(Assignee)

Comment 27

5 years ago
Created attachment 691533 [details] [diff] [review]
test_plugins.js fix

updated.
Attachment #691532 - Attachment is obsolete: true
Attachment #691533 - Flags: review?(dtownsend+bugmail)
Attachment #691533 - Flags: review?(dtownsend+bugmail) → review+
(Assignee)

Comment 28

5 years ago
Comment on attachment 689928 [details] [diff] [review]
test fix v.3

Swiching out reviewers w/bsmedberg's approval. Also, note per comment 23, I'll remove the unused logic for $(category)_DEST) in the final patch.
Attachment #689928 - Flags: review?(benjamin) → review?(mh+mozilla)
Comment on attachment 689928 [details] [diff] [review]
test fix v.3

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

r+, provided you take a look at local xpcshell results on mac.

::: config/makefiles/xpcshell.mk
@@ +48,5 @@
>  	  --tests-root-dir=$(testxpcobjdir) \
>  	  --testing-modules-dir=$(DEPTH)/_tests/modules \
>  	  --xunit-file=$(testxpcobjdir)/$(relativesrcdir)/results.xml \
>  	  --xunit-suite-name=xpcshell \
> +	  --test-plugin-path=$(DIST)/plugins \

Considering the changes in testplugin.mk, is $(DIST) really appropriate here? For instance, i'd expect failures when running tests locally from an objdir on mac (while it will work fine on test bots).

::: dom/plugins/base/Makefile.in
@@ +124,3 @@
>  	$(NSINSTALL) -D $@
>  
> +export:: $(DIST)/plugins

You can actually remove both these rules, the directory will be created when copying files over.

::: dom/plugins/test/testplugin/testplugin.mk
@@ +66,5 @@
> +ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
> +MAC_PLIST_FILES += $(srcdir)/Info.plist
> +endif
> +
> +ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)

You can group both conditions

::: js/xpconnect/shell/xpcshell.cpp
@@ +1306,5 @@
> +          // plugins path
> +          char *pluginPath = argv[++i];
> +          nsCOMPtr<nsIFile> pluginsDir;
> +          if (NS_FAILED(XRE_GetFileFromPath(pluginPath, getter_AddRefs(pluginsDir)))) {
> +              printf("Couldn't use given plugins dir.\n");

fprintf(gErrFile,
Attachment #689928 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/e18421c16c7a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.