Closed Bug 890098 Opened 7 years ago Closed 7 years ago

cannot run dom/plugins xpcshell tests concurrently

Categories

(Core :: Plug-ins, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mihneadb, Assigned: mihneadb)

References

Details

Attachments

(3 files, 3 obsolete files)

Getting intermittent failures from test_persist_in_prefs and test_bug813245
Blocks: 887064
A bit of context: this is with the patch in bug 887054. Every test gets its own process and profile dir.
What failures? Can you share links to try runs or logs?
Flags: needinfo?(mihneadb)
Attached file log of the failures
Here's an example log.
Flags: needinfo?(mihneadb)
This test failure is caused by the test plugin not being present. This in turn is caused by something around http://mxr.mozilla.org/mozilla-central/source/dom/plugins/test/unit/head_plugins.js#62
Ok, now I'm confused. These tests fail locally for me even with no patches applied (did a pull & rebuild as well).
You should have two test plugins in $OBJDIR/dist/plugins - are those present?
Are any other files like nptestcopy.so still lying around from a non-clean test abort?
Given that we have at least one test that moves plugins around, this obviously can't work properly when running concurrently to other tests that use plugins.

What we would need is for every test-runner instances to use a separate plugin directory.
Also, test_persist_in_prefs.js moves plugins out of the way to the parent directory, but we could just change it to use a lazily created sub directory.
Can we fix the tests that are copying plugins around to instead copy them to $profile/plugins? That would fix that issue, presumably. Copying files around in the plugin directory doesn't seem like a great idea in any event.
Good point, but that leaves the problem of moving plugins out of any scanned plugin directory.

The test-runners already specify the plugin directory to be used, so i'd think having it provide a plugin directory specific to the test-runner-instance (e.g. $profile/plugins) would be the logical fix.

Do we have any better option?
(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> Good point, but that leaves the problem of moving plugins out of any scanned
> plugin directory.
> 
> The test-runners already specify the plugin directory to be used, so i'd
> think having it provide a plugin directory specific to the
> test-runner-instance (e.g. $profile/plugins) would be the logical fix.
> 
> Do we have any better option?

You mean this[1]?

[1] http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#1095
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #10)
> You mean this[1]?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/
> runxpcshelltests.py#1095

Yes, thanks - didn't find it again at first glance.
Since we don't scan $appdir/plugins anymore, this passes the directory through which contains the test-plugins, which means this could be switched to non-shared directories rather easily.
So you are saying that before starting each test we should create a tmpdir and copy all of the stuff in the original pluginDir in there?

Wouldn't that be too expensive?
The patch in bug 892121 should fix this.
Depends on: 892121
Worth noting that we ended up discussing that on IRC and deemed the 'plugin dir per test run' in bug 892121 as not too expensive as it only needs to copy two smaller shared libraries/bundles.
So.. that patch fixed it for linux but I still saw this error on windows:

https://tbpl.mozilla.org/php/getParsedLog.php?id=25409498&tree=Try#error2

Any ideas?
Flags: needinfo?(georg.fritzsche)
So, progress - this is a different point of failure:
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/unit/test_persist_in_prefs.js#l84

I don't know off-hand whats wrong here though - need to check tomorrow or monday.
(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> So, progress - this is a different point of failure:
> http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/unit/
> test_persist_in_prefs.js#l84
> 
> I don't know off-hand whats wrong here though - need to check tomorrow or
> monday.

Does this use the system registry somehow on windows? (that would be a shared resource)
so.. test_nice_plugin_name test moves a plugin up one dir from the plugins dir, basically "escaping" the sandbox. Taking out that part seems to fix it. I'll check the other tests in the dir for this as well.
same in test_persist_in_prefs
Attached patch use tempdirSplinter Review
Changed the use of parentdir (which is outside of the sandbox) to tempdir.
Attachment #778192 - Flags: review?(georg.fritzsche)
Assignee: nobody → mihneadb
Comment on attachment 778192 [details] [diff] [review]
use tempdir

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

This looks good to me, over to Benjamin for an actual peer review :)
Attachment #778192 - Flags: review?(georg.fritzsche)
Attachment #778192 - Flags: review?(benjamin)
Attachment #778192 - Flags: feedback+
Comment on attachment 778192 [details] [diff] [review]
use tempdir

nsIFile.moveTo modifies the nsIFile object? That's kinda idiotic, but not your fault.
Attachment #778192 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #23)
> Comment on attachment 778192 [details] [diff] [review]
> use tempdir
> 
> nsIFile.moveTo modifies the nsIFile object? That's kinda idiotic, but not
> your fault.

I was "impressed" by that as well. :)
https://hg.mozilla.org/mozilla-central/rev/79c68a773080
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
This causes intermittent failures when run with the parxpc patch because we don't pass in pluginsPath in automation. Patch coming right up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #780012 - Flags: review?(georg.fritzsche)
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #28)
> Created attachment 780012 [details] [diff] [review]
> [follow up] always set up the plugins dir

I'm not sure if we need dist/plugins or dist/bin/plugins yet. Digging through automation code to find out. Also the try run should tell us that.
Attachment #780012 - Attachment is obsolete: true
Attachment #780012 - Flags: review?(georg.fritzsche)
Comment on attachment 780021 [details] [diff] [review]
[follow up] always set up the plugins dir

This is not ok. Need to figure out the default plugin dir part.
Attachment #780021 - Flags: review?(georg.fritzsche)
Ok, the problem is that the harness is self-tested during the build step, where we have no plugins dir. That's why I'll be adding a check in the setup dir logic.
Waiting for the try run to see if this is ok.
Attachment #780021 - Attachment is obsolete: true
And a try run with ../plugins to make sure
https://tbpl.mozilla.org/?tree=Try&rev=c52312fb6cf9
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #36)
> Created attachment 780095 [details] [diff] [review]
> [follow up] always set up the plugins dir
> 
> Waiting for the try run to see if this is ok.

Ok, try run turned out well so, flagging for r?.
Comment on attachment 780095 [details] [diff] [review]
[follow up] always set up the plugins dir

:jgriffin, we talked about this today and I m not sure who to ask for review since this is actually a change on the harness itself. I asked Ted for the previous changes but he is PTO for a while.

Hope asking you is ok. Thanks!
Attachment #780095 - Flags: review?(jgriffin)
Missed an issue with the remote harness. Should be fine now, try run
here: https://tbpl.mozilla.org/?tree=Try&rev=641ecfa766a1
Attachment #780207 - Flags: review?(jgriffin)
Attachment #780095 - Attachment is obsolete: true
Attachment #780095 - Flags: review?(jgriffin)
Maybe i'm missing something regarding the test-setups/-environments, but isn't it still possible that xpcshell ends up adding both $gredir/plugins and the plugin path to the "APluginsDL" list at [1]?

Or is only one of those supposed to get set? Couldn't we enforce that then?

If both directories are present in that list we might stumble over hard-to-trace-down issues later again, so it would be nice to clear this up.
(In reply to Georg Fritzsche [:gfritzsche] from comment #41)
> Maybe i'm missing something regarding the test-setups/-environments, but
> isn't it still possible that xpcshell ends up adding both $gredir/plugins
> and the plugin path to the "APluginsDL" list at [1]?

I think you forgot to add the link. I thought that xpcshell added that path only when no path provided.

> 
> Or is only one of those supposed to get set? Couldn't we enforce that then?
> 
> If both directories are present in that list we might stumble over
> hard-to-trace-down issues later again, so it would be nice to clear this up.

I think that the "if path contains plugins, skip it" fix would be easiest here, but it is inside the test, not inside xpcshell.
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #42)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #41)
> > Maybe i'm missing something regarding the test-setups/-environments, but
> > isn't it still possible that xpcshell ends up adding both $gredir/plugins
> > and the plugin path to the "APluginsDL" list at [1]?
> 
> I think you forgot to add the link. I thought that xpcshell added that path
> only when no path provided.

Sorry, missing link:
http://hg.mozilla.org/mozilla-central/annotate/b3fcd828cadc/js/xpconnect/shell/xpcshell.cpp#l1885
It adds both $gredir/plugins and $pluginpath if they are both set - but then i don't know when mGREDir is actually used.

> > 
> > Or is only one of those supposed to get set? Couldn't we enforce that then?
> > 
> > If both directories are present in that list we might stumble over
> > hard-to-trace-down issues later again, so it would be nice to clear this up.
> 
> I think that the "if path contains plugins, skip it" fix would be easiest
> here, but it is inside the test, not inside xpcshell.

That seems a little fragile, e.g. i think the mach test runner adds the pluginpath dist/bin/plugins.
I really don't know this code well at all; how can we verify that these changes are harmless?
(In reply to Jonathan Griffin (:jgriffin) from comment #44)
> I really don't know this code well at all; how can we verify that these
> changes are harmless?

So this is just a minor improvement over the changes in bug 892121.

This try run shows that it doesn't break existing behaviour:https://tbpl.mozilla.org/?tree=Try&rev=641ecfa766a1
I'll r+ if you verify it doesn't break the mach runner or the make target that some people still use to execute the tests.
(In reply to Jonathan Griffin (:jgriffin) from comment #46)
> I'll r+ if you verify it doesn't break the mach runner or the make target
> that some people still use to execute the tests.

Ok, turns out that the make target is/was already borken (even without my patch). Problems is: if no --pluginsPath is specified, then xpcshell (from the cpp land) assumes $appdir/plugins as the default place to look for plugins. On local builds (default settings), that means dist/bin/plugins. This is fine for automation, but on local builds plugins end up in dist/plugins by default.

The mach runner is fine because it specifies the --pluginsPath, while the make target does not.

The only solution I see right now is to try both dist/plugins and dist/bin/plugins (if no --pluginsPath is specified) and use whichever exists.


Does that make sense?
I'd be tempted to just update the make target to add --pluginsPath; it's implemented here:  http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#279

Can you think of a reason not to do that?
(In reply to Jonathan Griffin (:jgriffin) from comment #48)
> I'd be tempted to just update the make target to add --pluginsPath; it's
> implemented here: 
> http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.
> mk#279
> 
> Can you think of a reason not to do that?

I was afraid that I wouldn't know how to get the full (platform-independent) path to the plugins dir from there.
Depends on: 898142
(In reply to Jonathan Griffin (:jgriffin) from comment #48)
> I'd be tempted to just update the make target to add --pluginsPath; it's
> implemented here: 
> http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.
> mk#279
> 
> Can you think of a reason not to do that?

Ok, so I filed bug 898142 and added the patch there. After that lands, all 3 ways of running the tests (mach, make and mozharness) will work just fine.
Nice, thanks.
Attachment #780207 - Flags: review?(jgriffin) → review+
Only the follow up patch needs to be checked in.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7fa4f478c76d
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.