821014 - devicemanagerADB removeDir may return before directory is deleted

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Consider code such as:

        if self.device.dirExists(self.remoteTmpDir):
            self.device.removeDir(self.remoteTmpDir)
        self.device.mkDir(self.remoteTmpDir)

On some devices, when remoteTmpDir initially exists, the end result of executing this is that remoteTmpDir does not exist, at least intermittently. Debugging shows that removeDir was called and issued rm -r, but the mkdir failed because the directory existed; immediately afterwards, the directory does not exist.

It appears that rm -r is still executing when mkdir is called.

(Regression from bug 807841 that introduced the "rm -r" directory removal.)

Comment 1

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #0)
> Consider code such as:
> 
>         if self.device.dirExists(self.remoteTmpDir):
>             self.device.removeDir(self.remoteTmpDir)
>         self.device.mkDir(self.remoteTmpDir)
> 
> On some devices, when remoteTmpDir initially exists, the end result of
> executing this is that remoteTmpDir does not exist, at least intermittently.
> Debugging shows that removeDir was called and issued rm -r, but the mkdir
> failed because the directory existed; immediately afterwards, the directory
> does not exist.
> 
> It appears that rm -r is still executing when mkdir is called.
> 
> (Regression from bug 807841 that introduced the "rm -r" directory removal.)

This problem is because _runCmd returns a Popen object, which you're supposed to wait on. We don't always do that, hence we have silly race conditions that can result in things happening in the wrong order. I remember a b2g-related bug was filed about a similar issue a while back, not sure what happened with it (probably nothing).

Comment 2

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: read output from rm -r command

Simply reading the command output seems to effectively ensure that the directory is deleted before returning from removeDir.

(I also deleted unreferenced function _removeSingleDir() -- just to clean up.)

Comment 3

[William Lachance (:wlach)]

Comment on attachment 691506 [details] [diff] [review]
read output from rm -r command

Ok, so I'm going to give this an r+, but only on the condition that you take my advice: :)

I really don't think reading the output (into an unused variable at that) is the right approach here. I remember that was one of the things that was causing problems in b2g (the read would finish before the process was finished). 

You can easily just wait for the process to finish with Popen's wait() function:

self._runCmd(["shell", "rm", "-r", remoteDir]).wait()

Comment 4

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: updated for review comments

:wlach - thanks! That is tidier and just as effective. Here is the updated patch. Would you mind checking it in for me?

Comment 5

[William Lachance (:wlach)]

Pushed, thanks!

https://github.com/mozilla/mozbase/commit/66e6c444a5c43a6f28754dc2160f6335610c994a