489073 - race condition with nsinstall in toolkit/mozapps/plugins/tests Makefile

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[WildcatRay]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090419 Shiretoko/3.5b4pre Firefox/3.0.8
Build Identifier: 

The Trunk Nightly Build failed/busted. See Mozilla-Central tinderbox. It needs to be determined why the building of the nightly failed to complete successfully.

Reproducible: Always

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

The actual error was:

make[8]: Entering directory `/e/builds/moz2_slave/mozilla-central-win32-nightly/build/obj-firefox/toolkit/mozapps/plugins/tests'
d:/mozilla-build/moztools/bin/nsinstall.exe -m 755 GoodPlugin.exe BadPlugin.exe ../../../../dist/bin
d:/mozilla-build/moztools/bin/nsinstall.exe -D ../../../../_tests/testing/mochitest/browser/toolkit/mozapps/plugins/tests
d:/mozilla-build/moztools/bin/nsinstall.exe -D ../../../../_tests/testing/mochitest/browser/toolkit/mozapps/plugins/tests
d:/mozilla-build/moztools/bin/nsinstall.exe "/e/builds/moz2_slave/mozilla-central-win32-nightly/build/toolkit/mozapps/plugins/tests/browser_bug435788.js" "/e/builds/moz2_slave/mozilla-central-win32-nightly/build/toolkit/mozapps/plugins/tests/pfs_bug435788_1.rdf" "/e/builds/moz2_slave/mozilla-central-win32-nightly/build/toolkit/mozapps/plugins/tests/pfs_bug435788_2.rdf" "/e/builds/moz2_slave/mozilla-central-win32-nightly/build/toolkit/mozapps/plugins/tests/GoodExtension.xpi" "/e/builds/moz2_slave/mozilla-central-win32-nightly/build/toolkit/mozapps/plugins/tests/BadExtension.xpi" ../../../../_tests/testing/mochitest/browser/toolkit/mozapps/plugins/tests
cp BadPlugin.exe ../../../../_tests/testing/mochitest/browser/toolkit/mozapps/plugins/tests/BadPlugin
..\..\..\..\_tests\testing\mochitest\browser\toolkit\mozapps\plugins\tests: Could not create the directory: File exists
make[8]: *** [../../../../_tests/testing/mochitest/browser/toolkit/mozapps/plugins/tests/GoodPlugin] Error 3

Comment 2

[(not currently active) Ted Mielczarek]

I think there's a subtle race condition in the Win32 version of nsinstall (bug 463411), tickled by the makefile rule here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/tests/Makefile.in#70

Mossop: do you really need 'cp' there, or will $(INSTALL) work? (nsinstall will create the target directory if it needs to.) Alternately, if you do, we can add a few lines to make this not race, something like:
$(PROGRAMS): $(TESTROOT)
$(TESTROOT):
    $(NSINSTALL) -D $(TESTROOT)

Comment 3

[Chris AtLee [:catlee]]

Last night's build went through ok.

If you need the build from 20090419, let me know.

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #2)
> I think there's a subtle race condition in the Win32 version of nsinstall (bug
> 463411), tickled by the makefile rule here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/tests/Makefile.in#70
> 
> Mossop: do you really need 'cp' there, or will $(INSTALL) work? (nsinstall will
> create the target directory if it needs to.) Alternately, if you do, we can add
> a few lines to make this not race, something like:
> $(PROGRAMS): $(TESTROOT)
> $(TESTROOT):
>     $(NSINSTALL) -D $(TESTROOT)

I seem to remember I couldn't get INSTALL to work right because it only copies/links a file to a directory, while what I need to do here is copy a file to a specific location, the difference being that the copy can have a different name.

Comment 5

[(not currently active) Ted Mielczarek]

Ok, then we should probably just do the Makefile fix for now. Don't care if we morph this bug or file a new one.

Comment 6

[Jim Jeffery not reading bug-mail 1/2/11]

This same crash hit the M-C hourly build as well it seems, so not limited to just 'nightly' builds..

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1240239991.1240246479.25210.gz

Comment 7

[(not currently active) Ted Mielczarek]

Yeah, this is basically the perfect storm of Makefile logic to trigger bug 463411.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: tweak the makefile to avoid the race

bsmedberg is out on paternity leave, so r?Mossop since it's his Makefile. Let me explain what I did here for the non Makefile-inclined.

+$(PROGRAMS) $(_BROWSER_FILES): $(TESTROOT)
+$(TESTROOT):
+       $(NSINSTALL) -D $@

This is what I suggested above, it just makes sure that the rule to create $(TESTROOT) with nsinstall only gets executed once, and gets executed before the rules to copy $(PROGRAMS) and $(_BROWSER_FILES).

 $(PROGRAMS): $(TESTROOT)/% : %$(BIN_SUFFIX)
-       $(NSINSTALL) -D $(TESTROOT)
-       cp $^ $@
+       cp $< $@

$^ is "all prerequisites of this rule", $< is "just the first prerequisite of this rule". Unfortunately making $(TESTROOT) a prerequisite of $(PROGRAMS) gets it listed in $^, so this is a workaround.

-libs:: $(_BROWSER_FILES)
-       $(INSTALL) $(foreach f,$^,"$f") $(TESTROOT)
+libs::
+       $(INSTALL) $(foreach f,$(_BROWSER_FILES),"$(srcdir)/$f") $(TESTROOT)

Same thing here, $(TESTROOT) wound up in $^, so I just explicitly listed the files to install instead. (Having $(_BROWSER_FILES) as a prereq for libs doesn't get you much, since those aren't generated files.)

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 373689 [details] [diff] [review]
tweak the makefile to avoid the race

Sounds reasonable to me

Comment 10

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/e795ecf4adbb