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

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.

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.

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.

(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.

(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.

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.

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.)

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.

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

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.

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.

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.

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

(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.

r? bsmedberg -> xpshell test related changes

Comment on attachment 689763 [details] [diff] [review] test fix v.2 This is failing on mac, needs a little work.

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.

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.

try push: https://tbpl.mozilla.org/?tree=Try&rev=2163126d4a32

(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.

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?

Comment on attachment 691532 [details] [diff] [review] test_plugins.js fix Mossop suggests just killing this part of the test off.

updated.

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.

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,