Closed Bug 1347358 Opened 3 years ago Closed 3 years ago

Mutex directory added in bug 921063 never removed as intended

Categories

(Toolkit :: Startup and Profile System, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: glandium, Assigned: gcp)

References

Details

Attachments

(1 file)

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?
Flags: needinfo?(gpascutto)
Assignee: nobody → gpascutto
Flags: needinfo?(gpascutto)
(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...
Presumably nsProfileLock was actually meant for the profile, so it didn't have to care for (completely) cleaning up after itself.
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.
Attachment #8847710 - Flags: review?(mh+mozilla)
(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 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.
Attachment #8847710 - Flags: review?(mh+mozilla) → review+
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb634954425b
Add a Cleanup() function for profile locks. r=glandium
https://hg.mozilla.org/mozilla-central/rev/eb634954425b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.