977786 - nsProfileLock sets mHaveLock to true when locking fails

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Luca Niccoli]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20140205 Firefox/24.0 Iceweasel/24.3.0 (Nightly/Aurora)
Build ID: 20140205091623

Steps to reproduce:

Call nsProfileLock::Lock on a locked directory


Actual results:

nsProfileLock::Lock returns NS_ERROR_FILE_ACCESS_DENIED, but mHaveLock is set to true. Trying to call nsProfileLock::Lock again causes a warning at http://mxr.mozilla.org/mozilla-central/source/profile/dirserviceprovider/src/nsProfileLock.cpp#444


Expected results:

mHaveLock should be set to true only if locking succeeds

Comment 1

[Luca Niccoli]

Attachment: mutexlock.patch

This patch checks if locking has succeded before setting mHaveLock to true; also it removes two superflous "mHaveLock = true" for clarity.

Comment 2

[Benjamin Smedberg]

Comment on attachment 8383246 [details] [diff] [review]
mutexlock.patch

I'll have to apply this and check out the context/control flow in order to review this, so it'll be Monday or Tuesday until I can get to it.

Comment 3

[Luca Niccoli]

Sure, there's no hurry since the way nsProfileLock is used now the bug is latent (if locking fails the first time it is never tried again).
I realize I wasn't really clear in the bug report though: the problem at http://mxr.mozilla.org/mozilla-central/source/profile/dirserviceprovider/src/nsProfileLock.cpp#444 is not the warning, but the fact the the function returns a failure without trying again to acquire the lock.

Comment 4

[Luca Niccoli]

Hi Benjamin,

did you have the chance to take a look at this?

Comment 5

[Luca Niccoli]

Hi Mike,

Benjamin seems to be busy, maybe you could take a look at my patch?

Thanks,
Luca

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8383246 [details] [diff] [review]
mutexlock.patch

I'm not the right person to review this.

Comment 7

[Luca Niccoli]

Hi Benjamin,
It's been some time and this bug had splipped under my radar.
Did you have a chance to look at the patch?
If you are too busy, is there someone else I could ask for a review?

Thanks,

Luca

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8383246 [details] [diff] [review]
mutexlock.patch

Gian-Carlo, could you please pick up this review?

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails.

Review commit: https://reviewboard.mozilla.org/r/76890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76890/

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 977786 - Add tests for nsProfileLock.

Review commit: https://reviewboard.mozilla.org/r/76892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76892/

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8788394 [details]
Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails.

https://reviewboard.mozilla.org/r/76890/#review75008

Comment 12

[Julian Hector [:tedd] [:jhector]]

Comment on attachment 8788395 [details]
Bug 977786 - Add tests for nsProfileLock.

https://reviewboard.mozilla.org/r/76892/#review75480

Talked to :gcp on IRC, I have never looked at any tests for a review purpose, this looks good to me give as far as I can tell. If in doubt please ask someone else for review.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8788395 [details]
Bug 977786 - Add tests for nsProfileLock.

https://reviewboard.mozilla.org/r/76892/#review77238

::: toolkit/profile/gtest/TestProfileLock.cpp:12
(Diff revision 1)
> +#include "nsProfileLock.h"
> +#include "nsString.h"
> +
> +TEST(ProfileLock, BasicLock)
> +{
> +    unlink("/tmp/profilelocktest/.parentlock");

It'd be better to use `mkdtemp` to create a directory, there's less chance of things getting in each others' way. If you want to crib some code from Breakpad there's an `AutoTempDir` class there:
https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/common/tests/auto_tempdir.h

(you might actually be able to just include and use that directly if you set `LOCAL_INCLUDES += ['/toolkit/crashreporter/google-breakpad/src/common']`)

::: toolkit/profile/gtest/TestProfileLock.cpp:58
(Diff revision 1)
> +
> +        if (childpid) {
> +            // parent
> +
> +            // force us to lose the race
> +            sleep(1);

This is not a good way to do synchronization. A simple way to accomplish this (since this test is Linux-only) would be `eventfd`:
http://www.sourcexr.com/articles/2013/10/26/lightweight-inter-process-signaling-with-eventfd

::: toolkit/profile/gtest/TestProfileLock.cpp:77
(Diff revision 1)
> +        } else {
> +            // child
> +            rv = myLock.Lock(dir, nullptr);
> +            ASSERT_EQ(NS_SUCCEEDED(rv), true);
> +
> +            for(;;)

This could stand a comment noting that the parent process will `kill` this process, so it doesn't need to terminate.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 8788395 [details]
Bug 977786 - Add tests for nsProfileLock.

https://reviewboard.mozilla.org/r/76892/#review77244

A few small tweaks to ensure reliability and this should be fine.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> (you might actually be able to just include and use that directly if you set
> `LOCAL_INCLUDES += ['/toolkit/crashreporter/google-breakpad/src/common']`)

This includes breakpad_googletest_includes.h which lives in:
./toolkit/crashreporter/google-breakpad/src/breakpad_googletest_includes.h

This includes "testing/gtest/include/gtest/gtest.h", which, well:
morbo@mozwell:~/hg/firefox$ find -name gtest.h
./media/webrtc/trunk/testing/gtest/include/gtest/gtest.h
./security/nss/external_tests/google_test/gtest/include/gtest/gtest.h
./testing/gtest/gtest/include/gtest/gtest.h
./objdir-desktop/dist/include/gtest/gtest.h

only lives with the correct path in media/webrtc/trunk.

So I'm a bit surprised breakpad compiles, like, at all.

Comment 16

[(not currently active) Ted Mielczarek]

Oh, yeah, sorry, we don't actually build any Breakpad tests as part of the Firefox build, and we don't import Breakpad's copy of gtest. I was wrong above, Breakpad expects `/toolkit/crashreporter/google-breakpad/src/` to be in the include path, and it normally has a copy of gtest checked out in `src/testing/gtest`.

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8788395 [details]
Bug 977786 - Add tests for nsProfileLock.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76892/diff/1-2/

Comment 18

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8788395 [details]
Bug 977786 - Add tests for nsProfileLock.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76892/diff/2-3/

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails.

Review commit: https://reviewboard.mozilla.org/r/78778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78778/

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 977786 - Add tests for nsProfileLock.

Review commit: https://reviewboard.mozilla.org/r/78780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78780/

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8791357 [details]
Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails.

https://reviewboard.mozilla.org/r/78778/#review77378

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8791358 [details]
Bug 977786 - Add tests for nsProfileLock.

https://reviewboard.mozilla.org/r/78780/#review77382

::: toolkit/profile/gtest/TestProfileLock.cpp:18
(Diff revision 1)
> +#include "nsProfileLock.h"
> +#include "nsString.h"
> +
> +static void CleanParentLock(const char *tmpdir)
> +{
> +  // nsProfileLock doesn't clean up all it's files

typo: its

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8791358 [details]
Bug 977786 - Add tests for nsProfileLock.

https://reviewboard.mozilla.org/r/78780/#review77384

Comment 24

[(not currently active) Ted Mielczarek]

Comment on attachment 8791358 [details]
Bug 977786 - Add tests for nsProfileLock.

https://reviewboard.mozilla.org/r/78780/#review77400

::: toolkit/profile/gtest/TestProfileLock.cpp:60
(Diff revision 1)
> +}
> +
> +TEST(ProfileLock, RetryLock)
> +{
> +  char templ[] = "/tmp/profilelocktest.XXXXXX";
> +  char * tmpdir = mkdtemp(templ);

You want to remove the tempdir at the end of each test.

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8791358 [details]
Bug 977786 - Add tests for nsProfileLock.

https://reviewboard.mozilla.org/r/78780/#review77400

> You want to remove the tempdir at the end of each test.

That's what CleanParentLock() does.

Comment 26

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8791358 [details]
Bug 977786 - Add tests for nsProfileLock.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78780/diff/1-2/

Comment 27

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8791357 [details]
Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails.

MozReview <many expletives deleted)

Comment 28

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e42b1a442c466a7e1574e3b92c7bbf7e3b07dc5
Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails. r=gcp

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b9b1f7675ffbf450858dac44146dd8608024f6
Bug 977786 - Add tests for nsProfileLock. r=ted

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0e42b1a442c4
https://hg.mozilla.org/mozilla-central/rev/a8b9b1f7675f

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I'm struggling to find in which testsuite this test is getting executed. I looked at Treeherder by querying via the `gtest` filter, but in the logs I cannot see that this test runs:

https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=gtest&bugfiler

Do I misinterpret the following lines in moz.build?

> +if CONFIG['ENABLE_TESTS']:
> +    DIRS += ['gtest']

Comment 31

[(not currently active) Ted Mielczarek]

No, you're correct. This test only runs on Linux, so looking at the first Linux job I see from your treeherder link:
https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=gtest&bugfiler&selectedJob=154991806

the log shows:
[task 2018-01-09T11:27:52.040Z] 11:27:52     INFO -  TEST-START | ProfileLock.BasicLock
[task 2018-01-09T11:27:52.041Z] 11:27:52     INFO -  TEST-PASS | ProfileLock.BasicLock | test completed (time: 2ms)
[task 2018-01-09T11:27:52.042Z] 11:27:52     INFO -  TEST-START | ProfileLock.RetryLock
[task 2018-01-09T11:27:52.062Z] 11:27:52     INFO -  TEST-PASS | ProfileLock.RetryLock | test completed (time: 21ms)

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, my fault. I was looking for the file name, as what gets logged with all the other harnesses, but haven't used the test name. Thanks Ted!

Comment 33

[(not currently active) Ted Mielczarek]

No worries, gtests are a little weird compared to our other harnesses!