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

RESOLVED FIXED in Firefox 20

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20 fixed)

Details

Attachments

(1 attachment)

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.
Assignee: nobody → bmo
Status: NEW → ASSIGNED
Posted patch Patch v1Splinter Review
This is what you had in mind? :-)
Attachment #693170 - Flags: review?(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
Attachment #693170 - Flags: review?(gps) → review+
Fixed nit:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3067bf264c5f

Will need to port the automationutils.py change to mozcrash too.
Blocks: 839514
https://hg.mozilla.org/mozilla-central/rev/3067bf264c5f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 844902
No longer depends on: 844902
Depends on: 844797
You need to log in before you can comment on or make changes to this bug.