813577 - Use try/except/finally in remoteautomation.py and b2gautomation.py to ensure temp directories do not linger

Status: RESOLVED FIXED

Description

[Ed Morley [:emorley]]

Broken out from bug 808410.

(In reply to Gregory Szorc [:gps] from comment #21)
> Comment on attachment 681926 [details] [diff] [review]
> Part 5: Callers of checkForCrashes should use its return value
>
> ::: build/mobile/b2gautomation.py
> @@ +103,5 @@
> >          try:
> >            shutil.rmtree(dumpDir)
> >          except:
> >            print "WARNING: unable to remove directory: %s" % (dumpDir)
> > +        return crashed
> 
> See comment in remoteautomation.py regarding lingering temp directories and
> try..finally.
>
...
> ::: build/mobile/remoteautomation.py
> @@ +100,5 @@
> > +        try:
> > +            shutil.rmtree(dumpDir)
> > +        except:
> > +            print "WARNING: unable to remove directory: %s" % dumpDir
> > +        return crashed
> 
> If getDirectory() or checkForCrashes() raises, the temporary directory will
> linger. I'd use a try..finally to ensure dumpDir is deleted.


(In reply to Joel Maher (:jmaher) from comment #27)
> ::: build/mobile/remoteautomation.py
> @@ +100,5 @@
> > +        try:
> > +            shutil.rmtree(dumpDir)
> > +        except:
> > +            print "WARNING: unable to remove directory: %s" % dumpDir
> > +        return crashed
> 
> to that note, we should have a try/except/finall around the entire
> getDirectory/checkForCrashes code.  Right now we only have it around
> shutil.rmtree.

Comment 1

[Ed Morley [:emorley]]

Attachment: Patch v1

This is what you had in mind? :-)

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 693170 [details] [diff] [review]
Patch v1

Review of attachment 693170 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Sorry it disappeared in my review queue for over a month.

::: build/automationutils.py
@@ +147,5 @@
>      return False
>  
> +  try:
> +    removeSymbolsPath = False
> +    

Nit: leading whitespace

Comment 3

[Ed Morley [:emorley]]

Fixed nit:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3067bf264c5f

Will need to port the automationutils.py change to mozcrash too.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3067bf264c5f

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/releases/mozilla-beta/rev/654850067995