Closed Bug 899857 Opened 7 years ago Closed 6 years ago

APluginsDL provider lists shared plugin directory first

Categories

(Testing :: XPCShell Harness, defect)

x86
Windows 7
defect
Not set

Tracking

(firefox23 fixed, firefox24 fixed, firefox25 fixed, firefox26 fixed, firefox-esr17 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
mozilla26
Tracking Status
firefox23 --- fixed
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed
firefox-esr17 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: mihneadb, Assigned: mihneadb)

References

Details

Attachments

(2 files, 1 obsolete file)

[Context]
We are working on getting the xpcshell tests to run concurrently. In order to achieve this, we changed the xpcshell harness to give every test a plugins directory (bugs 892121 and 890098).

The harness specifies this path to xpcshell using the "-p" cmdline argument. The problem that we encounter is that (at least sometimes) the shared (i.e. $appdir/plugins) folder ends up being the first one in the APluginsDL enumeration, which makes some tests use that folder instead of their own for plugin handling.

The xpcshell (cpp land) adds the $appdir/plugins path on its own, regardless of the "-p" argument (if I'm not mistaken!).

Now, I don't know if changing xpcshell to add $appdir/plugins *ONLY* when there is no -p argument, but I think at least we could make sure that the "-p" specified path ends up first in the enumeration.
Blocks: 887064
To get the path specified with "-p" to appear first, you probably be need to swap the mGREDir and mPluginDir sections in [1].
But as pointed out in bug 890098, this is still fragile - if possible let's add only one path to APluginsDL.

I don't know about the situation where $appdir/plugins is needed, but couldn't we just use "-p" in that case too and drop the mGREDir part?

[1] http://hg.mozilla.org/mozilla-central/annotate/b3fcd828cadc/js/xpconnect/shell/xpcshell.cpp#l1885
Okay, I think the appdir/plugins stuff is just historical cruft, and bug 817881 made it obsolete. We should be able to drop that path from APluginsDL without problems now that we're passing in -p with our correct plugin path.

Incidentally, I think we could probably fix things so we didn't have to do all this file copying in xpcshell nowadays (bug 486570).
Attached patch take out $appdir/plugins (obsolete) — Splinter Review
I m waiting for try to open and then I'll push a try run. After/if it turns
green, I'll flag for r?.
Assignee: nobody → mihneadb
Depends on: 900273
Ok, so this fixes half of the problem. It seems that in automation, in runxpcshelltests.py's context, appdir != mgredir. I filed bug 900273 to have mozharness pass in the actual plugin path. After that lands, we can go on with removing the default location from xpcshell.cpp.
Will have to go ahead this way since it assures a smoother transition (with
mozharness changes and all).
Attachment #783902 - Attachment is obsolete: true
Comment on attachment 785230 [details] [diff] [review]
Make xpcshell default to a plugins directory only if -p is not specified

https://tbpl.mozilla.org/?tree=Try&rev=9e6b4a5450c4
Attachment #785230 - Flags: review?(ted)
Blocks: 900273
No longer depends on: 900273
Attachment #785230 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/7fcf4c1b8463
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
https://hg.mozilla.org/releases/mozilla-beta/rev/4099ce90560f
https://hg.mozilla.org/releases/mozilla-release/rev/231f0dbcfbef

I gave lsblakk a heads-up about this and she was OK with me pushing to m-r. I guess we still need to figure out what to do about b2g18/esr17.
Adds a dummy argument for the b2g18/esr17 branches.
Attachment #786439 - Flags: review?(ted)
Comment on attachment 786439 [details] [diff] [review]
b2g18/esr17 patch

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

This depresses me, but it's the simplest way forward.
Attachment #786439 - Flags: review?(ted) → review+
You need to log in before you can comment on or make changes to this bug.