890098 - cannot run dom/plugins xpcshell tests concurrently

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Mihnea Dobrescu-Balaur (:mihneadb)]

Getting intermittent failures from test_persist_in_prefs and test_bug813245

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

A bit of context: this is with the patch in bug 887054. Every test gets its own process and profile dir.

Comment 2

[Georg Fritzsche [:gfritzsche]]

What failures? Can you share links to try runs or logs?

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: log of the failures

Here's an example log.

Comment 4

[Benjamin Smedberg]

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

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

Ok, now I'm confused. These tests fail locally for me even with no patches applied (did a pull & rebuild as well).

Comment 6

[Georg Fritzsche [:gfritzsche]]

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?

Comment 7

[Georg Fritzsche [:gfritzsche]]

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.

Comment 8

[(not currently active) Ted Mielczarek]

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.

Comment 9

[Georg Fritzsche [:gfritzsche]]

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?

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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

Comment 11

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

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?

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

The patch in bug 892121 should fix this.

Comment 14

[Georg Fritzsche [:gfritzsche]]

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.

Comment 15

[Mihnea Dobrescu-Balaur (:mihneadb)]

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?

Comment 16

[Georg Fritzsche [:gfritzsche]]

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.

Comment 17

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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)

Comment 18

[Mihnea Dobrescu-Balaur (:mihneadb)]

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.

Comment 19

[Mihnea Dobrescu-Balaur (:mihneadb)]

same in test_persist_in_prefs

Comment 20

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use tempdir

Changed the use of parentdir (which is outside of the sandbox) to tempdir.

Comment 21

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://tbpl.mozilla.org/?tree=Try&rev=b9322c8ec422

Comment 22

[Georg Fritzsche [:gfritzsche]]

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 :)

Comment 23

[Benjamin Smedberg]

Comment on attachment 778192 [details] [diff] [review]
use tempdir

nsIFile.moveTo modifies the nsIFile object? That's kinda idiotic, but not your fault.

Comment 24

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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. :)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79c68a773080

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79c68a773080

Comment 27

[Mihnea Dobrescu-Balaur (:mihneadb)]

This causes intermittent failures when run with the parxpc patch because we don't pass in pluginsPath in automation. Patch coming right up.

Comment 28

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: [follow up] always set up the plugins dir

Comment 29

[Mihnea Dobrescu-Balaur (:mihneadb)]

Try run : https://tbpl.mozilla.org/?tree=Try&rev=ce1af2994b9f

Comment 30

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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.

Comment 31

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: [follow up] always set up the plugins dir

http://mxr.mozilla.org/build/source/mozharness/scripts/desktop_unittest.py#285 says
we want $appdir/plugins

Comment 32

[Mihnea Dobrescu-Balaur (:mihneadb)]

New try run: https://tbpl.mozilla.org/?tree=Try&rev=6942d7ec4fd7

Comment 33

[Mihnea Dobrescu-Balaur (:mihneadb)]

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.

Comment 34

[Mihnea Dobrescu-Balaur (:mihneadb)]

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.

Comment 35

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://tbpl.mozilla.org/?tree=Try&rev=d8a7bba197d5

Comment 36

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: [follow up] always set up the plugins dir

Waiting for the try run to see if this is ok.

Comment 37

[Mihnea Dobrescu-Balaur (:mihneadb)]

And a try run with ../plugins to make sure
https://tbpl.mozilla.org/?tree=Try&rev=c52312fb6cf9

Comment 38

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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 39

[Mihnea Dobrescu-Balaur (:mihneadb)]

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!

Comment 40

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: [followup] always set up the plugins dir

Missed an issue with the remote harness. Should be fine now, try run
here: https://tbpl.mozilla.org/?tree=Try&rev=641ecfa766a1

Comment 41

[Georg Fritzsche [:gfritzsche]]

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.

Comment 42

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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.

Comment 43

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 44

[Jonathan Griffin (:jgriffin)]

I really don't know this code well at all; how can we verify that these changes are harmless?

Comment 45

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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

Comment 46

[Jonathan Griffin (:jgriffin)]

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.

Comment 47

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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?

Comment 48

[Jonathan Griffin (:jgriffin)]

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?

Comment 49

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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.

Comment 50

[Mihnea Dobrescu-Balaur (:mihneadb)]

(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.

Comment 51

[Jonathan Griffin (:jgriffin)]

Nice, thanks.

Comment 52

[Mihnea Dobrescu-Balaur (:mihneadb)]

Only the follow up patch needs to be checked in.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa4f478c76d

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7fa4f478c76d