Closed Bug 977786 Opened 6 years ago Closed 3 years ago

nsProfileLock sets mHaveLock to true when locking fails

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: lultimouomo, Assigned: gcp)

References

Details

(Whiteboard: p=0)

Attachments

(2 files, 3 obsolete files)

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
Attached patch mutexlock.patch (obsolete) — Splinter Review
This patch checks if locking has succeded before setting mHaveLock to true; also it removes two superflous "mHaveLock = true" for clarity.
Attachment #8383246 - Flags: review?(benjamin)
Blocks: 921063
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.
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.
Hi Benjamin,

did you have the chance to take a look at this?
Attachment #8383246 - Flags: review?(mh+mozilla)
Hi Mike,

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

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

I'm not the right person to review this.
Attachment #8383246 - Flags: review?(mh+mozilla)
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: p=0
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 on attachment 8383246 [details] [diff] [review]
mutexlock.patch

Gian-Carlo, could you please pick up this review?
Attachment #8383246 - Flags: review?(benjamin) → review?(gpascutto)
Attachment #8383246 - Attachment is obsolete: true
Attachment #8383246 - Flags: review?(gpascutto)
Comment on attachment 8788394 [details]
Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails.

https://reviewboard.mozilla.org/r/76890/#review75008
Attachment #8788394 - Flags: review?(gpascutto) → review+
Attachment #8788395 - Flags: review?(ted)
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.
Attachment #8788395 - Flags: review?(julian.r.hector) → review+
Assignee: nobody → gpascutto
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 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.
Attachment #8788395 - Flags: review?(ted) → review-
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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.
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`.
Attachment #8788394 - Attachment is obsolete: true
Attachment #8788395 - Attachment is obsolete: true
Attachment #8788395 - Flags: review?(ted)
Comment on attachment 8791357 [details]
Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails.

https://reviewboard.mozilla.org/r/78778/#review77378
Attachment #8791357 - Flags: review?(gpascutto) → review+
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 on attachment 8791358 [details]
Bug 977786 - Add tests for nsProfileLock.

https://reviewboard.mozilla.org/r/78780/#review77384
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.
Attachment #8791358 - Flags: review?(ted) → review+
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.
Attachment #8791357 - Attachment is obsolete: true
Comment on attachment 8791357 [details]
Bug 977786 - nsProfileLock shouldn't set mHaveLock when locking fails.

MozReview <many expletives deleted)
Attachment #8791357 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/0e42b1a442c4
https://hg.mozilla.org/mozilla-central/rev/a8b9b1f7675f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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']
Flags: needinfo?(ted)
Flags: needinfo?(gpascutto)
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)
Flags: needinfo?(ted)
Flags: needinfo?(gpascutto)
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!
No worries, gtests are a little weird compared to our other harnesses!
You need to log in before you can comment on or make changes to this bug.