[DeviceStorage] addNamed() returns relative path when called in b2g parent process

RESOLVED FIXED in Firefox 25, Firefox OS v1.1hd

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

({regression})

unspecified
1.1 QE4 (15jul)
ARM
Gonk (Firefox OS)
regression

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18? verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [LeoVB+])

Attachments

(2 attachments, 1 obsolete attachment)

The following code in camera.js[1] results in different values getting stored in |this._videoRootDir| depending on whether it is run in the b2g parent process (as from a passcoded lockscreen) or as a regular app:

    DCFApi.createDCFFilename(this._videoStorage,
                             'video',
                             (function(path, name) {
      this._videoPath = path + name;

      // The CameraControl API will not automatically create directories
      // for the new file if they do not exist, so write a dummy file
      // to the same directory via DeviceStorage to ensure that the directory
      // exists before recording starts.
      var dummyblob = new Blob([''], {type: 'video/3gpp'});
      var dummyfilename = path + '.' + name;
      dump("mikeh: dummyfilename='" + dummyfilename + "'");
      var req = this._videoStorage.addNamed(dummyblob, dummyfilename);
      req.onerror = onerror;
      req.onsuccess = (function fileCreated(e) {
        // Extract video root directory string
        var absolutePath = e.target.result;
        dump("mikeh: absolutePath='" + absolutePath + "'");
        var rootDirLength = absolutePath.length - dummyfilename.length;
        this._videoRootDir = absolutePath.substring(0, rootDirLength);
        dump("mikeh: this._videoRootDir='" + this._videoRootDir + "'");

        this._videoStorage.delete(absolutePath); // No need to wait for success
        // Determine the number of bytes available on disk.
        var spaceReq = this._videoStorage.freeSpace();
        spaceReq.onerror = onerror;
        spaceReq.onsuccess = function() {
          startRecording(spaceReq.result);
        };
      }).bind(this);
    }).bind(this));

When run as a regular app:
07-12 11:25:15.970  2702  2702 I GeckoDump: mikeh: absolutePath='/sdcard/DCIM/101MZLLA/.VID_0033.3gp'
07-12 11:25:15.970  2702  2702 I GeckoDump: mikeh: this._videoRootDir='/sdcard/'

When run from a passcoded lockscreen:
07-12 11:24:31.827  2541  2541 I GeckoDump: mikeh: absolutePath='DCIM/100MZLLA/.VID_0025.3gp'
07-12 11:24:31.827  2541  2541 I GeckoDump: mikeh: this._videoRootDir=''

The difference is due to this condition[2] in DeviceStorageRequest::Allow(). If |XRE_GetProcessType()| returns |GeckoProcessType_Default|, we call |WriteFileEvent()| directly; otherwise, we dispatch a DeviceStorageRequest to the parent process[3].

When the former code path completes, it dispatches a |PostResultEvent| runnable[4] that invokes |FireSuccess()| on the original DOM request[5], returning the relative path of the new file.

When the latter code path completes, it uses |DeviceStorageRequestChild::Recv__delete__()| to dispatch |FireSuccess()|[6]; in this case, there is an extra call to |mFile->GetCompositePath()| to get the argument for |FireSuccess()|.  This call prepends the root path to the returned pathname, making it absolute instead of relative.

1. https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L609
2. http://mxr.mozilla.org/mozilla-b2g18/source/dom/devicestorage/nsDeviceStorage.cpp#2194
3. http://mxr.mozilla.org/mozilla-b2g18/source/dom/devicestorage/nsDeviceStorage.cpp#2235
4. http://mxr.mozilla.org/mozilla-b2g18/source/dom/devicestorage/nsDeviceStorage.cpp#1908
5. http://mxr.mozilla.org/mozilla-b2g18/source/dom/devicestorage/nsDeviceStorage.cpp#1857
6. https://mxr.mozilla.org/mozilla-b2g18/source/dom/devicestorage/DeviceStorageRequestChild.cpp#52

Observed on:
- gecko: b2g18:777ab5f22878
- gaia: v1-train:7cdcc46179d198cab11970964b181ede32a5b683

Blocks a blocked, so should be marked leo+.
Assignee: nobody → mhabicher
Good catch.
It looks like DeleteFileEvent suffers from the same malady
Created attachment 774709 [details] [diff] [review]
Make addNamed() return an absolute path when called in the b2g parent process
Attachment #774709 - Flags: review?(dhylands)
(In reply to Dave Hylands [:dhylands] from comment #2)
>
> It looks like DeleteFileEvent suffers from the same malady

Good catch--I'll fix that one too.
Created attachment 774717 [details] [diff] [review]
[b2g18] Make addNamed() return an absolute path when called in the b2g parent process, v2

This is the (current) b2g18 version of this patch. m-c version coming soon.
Attachment #774709 - Attachment is obsolete: true
Attachment #774709 - Flags: review?(dhylands)
Attachment #774717 - Flags: review?(dhylands)
Attachment #774717 - Attachment description: Make addNamed() return an absolute path when called in the b2g parent process, v2 → [b2g18] Make addNamed() return an absolute path when called in the b2g parent process, v2
Created attachment 774721 [details] [diff] [review]
[m-c] Make addNamed() return an absolute path when called in the b2g parent process, v2

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=36dca5fb208a
Attachment #774721 - Flags: review?(dhylands)
Status: NEW → ASSIGNED
Comment on attachment 774721 [details] [diff] [review]
[m-c] Make addNamed() return an absolute path when called in the b2g parent process, v2

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

Looks good to me
Attachment #774721 - Flags: review?(dhylands) → review+
Attachment #774717 - Flags: review?(dhylands) → review+

Comment 9

5 years ago
Blocks a hard blocker.
blocking-b2g: leo? → leo+
https://hg.mozilla.org/mozilla-central/rev/72b876e3d06b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e62eb4c456a5
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
status-firefox25: --- → fixed
Target Milestone: --- → 1.1 QE4 (15jul)

Updated

5 years ago
Whiteboard: LeoVB+

Updated

5 years ago
Whiteboard: LeoVB+ → [LeoVB+]

Comment 13

5 years ago
Can you please provide steps to verify this fix? - to perform blackbox testing
(In reply to dkumar from comment #13)
> Can you please provide steps to verify this fix? - to perform blackbox
> testing

Do you mean provide some JS which will verify that its working properly?

Or do mean using SW that already exists in FxOS? If the latter, then I'd image if you can verify bug 891507, then you're indirectly testing this.

Comment 15

5 years ago
Thankyou, no I do not need JS and will look into bug #891507 to tackle this issue.

 (In reply to Dave Hylands [:dhylands] from comment #14)
> (In reply to dkumar from comment #13)
> > Can you please provide steps to verify this fix? - to perform blackbox
> > testing
> 
> Do you mean provide some JS which will verify that its working properly?
> 
> Or do mean using SW that already exists in FxOS? If the latter, then I'd
> image if you can verify bug 891507, then you're indirectly testing this.

Updated

5 years ago
status-b2g18: fixed → verified
See Also: → bug 912957
You need to log in before you can comment on or make changes to this bug.