devicemanagerADB doesn't close mkstemp() file handles

RESOLVED FIXED in Firefox 39

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: m1, Assigned: wlach)

Tracking

37 Branch
mozilla39
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

tempfile.mkstemp() returns a file descriptor that the caller is expected to close.  Not doing this can cause trouble on platforms such as Windows where it's not permitted to delete an open file
(Reporter)

Comment 1

4 years ago
Created attachment 8567329 [details] [diff] [review]
0001-Bug-1135255-Close-the-fd-that-mkstemp-returns.patch
Assignee: nobody → mvines
Status: NEW → ASSIGNED
Attachment #8567329 - Flags: review?(wlachance)
Comment on attachment 8567329 [details] [diff] [review]
0001-Bug-1135255-Close-the-fd-that-mkstemp-returns.patch

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

This is reasonable but I think I'd prefer to use "with". i.e.

with tempfile.NamedTemporarilyFile() as f:
   do stuff with f.name
   ...

This should auto-close the file and be a lot cleaner than what we have right now to boot.
Attachment #8567329 - Flags: review?(wlachance) → review-
(Reporter)

Comment 3

4 years ago
Ah, yep that would be better.  Thanks for the quick feedback.  Turns out it's easier for us to maintain this as a local patch ATM than upstream so unassigning myself.
Assignee: mvines → nobody
No longer blocks: 1063044
Created attachment 8567954 [details] [diff] [review]
Fix mozdevice tempfile handling on Windows

I figured we may as well fix this properly for next time, while it's fresh in our minds. This uses mozfile's NamedTemporaryFile that should give us the behaviour we want on Windows.
Assignee: nobody → wlachance
Attachment #8567329 - Attachment is obsolete: true
Attachment #8567954 - Flags: review?(gbrown)

Updated

4 years ago
Attachment #8567954 - Flags: review?(gbrown) → review+
Try run looks good, let's land this.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c6e9ecd2bed0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Reporter)

Comment 9

4 years ago
Thanks guys

Updated

3 years ago
Duplicate of this bug: 1161986
Duplicate of this bug: 1161986
You need to log in before you can comment on or make changes to this bug.