810810 - xpcshell plugin test failures due to missing test plugin

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Debugging this a bit, I see head_plugins.js trying to find the test plugin in the /browser folder. After touching this part of the tests up, nsIPluginHost is still having issues. Not quite sure what's going on yet, I need to get a debug build going so I can step through the plugin code.


TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\dom\plugins\test\unit\test_bug455213.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | c:/talos-slave/test/build/xpcshell/tests/dom/plugins/test/unit/head_plugins.js | false == true - See following stack:
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\dom\plugins\test\unit\test_bug471245.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | c:/talos-slave/test/build/xpcshell/tests/dom/plugins/test/unit/test_bug471245.js | true == false - See following stack:

Comment 1

[Jim Mathies [:jimm]]

Some notes - 

- nsPluginHost searches directories retrieved from the NS_APP_PLUGINS_DIR_LIST dir provider.
- The first hit on a provider that supports NS_APP_PLUGINS_DIR_LIST is nsAppFileLocationProvider.
- (the custom xpcshell dir provider is queried first, but doesn't support this)
- nsAppFileLocationProvider returns two directory props for it's list, NS_USER_PLUGINS_DIR and NS_APP_PLUGINS_DIR
- NS_APP_PLUGINS_DIR maps to dist/bin/browser/plugins, which doesn't exist because the test plugin is installed in dist/bin/plugins
- elm's firefox.exe can't find the test plugin either, it goes through the same search process.
- firefox.exe -metrodesktop searches dist/bin/metro/plugins, which is good.

A couple possible solutions - 

We could install the test plugin in the proper app directory when it is built. This fixes the plugin search problem, although we would have to be careful about breaking current mc behavior prior to the landing of bug 755724.

However, I'm guessing we would prefer to search both the app and gre for plugin directories, in which case nsAppFileLocationProvider needs to be updated to add the gre path to it's list of possible plugin locations.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch v.1

per a discussion on irc, touch up the directory provider in xpcshell to return the correct test plugin location.

Comment 3

[Mike Hommey [:glandium]]

Not convinced the xpcshell.cpp change is granted.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Mike Hommey [:glandium] from comment #3)
> Not convinced the xpcshell.cpp change is granted.

granted or needed?

FWIW, this passed on try - 

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=4a9989fe07a3

..and solves some of the plugin errors on Elm, but not all though. I'll revisit the rest of the failures in bit.

https://tbpl.mozilla.org/?noignore=0&tree=Elm&rev=8ee334461013

TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\extensions\test\xpcshell\test_plugins.js | test failed (with xpcshell return code: 0), see following log:
Bug 562886 - test_plugins.js fails (8 == 4) with Linux objdir builds TEST-UNEXPECTED-FAIL | C:/talos-slave/test/build/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_plugins.js | false == true - See following stack:
Bug 562886 - test_plugins.js fails (8 == 4) with Linux objdir builds TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\extensions\test\xpcshell-unpack\test_plugins.js | test failed (with xpcshell return code: 0), see following log:
Bug 562886 - test_plugins.js fails (8 == 4) with Linux objdir builds TEST-UNEXPECTED-FAIL | C:/talos-slave/test/build/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell-unpack/test_plugins.js | false == true - See following stack:
Bug 562886 - test_plugins.js fails (8 == 4) with Linux objdir builds

Comment 5

[Mike Hommey [:glandium]]

needed.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Mike Hommey [:glandium] from comment #5)
> needed.

Well there's no question that we are looking in the wrong place (app dir/plugins) for the test plugin when running xpcshell tests. What would the alternative be? We could move the test plugin to a new location I suppose.

Comment 7

[Mike Hommey [:glandium]]

Ah, looking at head_plugins.js, it looks like it doesn't do what i thought it was doing. So yeah, either we do that in xpcshell.cpp, or we add a directory service in head_plugins.js. There are several other tests that add their own directory service.

Comment 8

[Jim Mathies [:jimm]]

FWIW, the last set of tests fail for a similar reason, test_plugins.js can't find the plugin using CurProcD. Switching to GreD fixes the problem. Also, there's a check in there that tests if the test plugin's scope is AddonManager.SCOPE_APPLICATION, which it isn't, in this case, it's AddonManager.SCOPE_SYSTEM. We can probably just get rid of that test, or test for both values.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Mike Hommey [:glandium] from comment #7)
> Ah, looking at head_plugins.js, it looks like it doesn't do what i thought
> it was doing. So yeah, either we do that in xpcshell.cpp, or we add a
> directory service in head_plugins.js. There are several other tests that add
> their own directory service.

Hrm, this doesn't appear to work. You can add providers, but nsAppFileLocationProvider sits higher in the provider list so it gets called first for "APluginsDL".

Comment 10

[Jim Mathies [:jimm]]

Attachment: dir prov fix

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 681156 [details] [diff] [review]
patch v.2

This passed on try/mc fine.

Comment 13

[Benjamin Smedberg]

Comment on attachment 681156 [details] [diff] [review]
patch v.2

looks ok to me, but glandium should approve this since it could break xpcshell tests of FF-on-XR.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> Comment on attachment 681156 [details] [diff] [review]
> patch v.2
> 
> looks ok to me, but glandium should approve this since it could break
> xpcshell tests of FF-on-XR.

Hmm, maybe we should file a bug on getting that type of build integrated into the tests that run on try.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 681156 [details] [diff] [review]
patch v.2

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

(In reply to Jim Mathies [:jimm] from comment #14)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> > Comment on attachment 681156 [details] [diff] [review]
> > patch v.2
> > 
> > looks ok to me, but glandium should approve this since it could break
> > xpcshell tests of FF-on-XR.

I think it likely fixes them. I am way behind on actually looking at test results on my iceweasel builds :(

> Hmm, maybe we should file a bug on getting that type of build integrated
> into the tests that run on try.

I doubt that would be very useful, especially now, considering FF on elm is essentially the same setup as FF-on-XR.

Comment 16

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4cd4714357d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee74c1c99707

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee74c1c99707
https://hg.mozilla.org/mozilla-central/rev/e4cd4714357d

Comment 18

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #15)
> I think it likely fixes them. I am way behind on actually looking at test
> results on my iceweasel builds :(

For the record, I actually already was applying a similar patch on iceweasel.

I also have a similar change on netwerk/test/unit/test_socks.js, which may be necessary for elm as well.