977372 - [Camera][Gecko] camera is holding a file descriptor open even after video recording has stopped

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, 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

Comment 1

[Jason Smith [:jsmith]]

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

Comment 2

[Dave Hylands [:dhylands]]

Attachment: Close device storage file descriptor when finished with it.

Comment 3

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

Comment on attachment 8383425 [details] [diff] [review]
Close device storage file descriptor when finished with it.

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

::: dom/camera/GonkCameraControl.cpp
@@ +862,2 @@
>    int fd = aFileDescriptor->mFileDescriptor.PlatformHandle();
> +  ScopedClose autoClose(fd);

I didn't even notice this had been removed in the earlier changeset. My bad. :(

Comment 4

[Dave Hylands [:dhylands]]

(In reply to Mike Habicher [:mikeh] from comment #3)
> Comment on attachment 8383425 [details] [diff] [review]
> Close device storage file descriptor when finished with it.
> 
> Review of attachment 8383425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/camera/GonkCameraControl.cpp
> @@ +862,2 @@
> >    int fd = aFileDescriptor->mFileDescriptor.PlatformHandle();
> > +  ScopedClose autoClose(fd);
> 
> I didn't even notice this had been removed in the earlier changeset. My bad.
> :(

I'm confused by your comment.

Comment 5

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

(In reply to Dave Hylands [:dhylands] from comment #4)
> 
> I'm confused by your comment.

Sorry--I reviewed your earlier changeset on this file where the scoped-close went away, and didn't notice.

Comment 6

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

(Earlier patch for bug 910498.)

Comment 7

[Dave Hylands [:dhylands]]

(In reply to Mike Habicher [:mikeh] from comment #5)
> (In reply to Dave Hylands [:dhylands] from comment #4)
> > 
> > I'm confused by your comment.
> 
> Sorry--I reviewed your earlier changeset on this file where the scoped-close
> went away, and didn't notice.

Ahh - Your comment makes more sense now (I wasn't thinking outside of this bug and this was the only changeset I submited). Although that scope close going away was for a file the camera was opening, so it was slighly different. Interesting that another one wound up in the same spot :)

Comment 8

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/461d5599aa8c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/b2g-inbound/rev/b03b65542900

https://tbpl.mozilla.org/php/getParsedLog.php?id=35439661&tree=B2g-Inbound

Comment 10

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/46841ca6d96a

Sigh the missing #include somehow wound up in a different patchset

Comment 11

[Dave Hylands [:dhylands]]

Backed out again due to windows bustage
https://hg.mozilla.org/integration/b2g-inbound/rev/17a26150a639

Comment 12

[Dave Hylands [:dhylands]]

Attachment: Pt 1 - Remove main thread assert so CloseFileRunnable can be used from non-main threads.

Comment 13

[Dave Hylands [:dhylands]]

Attachment: Close device storage file descriptor when finished with it. v2

Since this patch is totally different from the r+'d one, putting this one up for review again.

Comment 14

[Dave Hylands [:dhylands]]

https://tbpl.mozilla.org/?tree=Try&rev=f6b95daa21a3

Comment 15

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

Comment on attachment 8384916 [details] [diff] [review]
Close device storage file descriptor when finished with it. v2

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

LGTM.

Comment 16

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4fb349c12a63
https://hg.mozilla.org/integration/b2g-inbound/rev/1314168b5271

Comment 17

[Carsten Book [:Tomcat]]

(In reply to Dave Hylands [:dhylands] from comment #16)
> https://hg.mozilla.org/integration/b2g-inbound/rev/4fb349c12a63
> https://hg.mozilla.org/integration/b2g-inbound/rev/1314168b5271

sorry had to backout this changes, seems they caused https://tbpl.mozilla.org/php/getParsedLog.php?id=35708043&tree=B2g-Inbound

Comment 18

[Dave Hylands [:dhylands]]

Attachment: Pt 2 - Close device storage file descriptor when finished with it. v3

Added if test to verify that the file handle is valid before trying to use CloseFileRunnable.

Carrying r+

Comment 19

[Dave Hylands [:dhylands]]

https://tbpl.mozilla.org/?tree=Try&rev=f699aee12614

Comment 20

[Dave Hylands [:dhylands]]

Try run now looks green.

Re-landing

https://hg.mozilla.org/integration/b2g-inbound/rev/6541ba39a120
https://hg.mozilla.org/integration/b2g-inbound/rev/f8bf9630b972

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6541ba39a120
https://hg.mozilla.org/mozilla-central/rev/f8bf9630b972

Comment 22

[Dave Hylands [:dhylands]]

Comment on attachment 8384911 [details] [diff] [review]
Pt 1 - Remove main thread assert so CloseFileRunnable can be used from non-main threads.

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

I don't see a specific approval request for 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 23

[Dave Hylands [:dhylands]]

Comment on attachment 8386834 [details] [diff] [review]
Pt 2 - Close device storage file descriptor when finished with it. v3

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 24

[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 25

[Ryan VanderMeulen [:RyanVM]]

Part 2 needs rebasing around bug 909542 not being on 1.3T.

Comment 26

[Dave Hylands [:dhylands]]

Attachment: Pt 2 - Close device storage file descriptor when finished with it (v1.3t specific patch)

I was able to build for tarako with this patch, but I don't actually have a tarako, so I'm unable to test it.

I'd appreciate it if somebody could apply this patch and pt1 (About removing main thread assert)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/f96954fbca3f
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/63cdad87296c