Closed Bug 910498 Opened 11 years ago Closed 11 years ago

nsVolumeService::GetVolumeByPath() failed on nexus 4

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
1.3 Sprint 5 - 11/22

People

(Reporter: sotaro, Assigned: dhylands)

References

Details

(Whiteboard: [fxos:media])

Attachments

(4 files, 8 obsolete files)

910498_video_thumbnail
1014 bytes, text/plain
Details
Changes to camera code to use CreateFd
17.45 KB, patch
mikeh
: review+
Details | Diff | Splinter Review
Screenshot from 2013-11-27 12:16:20.png
92.52 KB, image/png
Details
Device Storage changes to remote CreateFd to the parent v5
65.43 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #871364 +++ Start Video recording was failed at nsVolumeService::GetVolumeByPath(). On nexus 4, sd card is not present and added internal storage by Bug 890144. It might affected to the failure. I will create a bug for it. The failed file path was following. - /storage/emulated/legacy/DCIM/100MZLLA/VID_0001.3gp
Blocks: 871364
blocking-b2g: koi? → koi+
Whiteboard: MEDIA_TRIAGED
"realpath(utf8Path.get(), realPathBuf)" returns "/storage/emulated/" from the following path. - /storage/emulated/legacy/DCIM/100MZLLA/VID_0001.3gp
(In reply to Sotaro Ikeda [:sotaro] from comment #1) > "realpath(utf8Path.get(), realPathBuf)" returns "/storage/emulated/" from > the following path. > - /storage/emulated/legacy/DCIM/100MZLLA/VID_0001.3gp Actual volume path is "/storage/emulated/legacy/"
"/storage/emulated/legacy/" is not present actually. It is a link to "/mnt/shell/emulated/0" >root@mako:/storage/emulated # ls -al >lrwxrwxrwx root root 2013-08-30 02:18 legacy -> /mnt/shell/emulated/0
Can someone take a bug?
(In reply to Sotaro Ikeda [:sotaro] from comment #4) > Can someone take a bug? Yeah - I'll take a look at it. I just need to get a nexus-4 tree checked out and building...
Assignee: nobody → dhylands
(In reply to Sotaro Ikeda [:sotaro] from comment #2) > (In reply to Sotaro Ikeda [:sotaro] from comment #1) > > "realpath(utf8Path.get(), realPathBuf)" returns "/storage/emulated/" from > > the following path. > > - /storage/emulated/legacy/DCIM/100MZLLA/VID_0001.3gp > > Actual volume path is "/storage/emulated/legacy/" Interesting. Since /storage/emulated/legacy is a symlink to /mnt/shell/emulated/0, I would have expected that realpath on /storage/emulated/legacy would return /mnt/shell/emulated/0 realpath on /storage/emulated would return /storage/emulated (since that's a real directory). The question is why the legacy bit got skipped.
When "/storage/emulated/" is resolved within realpath(). Following lstat() failed and end the processing. > if (lstat(resolved, &sb) != 0) { http://androidxref.com/4.3_r2.1/xref/bionic/libc/upstream-freebsd/lib/libc/stdlib/realpath.c#183
It seems like related to permission setting of the directory.
when I changed the permission of "/storage" from 0050 to 0555 by changing init.rc. I got "/mnt/shell/emulated/" as the path.
It would be good to the output of adb shell ls -l / adb shell ls -l /storage adb shell ls -l /storage/emulated adb shell ls -l /mnt adb shell ls -l /mnt/shell adb shell ls -l /mnt/shell/emulated
(In reply to Sotaro Ikeda [:sotaro] from comment #9) > when I changed the permission of "/storage" from 0050 to 0555 by changing > init.rc. I got "/mnt/shell/emulated/" as the path. It happens because lstat() failed at "/mnt/shell/emulated/". It seems happens because permission of "/mnt/shell/" is the following. > drwx------ shell shell 2013-08-30 04:06 shell
> adb shell ls -l / drwxr-xr-x root root 2013-08-30 04:32 acct drwxrwx--- system cache 2013-08-29 18:33 cache -rwxr-x--- root root 268128 1969-12-31 19:00 charger dr-x------ root root 2013-08-30 04:32 config lrwxrwxrwx root root 2013-08-30 04:32 d -> /sys/kernel/debug drwxrwx--x system system 2013-08-30 04:06 data -rw-r--r-- root root 164 1969-12-31 19:00 default.prop drwxr-xr-x root root 2013-08-30 04:32 dev lrwxrwxrwx root root 2013-08-30 04:32 etc -> /system/etc -rw-r--r-- root root 10340 1969-12-31 19:00 file_contexts dr-xr-x--- system system 1969-12-31 19:00 firmware -rw-r----- root root 2588 1969-12-31 19:00 fstab.mako -rwxr-x--- root root 154500 1969-12-31 19:00 init -rwxr-x--- root root 654 1969-12-31 19:00 init.b2g.rc -rwxr-x--- root root 15178 1969-12-31 19:00 init.mako.rc -rwxr-x--- root root 5935 1969-12-31 19:00 init.mako.usb.rc -rwxr-x--- root root 20060 1969-12-31 19:00 init.rc -rwxr-x--- root root 1795 1969-12-31 19:00 init.trace.rc -rwxr-x--- root root 3915 1969-12-31 19:00 init.usb.rc drwxrwxr-x root system 2013-08-30 04:32 mnt drwxrwx--x system system 1970-01-01 19:22 persist dr-xr-xr-x root root 1969-12-31 19:00 proc -rw-r--r-- root root 2109 1969-12-31 19:00 property_contexts drwxr-xr-x root root 1969-12-31 19:00 res drwx------ root root 2013-06-28 17:26 root drwxr-x--- root root 1969-12-31 19:00 sbin lrwxrwxrwx root root 2013-08-30 04:32 sdcard -> /storage/emulated/legacy -rw-r--r-- root root 611 1969-12-31 19:00 seapp_contexts -rw-r--r-- root root 66639 1969-12-31 19:00 sepolicy d---r-x--- root sdcard_r 2013-08-30 04:32 storage dr-xr-xr-x root root 2013-08-30 04:32 sys drwxr-xr-x root root 1969-12-31 19:00 system -rw-r--r-- root root 2286 1969-12-31 19:00 ueventd.mako.rc -rw-r--r-- root root 4024 1969-12-31 19:00 ueventd.rc lrwxrwxrwx root root 2013-08-30 04:32 vendor -> /system/vendor
> adb shell ls -l /storage dr-xr-xr-x root root 2013-08-30 04:32 emulated lrwxrwxrwx root root 2013-08-30 04:32 sdcard0 -> /storage/emulated/legacy
> adb shell ls -l /storage/emulated lrwxrwxrwx root root 2013-08-30 04:32 legacy -> /mnt/shell/emulated/0
>adb shell ls -l /mnt drwxr-xr-x root system 2013-08-30 04:32 asec drwxr-xr-x root system 2013-08-30 04:32 obb lrwxrwxrwx root root 2013-08-30 04:32 sdcard -> /storage/emulated/legacy drwx------ root root 2013-08-30 04:32 secure drwx------ shell shell 2013-08-30 04:32 shell
> adb shell ls -l /mnt/shell drwxrwxr-x root sdcard_rw 2013-08-30 02:18 emulated
> adb shell ls -l /mnt/shell/emulated drwxrwxr-x root sdcard_rw 2013-08-30 02:19 0 drwxrwxr-x root sdcard_rw 2013-08-30 02:18 obb
As a temporary workaround, you should be able to do something like this: adb shell mkdir /data/sdcard and point the sdcard in volume.cfg to /data/sdcard
(In reply to Dave Hylands [:dhylands] from comment #18) > As a temporary workaround, you should be able to do something like this: > > adb shell mkdir /data/sdcard I tried this but failed to create a file descriptor with "(13) Permission denied". > ScopedClose fd(open(nativeFilename.get(), O_RDWR | O_CREAT, 0644)); http://mxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#946
When app run as root permission, the file descriptor creation succeeded and succeeded to record a video by camera app.
From security point of view, it might be better that the file descriptor will be created in b2g process in near future.
I wasn't able to test this because the camera app wouldn't let me record. I think I figured out the problem though. You should replace the contents of volume.cfg with: create sdcard /mnt/shell/emulated/0 I'll fix adding the volume to run realpath on the path that comes from volume.cfg, so that the volume service winds up comparing 2 paths which have both had realpath run on them.
This patch does the following: - realpath returns a pointer, not an int, and GetVolumeByPath testing the result as if it were an int. - made all lines of GetVolumeByPath fit in 80 characters. - improved error messages for GetVolumeByPath. - Made CreateFakeVolume use realpath to normalize the mount point.
Comment on attachment 799202 [details] [diff] [review] Normalize fake volume paths by using realpath I put you down to review this, since I was unable to get the camera to work on my n4.
Attachment #799202 - Flags: review?(sotaro.ikeda.g)
(In reply to Dave Hylands [:dhylands] from comment #24) > Comment on attachment 799202 [details] [diff] [review] > Normalize fake volume paths by using realpath The function failed by following log. ---------------------------- 09-05 08:36:15.254 898 912 E nsVolumeService: GetVolumeByPath: realpath on '/mnt/shell/emulated/0/DCIM/100MZLLA/VID_0005.3gp' failed. errno 13 Permission denied 09-05 08:36:15.254 898 912 I Gecko : [Child 898] WARNING: NS_ENSURE_SUCCESS(rv, nsresult::NS_ERROR_INVALID_ARG) failed with result 0x80520015: file ../../../gecko/dom/camera/GonkCameraControl.cpp, line 928
> I put you down to review this, since I was unable to get the camera to work > on my n4. On default nexus4's ROM, camera do not work. To enable camera, following change is necessary. - [1] Set layers.use-deprecated-textures=false - [2] Apply patches in Bug 871364.
(In reply to Sotaro Ikeda [:sotaro] from comment #25) > (In reply to Dave Hylands [:dhylands] from comment #24) > > Comment on attachment 799202 [details] [diff] [review] > > Normalize fake volume paths by using realpath > > The function failed by following log. > ---------------------------- > 09-05 08:36:15.254 898 912 E nsVolumeService: GetVolumeByPath: realpath > on '/mnt/shell/emulated/0/DCIM/100MZLLA/VID_0005.3gp' failed. errno 13 > Permission denied > > 09-05 08:36:15.254 898 912 I Gecko : [Child 898] WARNING: > NS_ENSURE_SUCCESS(rv, nsresult::NS_ERROR_INVALID_ARG) failed with result > 0x80520015: file ../../../gecko/dom/camera/GonkCameraControl.cpp, line 928 If we change volume.cfg as create sdcard /data/media/0 and change directory permissions of /data/media/0/DCIM/100MZLLA/ to 777 the video recorder is working, but not able to play with Video player.
(In reply to Sridhar Ravipati from comment #27) Permissions to be changed for all the below directories /data/media/0/DCIM/100MZLLA/ /data/media/0/DCIM/ /data/media/0/ /data/media/ when DCIM/100MZLLA/ directories are created with root permissions (during dummy video file creation in camera.js).
I could see only difference between Emulated devices (mnt & data) is pid. In data has both pid and uid is 'media_rw'. root@mako:/ # ls -al /mnt/shell/emulated/0 drwxrwxr-x root media_rw 2013-09-11 20:45 DCIM root@mako:/ # ls -al /data/media/0/ drwxrwxr-x media_rw media_rw 2013-09-11 20:45 DCIM Is that will affect the Video file creation?
The real problem is: root@mako:/ # ls -ld /mnt/shell drwx------ shell shell 2013-09-07 12:45 shell If we modify https://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#306 to add AID_MEDIA_RW then we could use /data/media
Attachment #803138 - Attachment description: HACK pt1 - Change b2g.sh to use a umask of 007 instead of 027 → HACK pt1 - gonk-misc - Change b2g.sh to use a umask of 007 instead of 027
With the 3 hacks applied, I can record a video. Gallery still doesn't see the video (need to investigate), but you can pull it from /data/sdcard and play it back on your PC.
With above three patches, I am able to Record it. But Video player not working. Further Analysis in the Player: In gaia/apps/video/js/db.js. First time Video DB interface Initializing. It got error in videos in the MediaDB var isVideo = videoinfo.metadata.isVideo; alert('enumerateDB status '+ isVideo ); >>>>>>>>>>>>>>>>>It displays 'false' The console error messages are below: /Gecko ( 889): [Child 889] WARNING: NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed: file ../../../../gecko/content/base/src/nsScriptLoader.cpp, line 454 I/Gecko ( 889): [Child 889] WARNING: NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed: file ../../../../gecko/content/base/src/nsScriptLoader.cpp, line 454 I/Gecko ( 889): [Child 889] WARNING: NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed: file ../../../../gecko/content/base/src/nsScriptLoader.cpp, line 454 I/Gecko ( 889): [Child 889] WARNING: NS_ENSURE_TRUE(ParseTypeAttribute(type, &version)) failed: file ../../../../gecko/content/base/src/nsScriptLoader.cpp, line 454 I/Gecko ( 889): [Child 889] WARNING: NS_ENSURE_TRUE(startupCache) failed: file ../../../../gecko/content/xbl/src/nsXBLDocumentInfo.cpp, line 504 I/Gecko ( 889): [Child 889] WARNING: NS_ENSURE_TRUE(startupCache) failed: file ../../../../gecko/content/xbl/src/nsXBLDocumentInfo.cpp, line 574 I/Gecko ( 178): [Parent 178] ###!!! ASSERTION: Null window but not chrome!: 'nsContentUtils::IsCallerChrome()', file ../../../gecko/dom/quota/QuotaManager.cpp, line 1074 I/Gecko ( 178): [Parent 178] WARNING: Failed to get caller.: file ../../../gecko/dom/indexedDB/IDBRequest.cpp, line 292 I/Gonk ( 178): Setting nice for pid 622 to 18 I/Gonk ( 178): Changed nice for pid 622 from 1 to 18. E/LocalFileUnix( 178): ACC CreateAndKeepOpen I/Gecko ( 178): [Parent 178] WARNING: No docshells for remote frames!: file ../../../../gecko/content/base/src/nsFrameLoader.cpp, line 570 Second time onwards, Application won't call the InitDB(), Just return the Error information.
I made change 'videoinfo.metadata.isVideo' as 'true' in gaia/apps/video/js/db.js. After that player able to display the video files from /data/sdcard/DCIM/100MZLLA/ folder. Changes :gaia/apps/video/js/db.js var isVideo = videoinfo.metadata.isVideo; isVideo = true; // Added // var isVideo = videoinfo.metadata.isVideo; isVideo = true; // Modified. After Video recorded, it does not display the Video files from the Storage devices. It requires Device reboot, MediaDB update the database and display the Videofiles.
I made change 'videoinfo.metadata.isVideo' as 'true' in gaia/apps/video/js/db.js. After that player able to display the video files from /data/sdcard/DCIM/100MZLLA/ folder. Changes :gaia/apps/video/js/db.js var isVideo = videoinfo.metadata.isVideo; isVideo = true; // Added function addVideo(videodata) { // if (!videodata || !videodata.metadata.isVideo) { if (!videodata ) { // Modified After Video recorded, it does not display the Video files from the Storage devices. It requires Device reboot, MediaDB update the database and display the Videofiles.
The real problem is on metadata.js It starts parse metadata from video files. GetMetaData() checks the video file object and made isVideo=true. After this createThumbnail() function get invoked and set the default property of 'metadata.isVideo = false' Finally the videoinfo.metadata.isVideo=false. Due to this db() functions not adding any video files into Queue.
CreateThumbnail uses the HTMLVideoElement to get the ImageSurface. But the Video element graphics buffer format is 'HAL_PIXEL_FORMAT_YCbCr_420_SP_TILE'. This is not implemented in GrallocImages.cpp. During the Video player, android Color converter is invalid, return as [Exception... "Component is not available" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE) Color conversion is implemented from YUV420 to RGB for 'YCbCr_420_SP_TILE' and Fixed this problem Review & provide feedback. patch attached: 910498_video_thumbnail
Attached file 910498_video_thumbnail —
This patch convert video data from YUV to RGB for HAL_PIXEL_FORMAT_YCbCr_420_SP_TILE format
(In reply to saminath from comment #40) > Created attachment 807695 [details] > 910498_video_thumbnail > > This patch convert video data from YUV to RGB for > HAL_PIXEL_FORMAT_YCbCr_420_SP_TILE format I think you may have commented on the wrong bug?
(In reply to Dave Hylands [:dhylands] from comment #41) > (In reply to saminath from comment #40) > > Created attachment 807695 [details] > > 910498_video_thumbnail > > > > This patch convert video data from YUV to RGB for > > HAL_PIXEL_FORMAT_YCbCr_420_SP_TILE format > > I think you may have commented on the wrong bug? Add color format support is included in Bug 911548.
Blocks: 922113
(In reply to Sotaro Ikeda [:sotaro] from comment #21) > From security point of view, it might be better that the file descriptor > will be created in b2g process in near future. Created Bug 922113 for the above.
We have encountered the same question as you memtioned. And we have create a bug 891274.Please refer it. (In reply to saminath from comment #38) > The real problem is on metadata.js > It starts parse metadata from video files. GetMetaData() checks the video > file object and made isVideo=true. > After this createThumbnail() function get invoked and set the default > property of 'metadata.isVideo = false' > > Finally the videoinfo.metadata.isVideo=false. > > Due to this db() functions not adding any video files into Queue.
Comment on attachment 799202 [details] [diff] [review] Normalize fake volume paths by using realpath clear the review? flag. This patch is wip patch.
Attachment #799202 - Flags: review?(sotaro.ikeda.g)
dhylands, is there a progress?
Flags: needinfo?(dhylands)
(In reply to Sotaro Ikeda [:sotaro] from comment #47) > dhylands, is there a progress? Sorry - it's been slow. I just got back from PTO and will be working on this bug again.
Flags: needinfo?(dhylands)
Okey. Thanks for the quick response.
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Sotaro - Dave is working on the device storage support for nexus 4. He says he has a workaround for you temporarily while he continues to work on adding this support. I am moving this into 1.3 If this needs to go into koi+, please renom and provide more info on why this should be in koi+ bucket.
blocking-b2g: koi+ → 1.3+
There is no problem this bug about it. I already did a workaround locally when necessary.
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
Attachment #828778 - Attachment is obsolete: true
Attachment #828800 - Flags: review?(bent.mozilla)
Attachment #828776 - Flags: review?(mhabicher)
Comment on attachment 828776 [details] [diff] [review] Changes to camera code to use CreateFd Review of attachment 828776 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r+ once you've fixed the nits and the comment I call out in DOMCameraControl.h. ::: dom/camera/CameraControlImpl.h @@ +56,5 @@ > nsresult StartPreview(DOMCameraPreview* aDOMPreview); > void StopPreview(); > nsresult AutoFocus(nsICameraAutoFocusCallback* onSuccess, nsICameraErrorCallback* onError); > nsresult TakePicture(const idl::CameraSize& aSize, int32_t aRotation, const nsAString& aFileFormat, idl::CameraPosition aPosition, uint64_t aDateTime, nsICameraTakePictureCallback* onSuccess, nsICameraErrorCallback* onError); > + nsresult StartRecording(idl::CameraStartRecordingOptions* aOptions, DeviceStorageFileDescriptor *aDSFileDescriptor, nsICameraStartRecordingCallback* onSuccess, nsICameraErrorCallback* onError); nit: asterisk with the type. @@ +454,5 @@ > { > public: > + StartRecordingTask(CameraControlImpl* aCameraControl, > + idl::CameraStartRecordingOptions aOptions, > + DeviceStorageFileDescriptor *aDSFileDescriptor, Ditto. ::: dom/camera/DOMCameraControl.cpp @@ +399,5 @@ > + > +NS_IMETHODIMP > +nsDOMCameraControl::HandleEvent(nsIDOMEvent* aEvent) > +{ > + nsString eventType; nit: one space between type and varname. ::: dom/camera/DOMCameraControl.h @@ +111,5 @@ > > + mozilla::idl::CameraStartRecordingOptions mOptions; > + nsRefPtr<DeviceStorageFileDescriptor> mDSFileDescriptor; > + nsCOMPtr<nsICameraStartRecordingCallback> mOnSuccessCb; > + nsCOMPtr<nsICameraErrorCallback> mOnErrorCb; Make sure you clean 'mOnSuccessCb' and 'mOnErrorCb' in nsDOMCameraControl::Shutdown() so that we don't have any leftover object references keeping the window alive. ::: dom/camera/ICameraControl.h @@ +26,5 @@ > virtual nsresult StartPreview(DOMCameraPreview* aDOMPreview) = 0; > virtual void StopPreview() = 0; > virtual nsresult AutoFocus(nsICameraAutoFocusCallback* onSuccess, nsICameraErrorCallback* onError) = 0; > virtual nsresult TakePicture(const idl::CameraSize& aSize, int32_t aRotation, const nsAString& aFileFormat, idl::CameraPosition aPosition, uint64_t aDateTime, nsICameraTakePictureCallback* onSuccess, nsICameraErrorCallback* onError) = 0; > + virtual nsresult StartRecording(idl::CameraStartRecordingOptions* aOptions, DeviceStorageFileDescriptor *aFileDescriptor, nsICameraStartRecordingCallback* onSuccess, nsICameraErrorCallback* onError) = 0; nit: asterisk goes with the type, not the varname.
Attachment #828776 - Flags: review?(mhabicher) → review+
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.3 Sprint 5 - 11/22
Comment on attachment 828800 [details] [diff] [review] Device Storage changes to remote CreateFd to the parent v2 Review of attachment 828800 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good. There are a few things we need to address though: ::: dom/devicestorage/DeviceStorage.h @@ +11,5 @@ > #include "nsIFile.h" > #include "nsIPrincipal.h" > #include "nsIObserver.h" > #include "nsDOMEventTargetHelper.h" > +#include "mozilla/FileUtils.h" Hm, can this be moved to the cpp? @@ +39,5 @@ > +class DeviceStorageFileDescriptor MOZ_FINAL > + : public mozilla::RefCounted<DeviceStorageFileDescriptor> > +{ > +public: > + DeviceStorageFileDescriptor() {} Nit: No need for this. @@ +113,5 @@ > nsIFile** aFile); > > nsresult CalculateSizeAndModifiedDate(); > nsresult CalculateMimeType(); > + nsresult CreateFileDescriptor(mozilla::ipc::FileDescriptor* aFileDescriptor); Make this take a reference since it can never be null. @@ +252,5 @@ > already_AddRefed<DOMRequest> UsedSpace(ErrorResult& aRv); > already_AddRefed<DOMRequest> Available(ErrorResult& aRv); > > + already_AddRefed<DOMRequest> > + CreateFileDescriptor(const nsAString& aPath, This is a WebIDL signature but you're not exposing this via WebIDL... I'd just remove it and make the callers use the XPIDL version. ::: dom/devicestorage/DeviceStorageRequestChild.cpp @@ +29,5 @@ > +} > + > +DeviceStorageRequestChild::DeviceStorageRequestChild(DOMRequest* aRequest, > + DeviceStorageFile* aDSFile, > + DeviceStorageFileDescriptor *aDSFileDescriptor) Nit: * on the left @@ +32,5 @@ > + DeviceStorageFile* aDSFile, > + DeviceStorageFileDescriptor *aDSFileDescriptor) > + : mRequest(aRequest) > + , mDSFile(aDSFile) > + , mDSFileDescriptor(aDSFileDescriptor) Can you assert that these are non-null here? ::: dom/devicestorage/DeviceStorageRequestChild.h @@ +7,5 @@ > #define mozilla_dom_devicestorage_DeviceStorageRequestChild_h > > #include "mozilla/dom/devicestorage/PDeviceStorageRequestChild.h" > #include "DOMRequest.h" > +#include "DeviceStorage.h" You can probably fwd-declare DeviceStorageFile and DeviceStorageFileDescriptor, move this #include to the cpp. ::: dom/devicestorage/DeviceStorageRequestParent.cpp @@ +57,5 @@ > target->Dispatch(r, NS_DISPATCH_NORMAL); > break; > } > > + case DeviceStorageParams::TDeviceStorageCreateFdParams: Let's discuss this. @@ +781,5 @@ > +{ > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + > + FileDescriptorResponse response(mFileDescriptor); > + unused << mParent->Send__delete__(mParent, response); Hrm, if this fails you'll need to close the fd yourself. Otherwise we'll have an FD leak. This isn't one of those cases we can ignore. ::: dom/devicestorage/nsDeviceStorage.cpp @@ +1925,5 @@ > + : mDSFileDescriptor(aDSFileDescriptor) > + , mRequest(aRequest) > + {} > + > + ~CreateFdEvent() {} Maybe assert that mRequest is non-null here? @@ +1931,5 @@ > + NS_IMETHOD Run() > + { > + MOZ_ASSERT(!NS_IsMainThread()); > + > + nsRefPtr<DeviceStorageFile> dsFile = mDSFileDescriptor->mDSFile; Make this an alias to avoid another AddRef/Release pair. @@ +1939,5 @@ > + bool check = false; > + dsFile->mFile->Exists(&check); > + if (check) { > + nsCOMPtr<PostErrorEvent> event = > + new PostErrorEvent(mRequest.forget(), POST_ERROR_EVENT_FILE_EXISTS); Hrm. If you're failing here when the file exists then why does CreateFileDescriptor pass PR_TRUNCATE to the OpenNSPRFileDesc? @@ +1951,5 @@ > + dsFile->mFile->Remove(false); > + > + nsCOMPtr<PostErrorEvent> event = > + new PostErrorEvent(mRequest.forget(), POST_ERROR_EVENT_UNKNOWN); > + NS_DispatchToMainThread(event); At least assert that this succeeds, below too. @@ +1956,5 @@ > + return NS_OK; > + } > + > + nsString fullPath; > + dsFile->GetFullPath(fullPath); I'd either assert that this succeeded or that fullPath is not empty. @@ +2213,5 @@ > > + DeviceStorageRequest(const DeviceStorageRequestType aRequestType, > + nsPIDOMWindow *aWindow, > + nsIPrincipal *aPrincipal, > + DeviceStorageFile *aFile, Nit: (multiple) * on the left @@ +2221,5 @@ > + , mWindow(aWindow) > + , mPrincipal(aPrincipal) > + , mFile(aFile) > + , mRequest(aRequest) > + , mDSFileDescriptor(aDSFileDescriptor) {} What can you assert about these arguments? Some must be non-null I think? And aRequestType must be DEVICE_STORAGE_REQUEST_CREATEFD, right? @@ +2349,5 @@ > + > + if (!typeChecker->Check(mFile->mStorageType, mFile->mFile)) { > + r = new PostErrorEvent(mRequest.forget(), > + POST_ERROR_EVENT_ILLEGAL_TYPE); > + NS_DispatchToMainThread(r); Hrm, you're already on the main thread here, NS_DispatchToCurrentThread is cheaper. I know that the other cases do this too but we might as well fix them. @@ +3207,5 @@ > } > > NS_IMETHODIMP > +nsDOMDeviceStorage::CreateFileDescriptor(const nsAString& aPath, > + DeviceStorageFileDescriptor *aDSFileDescriptor, Nit: * on the left. @@ +3223,5 @@ > +nsDOMDeviceStorage::CreateFileDescriptor(const nsAString& aPath, > + DeviceStorageFileDescriptor* aDSFileDescriptor, > + ErrorResult& aRv) > +{ > + nsCOMPtr<nsPIDOMWindow> win = GetOwner(); Since you're touching a window and DOM stuff here it would be nice to add a NS_IsMainThread assertion. @@ +3236,5 @@ > + aRv.Throw(NS_ERROR_FAILURE); > + return nullptr; > + } > + > + nsCOMPtr<nsIRunnable> r; Nit: This can get moved lower (closer to where you actually use it) @@ +3247,5 @@ > + r = new PostErrorEvent(request, POST_ERROR_EVENT_UNKNOWN); > + NS_DispatchToMainThread(r); > + return request.forget(); > + } > + return ds->CreateFileDescriptor(aPath, aDSFileDescriptor, aRv); This needs to be 'storagePath' to avoid an iloop. @@ +3267,3 @@ > } > + > + NS_DispatchToMainThread(r); NS_DispatchToCurrentThread is cheaper here. ::: dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl @@ +13,5 @@ > > +%{C++ > +class DeviceStorageFileDescriptor; > +%} > +[ptr] native DeviceStorageFdPtr(DeviceStorageFileDescriptor); I think you should actually make this a [ref], you never want a null pointer
Attachment #828800 - Flags: review?(bent.mozilla) → review-
Based on the update from Dave, we are not ready to land this for 1.3. We will need more time to address review comments and test. Also this issue affects nexus4 only - not a blocker.
blocking-b2g: 1.3+ → ---
:Hema -- :Sotaro correct me if i am wrong, but this issue is affecting camcorder functionality in general on 1.3. Video recording is not working on our reference hardware as well with the same meessage: "Couldn't create file '/storage/sdcard1/DCIM/100MZLLA/VID_0001.3gp': (13) Permission denied" Have we verified camcorder functionality on any hardware on 1.3?
Hema / Sotaro / Dave, This is not just seen on nexus 4. Even the emulator build has the problem. This is a must fix and a blocker. Basic camcorder doesn't work. I have another device ported gecko to my android device has this issue. Even I get the same message when enabled logs.. E/manzzee ( 1232): Enter StartRecording E/manzzee ( 1232): mozilla::StartRecordingTask::StartRecordingTask(mozilla::CameraControlImpl*, mozilla::idl::CameraStartRecordingOptions, nsIFile*, const nsAString_internal&, nsICameraStartRecordingCallback*, nsICameraErrorCallback*, uint64_t):462 : this=0xb3075740 E/manzzee ( 1232): Exit: StartRecording. E/manzzee ( 1232): virtual nsresult mozilla::StartRecordingTask::Run():472 E/manzzee ( 1232): Enter: nsGonkCameraControl::StartRecordingImpl E/manzzee ( 1232): Video filename is '/storage/sdcard1/DCIM/100MZLLA/VID_0050.3gp' E/manzzee ( 1232): Couldn't create file '/storage/sdcard1/DCIM/100MZLLA/VID_0050.3gp': (13) Permission denied E/manzzee ( 1232): virtual nsresult mozilla::StartRecordingTask::Run():474 : result -2147467259 E/manzzee ( 1232): virtual mozilla::StartRecordingTask::~StartRecordingTask():467 : this=0xb3075740
Video Recording re-uses android's StagefrightRecorder's code. It requires to allocate file descriptor in content process. Camera application's process has special capability to allocate files in sdcard. It is "AID_SDCARD_RW". If the system requires more capabilities to allocate a file descriptor, the descriptor allocation failed like the following log. > "Couldn't create file '/storage/sdcard1/DCIM/100MZLLA/VID_0001.3gp': (13) Permission denied" This happens typically when storage is an internal storage. It requires more capability. attachment 828800 [details] [diff] [review] is for allocating a file descriptor in b2g process by request from a content process.
(In reply to Madana Manjunatha from comment #60) > Even the emulator build has the problem. Madana, which kind of problem did you see on the emulator? On ICS arm emulator, I confirmed the recording works. On JB arm emulator, camera does not work now. It is Bug 918870.
Flags: needinfo?(manjunatha.m)
(In reply to Inder from comment #59) > :Hema -- :Sotaro correct me if i am wrong, but this issue is affecting > camcorder functionality in general on 1.3. > > Video recording is not working on our reference hardware as well with the > same meessage: > "Couldn't create file '/storage/sdcard1/DCIM/100MZLLA/VID_0001.3gp': (13) > Permission denied" > > Have we verified camcorder functionality on any hardware on 1.3? To the best of my knowledge, only JB devices (JB-emulator and nexus4) are affected. There is a working patch attached to this bug, which developers can use until the patch lands. Also to the best of my knowledge, none of our partners have phones which will be released using 1.3 and a JB base.
Attachment #799202 - Attachment is obsolete: true
Attachment #803138 - Attachment is obsolete: true
Attachment #803143 - Attachment is obsolete: true
Attachment #803147 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #63) > (In reply to Madana Manjunatha from comment #60) > > Even the emulator build has the problem. > > Madana, which kind of problem did you see on the emulator? On ICS arm > emulator, I confirmed the recording works. On JB arm emulator, camera does > not work now. It is Bug 918870. Sotaro, I have a developer device with firefox 1.0.0. Camera and Camcorder works perfect. On a device which has android 4.3, with external sdcard, Camera image capture works perfect. But Video recorder has the permission problems. On the jb emulator, there is a problem with the image capture, the camera app freezes after the image capture. But the Video capture has the permission denied issue, it straight away throws the permission denied error. Dave, Should I take the both the patches attached in the Bug for the workaround?
Flags: needinfo?(dhylands)
Flags: needinfo?(manjunatha.m)
> Dave, Should I take the both the patches attached in the Bug for the > workaround? Yes. I wrote and tested them on the Nexus 4, but they're generic so they should work on any device.
Flags: needinfo?(dhylands)
:Hema, :Sotaro - Filed bug 945562. We need to land :dhylands' patch in 1.3 timeframe or group privilege change mentioned in: https://bugzilla.mozilla.org/show_bug.cgi?id=945562#c0 to unblock camcorder functionality in v1.3 Thanks.
Flags: needinfo?(hkoka)
(In reply to Inder from comment #67) > :Hema, :Sotaro - Filed bug 945562. > We need to land :dhylands' patch in 1.3 timeframe or group privilege change > mentioned in: > https://bugzilla.mozilla.org/show_bug.cgi?id=945562#c0 to unblock camcorder > functionality in v1.3 > > Thanks. The group privledge change is a low-risk change, and I would highly recommend that we go that route (it's a one line change). The patchset in this bug would be a much higher risk change since it introduces a totally new mechanism for opening files, and is a 2500 line patch.
sgtm. Inder already has a patch so assigning to him
Assignee: dhylands → ikumar
blocking-b2g: --- → 1.3+
Flags: needinfo?(hkoka)
Michael, Dave has already being working on this bug and the patch for this (under review) is risky to land for 1.3 (see comment #68) . We will target it for the next train (1.4) Can you please track inder's changes in another bug? Thanks Hema
Flags: needinfo?(mvines)
Opps, sorry! My brain was editing bug 945562 instead of this bug. :(
Assignee: ikumar → dhylands
blocking-b2g: 1.3+ → ---
Flags: needinfo?(mvines)
> > The group privledge change is a low-risk change, and I would highly > recommend that we go that route (it's a one line change). > > The patchset in this bug would be a much higher risk change since it > introduces a totally new mechanism for opening files, and is a 2500 line > patch. Thanks Dave. Will upload my patch in bug 945562.
Address review comments
Attachment #828800 - Attachment is obsolete: true
A couple minor cleanups
Attachment #8346674 - Attachment is obsolete: true
Removed a bogus comment
Attachment #8346693 - Attachment is obsolete: true
Attachment #8346695 - Flags: review?(bent.mozilla)
Added [fxos:media] whiteboard for bugs assigned to me so that they can be triaged/prioritized, etc.
Whiteboard: MEDIA_TRIAGED → [fxos:media]
(In reply to Sotaro Ikeda [:sotaro] from comment #62) > Video Recording re-uses android's StagefrightRecorder's code. It requires to > allocate file descriptor in content process. Camera application's process > has special capability to allocate files in sdcard. It is "AID_SDCARD_RW". > If the system requires more capabilities to allocate a file descriptor, the > descriptor allocation failed like the following log. > > > "Couldn't create file '/storage/sdcard1/DCIM/100MZLLA/VID_0001.3gp': (13) Permission denied" > > This happens typically when storage is an internal storage. It requires more > capability. attachment 828800 [details] [diff] [review] is for allocating a > file descriptor in b2g process by request from a content process. Hi Sotaro, I also ran into the video recording issue today and ended up with writing to /data/media/... and it worked. But there's still one thing confusing me: What's the additional capability the content prcoess requires to allocate a file descriptor? It already has [AID_SDCARD_R, AID_SDCARD_RW, AID_MEDIA_RW] as shown here: http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#309 Thanks :)
(In reply to Sotaro Ikeda [:sotaro] from comment #62) > Video Recording re-uses android's StagefrightRecorder's code. It requires to > allocate file descriptor in content process. Camera application's process > has special capability to allocate files in sdcard. It is "AID_SDCARD_RW". > If the system requires more capabilities to allocate a file descriptor, the > descriptor allocation failed like the following log. > > > "Couldn't create file '/storage/sdcard1/DCIM/100MZLLA/VID_0001.3gp': (13) Permission denied" > > This happens typically when storage is an internal storage. It requires more > capability. attachment 828800 [details] [diff] [review] is for allocating a > file descriptor in b2g process by request from a content process. Hi Sotaro, I also ran into the video recording issue today and ended up with writing to /data/media/... and it worked. But there's still one thing confusing me: What's the additional capability the content prcoess requires to allocate a file descriptor? It already has [AID_SDCARD_R, AID_SDCARD_RW, AID_MEDIA_RW] as shown here: http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#309 Thanks :)
Flags: needinfo?(ikeda.sohtaroh)
Flags: needinfo?(ikeda.sohtaroh) → needinfo?(sotaro.ikeda.g)
(In reply to Henry Chang [:henry] from comment #78) > > What's the additional capability the content prcoess requires to allocate a > file descriptor? It already has [AID_SDCARD_R, AID_SDCARD_RW, AID_MEDIA_RW] > as shown here: > > http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/ > process_util_linux.cc#309 [AID_SDCARD_R, AID_SDCARD_RW, AID_MEDIA_RW] seem to be added just to fix the problem of external sdcard. In general, if the content process want to allocate file descriptor to a internal storage, the internal storage's filesystem needs to be correctly configured to permit it. Otherwise, system/root level permission becomes necessary. It seems better to ask to dhylands if you want to know more.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Henry Chang [:henry] from comment #79) ...snip > What's the additional capability the content prcoess requires to allocate a > file descriptor? It already has [AID_SDCARD_R, AID_SDCARD_RW, AID_MEDIA_RW] > as shown here: On JB and newer, the content process has no access to the sdcard regardless of what groups it has. The file descriptor needs to be allocated from the parent (which has root access) and passed to the content. That's what the patch attached to this bug does.
Sotaro, Dave, thanks for your reply! I am also curious about how this works. Since according to this http://androidxref.com/4.3_r2.1/xref/system/vold/Volume.cpp#429 internal sdcard has uid:gid system:sdcard_rw. Process with uid system has no r/w/x permission and process with gid sdcard_rw has full access permission to it. (since umask is 702) Anyway, I am very interested in this issue. Thanks :)
In JB, android added support for "Users". This support is configurable on tablets (i.e. you can add multiple users), but on phones you can't. The way Users works, is that on internal sdcards, user A can't even see any files owned by user B. In order to do this, the default permissions (on internal sdcards) is that no app can access any files. user directories are dynamically bind-mounted to give that particular user access to their directory. For FirefoxOS, we don't currently have the notion of users. In our security model, we wanted to restrict access to the internal sdcard, so we leave the default permissions, don't do the bind mounting and require that the parent process (which runs as root) opens the file. The AID_SDCARD_R and AID_SDCARD_RW stuff only applies to external sdcards, which use the FAT file system and don't have any type of permissions control.
Thanks Dave! I learned a lot from your reply :)
Comment on attachment 8346695 [details] [diff] [review] Device Storage changes to remote CreateFd to the parent v5 Review of attachment 8346695 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Sorry it took me a while to get to it. Nothing big here, just a few little things: ::: dom/devicestorage/DeviceStorageRequestParent.cpp @@ +34,5 @@ > > void > DeviceStorageRequestParent::Dispatch() > { > + nsresult rv; |DebugOnly<nsresult>| here, I think? @@ +70,5 @@ > + nsRefPtr<CancelableRunnable> r = new CreateFdEvent(this, dsf); > + > + nsCOMPtr<nsIEventTarget> target > + = do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID); > + NS_ASSERTION(target, "Must have stream transport service"); s/NS_ASSERTION/MOZ_ASSERT/, here and in a few more places. ::: dom/devicestorage/nsDeviceStorage.cpp @@ +1643,3 @@ > if (XRE_GetProcessType() == GeckoProcessType_Default) { > + rv = NS_DispatchToMainThread(this); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); This will warn about |rv| being unused on opt builds, make |rv| a |DebugOnly<nsresult>| to fix. @@ +2017,5 @@ > + MOZ_ASSERT(mDSFileDescriptor); > + MOZ_ASSERT(mRequest); > + } > + > +D NS_IMETHOD Run() Typo, 'D' @@ +2039,5 @@ > + > + nsresult rv = dsFile->CreateFileDescriptor(mDSFileDescriptor->mFileDescriptor); > + > + if (NS_FAILED(rv)) { > + dsFile->mFile->Remove(false); Probably want to warn if this fails. @@ +2468,5 @@ > + return NS_DispatchToCurrentThread(r); > + } > + > + if (XRE_GetProcessType() != GeckoProcessType_Default) { > + Nit: extra line? @@ +3427,5 @@ > + if (!ds) { > + nsRefPtr<DOMRequest> request = new DOMRequest(win); > + r = new PostErrorEvent(request, POST_ERROR_EVENT_UNKNOWN); > + rv = NS_DispatchToCurrentThread(r); > + request.forget(aRequest); Outparams should only be addrefed if you're returning a success code. Otherwise the outparams are normally ignored and this will leak. (Though, in this case, it's very unlikely that NS_DispatchToCurrentThread can fail when you're on the main thread.) @@ +3449,5 @@ > + aDSFileDescriptor); > + } > + > + rv = NS_DispatchToCurrentThread(r); > + request.forget(aRequest); Same here.
Attachment #8346695 - Flags: review?(bent.mozilla) → review+
Blocks: gonk-kk
No longer blocks: gonk-kk
Depends on: 977373
Blocks: 989218
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: