Last Comment Bug 761930 - Multi-process support for MediaStorage
: Multi-process support for MediaStorage
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Doug Turner (:dougt)
:
Mentors:
: 766203 (view as bug list)
Depends on: 759427 767894 767905 768654 775053
Blocks: e10s-device-apis b2g-e10s-work 773798
  Show dependency treegraph
 
Reported: 2012-06-05 23:48 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-07-30 15:42 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
patch v.1 (78.93 KB, patch)
2012-06-27 14:00 PDT, Doug Turner (:dougt)
jonas: review+
josh: feedback+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-05 23:48:01 PDT
MediaStorage needs to work from content processes.

We can begin work on this without cross-process Blobs, but can't land anything useful.
Comment 1 Andrew Overholt [:overholt] 2012-06-19 12:12:09 PDT
*** Bug 766203 has been marked as a duplicate of this bug. ***
Comment 2 Doug Turner (:dougt) 2012-06-27 14:00:49 PDT
Created attachment 637248 [details] [diff] [review]
patch v.1

This patch needs to be applied on top of bug 767894, 767905, and 768654.

Passes all existing test for non-ipc.
Passes all but one(*) test for ipc

* we have a todo/followup of supporting the directory service in child process mochitests


The way we are creating blobs in the child process is terrible.  we are currently copying the data from the parent to the child.  This is a workaround until bent fixes bug 759427.

Asking for feedback on the approach and review on everything but the pieces relating to copying data from the parent to the child.  I marked those areas in code with comments.
Comment 3 Doug Turner (:dougt) 2012-07-09 14:48:50 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c02ee4f1ad13
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-13 02:22:29 PDT
Comment on attachment 637248 [details] [diff] [review]
patch v.1

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

r=me with those things fixed. And maybe have someone check the permission-prompt refcounting stuff.

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +103,5 @@
> +
> +DeviceStorageRequestParent::PostErrorEvent::~PostErrorEvent() {}
> +
> +NS_IMETHODIMP
> +DeviceStorageRequestParent::PostErrorEvent::Run() {

Put the '{' on a new line.

@@ +111,5 @@
> +  return NS_OK;
> +}
> +
> +
> +DeviceStorageRequestParent::PostSuccessEvent::PostSuccessEvent(DeviceStorageRequestParent* aParent)

Is this class used anywhere?

@@ +349,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +  SuccessResponse response;
> +  unused <<  mParent->Send__delete__(mParent, response);

Shouldn't you send mPath here somewhere? I'm surprised this passes tests?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +51,5 @@
> +  aFile->Clone(getter_AddRefs(mFile));
> +}
> +
> +void
> +DeviceStorageFile::setPath(const nsAString& aPath) {

Nit: Fix weird capitalization. Also on some other functions in this class.

@@ +236,5 @@
> +    nsString fullpath;
> +    f->GetPath(fullpath);
> +
> +    if (!StringBeginsWith(fullpath, aRootPath)) {
> +      NS_WARNING("collectFiles returned a path that does not belong!");

Change to NS_ERROR so that we assert.

@@ +243,5 @@
> +
> +    nsAString::size_type len = aRootPath.Length() + 1; // +1 for the trailing /
> +    nsDependentSubstring newPath = Substring(fullpath, len);
> +    nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(f);
> +    dsf->setPath(newPath);

When isDir is true here you are doing an unneccesary allocation. The DeviceStorageFile is just going to be used internally here and isn't going to survive exiting the function.

One solution would be to stack-allocate it in the if(isDir) branch.

@@ +334,5 @@
>  
>    // in testing, we have access to a few more directory locations
>    if (mozilla::Preferences::GetBool("device.storage.testing", false)) {
>  
> +    if (XRE_GetProcessType() != GeckoProcessType_Default) {

Why don't you have to do the proxying to parent process for the music/pictures folders?

@@ +339,5 @@
> +      nsString path;
> +
> +      if (aType.Equals(NS_LITERAL_STRING("temp")) && aIndex == 0) {
> +	ContentChild::GetSingleton()->SendGetDirectoryPath(NS_LITERAL_STRING("temp"), &path);
> +	if (!path.Equals(NS_LITERAL_STRING(""))) {

if (!path.IsEmpty())

@@ +345,5 @@
> +	}
> +      }
> +      else if (aType.Equals(NS_LITERAL_STRING("profile")) && aIndex == 0) {
> +	ContentChild::GetSingleton()->SendGetDirectoryPath(NS_LITERAL_STRING("profile"), &path);
> +	if (!path.Equals(NS_LITERAL_STRING(""))) {

Same here

@@ +470,5 @@
> +  }
> +
> +  void IPDLRelease()
> +  {
> +    Release();

I still don't understand this part well enough that I feel that I can review it. Would be great if someone else could vouch for this.

::: dom/ipc/ContentParent.cpp
@@ +741,5 @@
> +    f->GetPath(*path);
> +  }
> +  else {
> +    // better way to return an error?
> +    path->Assign(NS_LITERAL_STRING(""));

path->Truncate();

::: dom/ipc/PContent.ipdl
@@ +163,5 @@
>      PAudio(PRInt32 aNumChannels, PRInt32 aRate, PRInt32 aFormat);
>  
> +    PDeviceStorageRequest(DeviceStorageParams params);
> +
> +    // for testing only - returns pathes for temp and profile

"paths"
Comment 5 Josh Matthews [:jdm] 2012-07-13 07:59:10 PDT
Comment on attachment 637248 [details] [diff] [review]
patch v.1

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

Given that there are now so many implementations of PCOMContentPermissionRequest, I would like to propose some runtime asserts to help catch any misuses of the class. Specifically, add a member variable mIPCOpen that is initialized to true, and is set to false in a method that is called by TabChild::DeallocPContentPermissionRequest. Add a destructor to PCOMContentPermissionRequest that asserts that mIPCOpen is false, which should catch any cases when the request object dies before IPDL would expect it to.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +470,5 @@
> +  }
> +
> +  void IPDLRelease()
> +  {
> +    Release();

I vouch. This follows the contract laid out at http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PCOMContentPermissionRequestChild.h#15 which ensures that protocols that depend on XPCOM objects being alive at certain points won't be surprised by finding dead objects instead.
Comment 6 Doug Turner (:dougt) 2012-07-13 09:33:53 PDT
josh, thanks.  I agree with the protection against misuse.  i filed bug 773683 to track.
Comment 7 Doug Turner (:dougt) 2012-07-13 10:17:39 PDT
> Shouldn't you send mPath here somewhere? I'm surprised this passes tests?

Right, the parent never sees the relative paths.  The child just sends up the char* full path the the parent.   When the parent sends its response, the DeviceStorageRequestChild uses the relative path stored in its DeviceStorageFile (mFile).


> Why don't you have to do the proxying to parent process for the music/pictures folders?

Good question.  Ignoring the temp directory for a second, the profile directory doesn't exist in the child process -- you can't ask the directory service at all for that directory.

Digging a bit deeper, there are basically two types of directory service providers.  The first type are directories that are system specific and are defined here:

  http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsDirectoryServiceDefs.h

These defines define where music, photos, and videos live.  For B2G, we just have hard coded places for now.

The second type of provider is application specific, those are defined here:

  http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsAppDirectoryServiceDefs.h

In general, only the directories that are system specific are exposed to the child process.

Given that the temp should be exposed in the child, i will rework this code a bit so that temp always goes through the normal path.
Comment 8 Doug Turner (:dougt) 2012-07-13 13:04:51 PDT
i filed bug 773713 to clean up using the profile directory for testing.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-07-13 21:09:21 PDT
Sorry, but your push had to be backed out due to compilation errors on all platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7db81ae082e7

/builds/slave/m-in-lnx/build/dom/ipc/PContent.ipdl:166: error: ctor for protocol `PDeviceStorageRequest', which is not managed by protocol `PContent'
Specification is not well typed.
Comment 11 Doug Turner (:dougt) 2012-07-13 22:37:47 PDT
bad rebase - https://hg.mozilla.org/integration/mozilla-inbound/rev/8349edd9f8a1
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-07-14 10:02:11 PDT
https://hg.mozilla.org/mozilla-central/rev/8349edd9f8a1

Note You need to log in before you can comment on or make changes to this bug.