Closed Bug 810810 Opened 7 years ago Closed 7 years ago

xpcshell plugin test failures due to missing test plugin

Categories

(Core :: Plug-ins, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [metro-it1])

Attachments

(1 file, 2 obsolete files)

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:
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.
Attached patch patch v.1 (obsolete) — Splinter Review
per a discussion on irc, touch up the directory provider in xpcshell to return the correct test plugin location.
Assignee: nobody → jmathies
Not convinced the xpcshell.cpp change is granted.
(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
needed.
(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.
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.
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.
(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".
Attached patch dir prov fix (obsolete) — Splinter Review
Attached patch patch v.2Splinter Review
Attachment #681023 - Attachment is obsolete: true
Comment on attachment 681156 [details] [diff] [review]
patch v.2

This passed on try/mc fine.
Attachment #681156 - Flags: review?(benjamin)
Whiteboard: metro-it1
Whiteboard: metro-it1 → [metro-it1]
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.
Attachment #681156 - Flags: review?(mh+mozilla)
Attachment #681156 - Flags: review?(benjamin)
Attachment #681156 - Flags: review+
(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 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.
Attachment #681156 - Flags: review?(mh+mozilla) → review+
Attachment #681155 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ee74c1c99707
https://hg.mozilla.org/mozilla-central/rev/e4cd4714357d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(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.
Depends on: 815699
You need to log in before you can comment on or make changes to this bug.