Closed Bug 893019 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 ? verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

(Keywords: regression, Whiteboard: [LeoVB+])

Attachments

(2 files, 1 obsolete file)

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
(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.
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
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+
Blocks a hard blocker.
blocking-b2g: leo? → leo+
https://hg.mozilla.org/mozilla-central/rev/72b876e3d06b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: LeoVB+
Whiteboard: LeoVB+ → [LeoVB+]
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.
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.
See Also: → 912957
You need to log in before you can comment on or make changes to this bug.