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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

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+.

Comment 1

[Dave Hylands [:dhylands]]

Good catch.

Comment 2

[Dave Hylands [:dhylands]]

It looks like DeleteFileEvent suffers from the same malady

Comment 3

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

Attachment: Make addNamed() return an absolute path when called in the b2g parent process

Comment 4

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

(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.

Comment 5

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

Attachment: [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.

Comment 6

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

Attachment: [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

Comment 7

[Dave Hylands [:dhylands]]

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

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/72b876e3d06b

Comment 9

[Alex Keybl [:akeybl]]

Blocks a hard blocker.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/72b876e3d06b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e62eb4c456a5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/e62eb4c456a5

Comment 13

[dkumar]

Can you please provide steps to verify this fix? - to perform blackbox testing

Comment 14

[Dave Hylands [:dhylands]]

(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

[dkumar]

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.