Closed Bug 817881 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Plug-ins, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [metro-it2])

Attachments

(2 files, 7 obsolete files)

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.
Blocks: metro-build
Component: Developer Tools → Plug-ins
Product: Firefox → Core
Summary: browser_cmd_addon.js fails on elm due to test plugin → Test plugin isn'
Summary: Test plugin isn' → Test plugin isn't getting picked up by the browser for elm builds
Attached patch fix v.1 (obsolete) — Splinter Review
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)
(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.
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.)
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.
Attachment #688258 - Attachment is obsolete: true
Whiteboard: [metro-it2]
Attached patch test fix v.1 (obsolete) — Splinter Review
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.
Attached patch test fix v.1 (obsolete) — Splinter Review
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+
(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.
Attached patch test fix v.2 (obsolete) — Splinter Review
r? bsmedberg -> xpshell test related changes
Attachment #689763 - Flags: review?(benjamin)
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)
Attached patch test fix v.2 (obsolete) — Splinter Review
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
Attached patch test fix v.3Splinter Review
Attachment #689804 - Attachment is obsolete: true
Attachment #689928 - Flags: review?(benjamin)
Attached patch mozapps patch (obsolete) — Splinter Review
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.
(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?
Attachment #689706 - Attachment is obsolete: true
Attached patch test_plugins.js fix (obsolete) — Splinter Review
Attachment #690849 - Attachment is obsolete: true
Attachment #691532 - Flags: review?(benjamin)
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)
updated.
Attachment #691532 - Attachment is obsolete: true
Attachment #691533 - Flags: review?(dtownsend+bugmail)
Attachment #691533 - Flags: review?(dtownsend+bugmail) → review+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.