1347358 - Mutex directory added in bug 921063 never removed as intended

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, enhancement)

Description

[Mike Hommey [:glandium]]

The code added in bug 921063 does:

if (mRemoteLockDir) {
      mRemoteLock.Unlock();
      mRemoteLockDir->Remove(false);
}

And that is meant to remove the lock directory.

Except, in practice, that doesn't happen:

14434 rmdir("/tmp/firefox_glandium")    = -1 ENOTEMPTY (Directory not empty)

The reason is that nsProfileLock creates a .parentlock in there, but never removes it.

A "simple" solution would be to replace the false with a true, which removes the directory recursively, but if for some reason there is a real directory with that name with unrelated stuff in there, it would also be emptied.

gcp, can you take this, as a followup to bug 921063?

Comment 1

[Gian-Carlo Pascutto [:gcp]]

(In reply to Mike Hommey [:glandium] from comment #0)
> A "simple" solution would be to replace the false with a true, which removes
> the directory recursively, but if for some reason there is a real directory
> with that name with unrelated stuff in there, it would also be emptied.
>

We specifically try to construct this with a unique path for the application + user (which lead to that other bug), and it's in /tmp (OS_TemporaryDirectory), so I believe the simple solution here is acceptable. Files in /tmp aren't expected to survive long anyway - though that behavior seems to have eroded in recent Ubuntu, regrettably.

That said, I think nsProfileLock leaves that file behind on purpose. At least I seemed to have observed this when writing TestProfileLock.cpp. Going to take a look if I can see why...

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Presumably nsProfileLock was actually meant for the profile, so it didn't have to care for (completely) cleaning up after itself.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 1347358 - Add a Cleanup() function for profile locks.

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

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8847710 [details]
Bug 1347358 - Add a Cleanup() function for profile locks.

https://reviewboard.mozilla.org/r/120638/#review122736

::: toolkit/profile/nsProfileLock.cpp:547
(Diff revision 1)
> +    // Remember the name we're using so we can clean up
> +    rv = lockFile->Clone(getter_AddRefs(mLockFile));
> +    if (NS_FAILED(rv))
> +        return rv;

I don't think this should be XP_UNIX only.

::: toolkit/profile/nsProfileLock.cpp:667
(Diff revision 1)
>      }
>  
>      return rv;
>  }
> +
> +#if defined(XP_UNIX)

You're making this ifdef XP_UNIX, but you're calling it unconditionally in the gtest.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

(In reply to Mike Hommey [:glandium] from comment #4)
> > +#if defined(XP_UNIX)
> 
> You're making this ifdef XP_UNIX, but you're calling it unconditionally in
> the gtest.

The gtest is Linux only: http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/toolkit/profile/gtest/moz.build#11

Maybe with the Cleanup() stuff it could work elsewhere but Linux is the only platform where nsProfileLock is used in a way we are interested in cleaning up afterwards.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8847710 [details]
Bug 1347358 - Add a Cleanup() function for profile locks.

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

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8847710 [details]
Bug 1347358 - Add a Cleanup() function for profile locks.

https://reviewboard.mozilla.org/r/120638/#review127078

It would have been easier to review if you had moved the test first in a separate patch, and then add the Cleanup function.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb634954425b
Add a Cleanup() function for profile locks. r=glandium

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eb634954425b