977373 - DeviceStorage is holding a file open

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dave Hylands [:dhylands]]

STR:
 - unplug USB cable
 - setup USB Mass Storage
 - start recording a video
 - stop recording the video
 - connect USB cable

The AutoMounter will try to share the sdcard, but it will fail due to open files. I see the following in logcat:

The following files are open under '/mnt/sdcard'                                                                               
  PID: 626 file: '/mnt/sdcard/1393458298451_tmp.3gp (deleted)' app: '' comm: 'b2g' exe: '/system/b2g/b2g'                      
  PID: 832 file: '/mnt/sdcard/1393458298451_tmp.3gp (deleted)' app: 'Camera' comm: 'Camera' exe: '/system/b2g/plugin-container'
  PID: 832 file: '/mnt/sdcard/1393458298451_tmp.3gp (deleted)' app: 'Camera' comm: 'Camera' exe: '/system/b2g/plugin-container'
UpdateState: Mounted volume sdcard has open files, not sharing or formatting

I think that this is related to the changes in bug 910498.

Comment 1

[Jason Smith [:jsmith]]

Should this be marked as 1.4+ as it blocks a blocker?

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Fix DeviceStorage file handle leak

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8383424 [details] [diff] [review]
Fix DeviceStorage file handle leak

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

This looks fine, but I'm wondering if the comment is accurate. I didn't see the dup() call anywhere in PR_FileDesc2NativeHandle().

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +944,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // NOTE: PR_FileDesc2NativeHandle returns a dup of the file descriptor, so

Where does the dupe take place? I can't find it.

Comment 4

[Dave Hylands [:dhylands]]

(In reply to Mike Habicher [:mikeh] from comment #3)
> Comment on attachment 8383424 [details] [diff] [review]
> Fix DeviceStorage file handle leak
> 
> Review of attachment 8383424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, but I'm wondering if the comment is accurate. I didn't see
> the dup() call anywhere in PR_FileDesc2NativeHandle().
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +944,3 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +  // NOTE: PR_FileDesc2NativeHandle returns a dup of the file descriptor, so
> 
> Where does the dupe take place? I can't find it.

You are right. The dup takes place in the FileDecriptor constructor:
http://dxr.mozilla.org/mozilla-central/source/ipc/glue/FileDescriptor.cpp#39

I'll fix the comment.

Comment 5

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5ae8fa849042

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ae8fa849042

Comment 7

[Dave Hylands [:dhylands]]

Comment on attachment 8383424 [details] [diff] [review]
Fix DeviceStorage file handle leak

This bug blocks 1028877 which is marked 1.3T+, so I'm requesting for approval on this patch to be uplifted to 1.3T

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 901498 (which was uplifted to 1.3T)
User impact if declined: See bug 9775597 and 1028877
Testing completed: Manual testing (since testing involves unplugging USB which is currently not part of the automated tests)
Risk to taking this patch (and alternatives if risky): Low (since this has been on master for 3 months)
String or UUID changes made by this patch: none

Comment 8

[Dave Hylands [:dhylands]]

Adding needinfo for jcheng and bbajaj since this patch needs to be uplifted to 1.3T, but the flags don't seem to be present to do that.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/6a4dbed87408