773798 - Multi-process support for MediaStorage - Use PBlob

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Doug Turner (:dougt)]

+++ This bug was initially created as a clone of Bug #761930 +++

MediaStorage needs to work from content processes.

We will land support without cross-process Blobs.  This bug is to track the changes required to support pblobs.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 648454 [details] [diff] [review]
patch v.1

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

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +156,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +  nsString mime;
> +  mime.AssignWithConversion(mMimeType);
> +  CopyASCIItoUTF16(mMimeType, mime);

You want just this latter line I think.

@@ +161,5 @@
> +
> +  nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(mFile->mPath, mime, mLength, mFile->mFile);
> +
> +  ContentParent* cp = static_cast<ContentParent*>(mParent->Manager());
> +  BlobParent* actor = cp->GetOrCreateActorForBlob(blob);

This can fail if the child crashes.

::: dom/devicestorage/DeviceStorageRequestParent.h
@@ +75,5 @@
>        ~WriteFileEvent();
>        NS_IMETHOD Run();
>      private:
>        DeviceStorageRequestParent* mParent;
> +      nsRefPtr<nsIDOMBlob> mBlob;

nsCOMPtr.

Though, I think you have a patch waiting which makes this unnecessary now that you don't have to hold the blob alive while using the stream.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +904,5 @@
>  
>    NS_IMETHOD Run()
>    {
>      NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
> +    

Nit: You've got some extra whitespace here.

@@ +906,5 @@
>    {
>      NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
> +    
> +    nsCOMPtr<nsIInputStream> stream;
> +    mBlob->GetInternalStream(getter_AddRefs(stream));

This can fail for the remote blobs if the child process crashes.

::: dom/devicestorage/nsDeviceStorage.h
@@ +51,5 @@
>  
>    // we want to make sure that the names of file can't reach
>    // outside of the type of storage the user asked for.
>    bool IsSafePath();
>    

Nit: You've got some extra whitespace here.

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.2

this changes the ownership of the parent to be refcounted.  it also notifies any outstanding runnables if the parent is ipdl destroyed.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 648791 [details] [diff] [review]
patch v.2

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

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +33,5 @@
> +      BlobParent* bp = static_cast<BlobParent*>(p.blobParent());
> +      nsCOMPtr<nsIDOMBlob> blob = bp->GetBlob();
> +
> +      nsCOMPtr<nsIInputStream> stream;
> +      blob->GetInternalStream(getter_AddRefs(stream));

This will fail if the child crashes, need to handle it somehow.

@@ +175,5 @@
> +
> +  nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(mFile->mPath, mime, mLength, mFile->mFile);
> +
> +  ContentParent* cp = static_cast<ContentParent*>(mParent->Manager());
> +  BlobParent* actor = cp->GetOrCreateActorForBlob(blob);

This can also fail if the child crashes... Though I guess it doesn't really matter, the Send__delete__ call below will fail too and it seems like it doesn't matter here.

::: dom/devicestorage/DeviceStorageRequestParent.h
@@ +20,5 @@
>  class DeviceStorageRequestParent : public PDeviceStorageRequestParent
>  {
>  public:
>    DeviceStorageRequestParent(const DeviceStorageParams& aParams);
>    ~DeviceStorageRequestParent();

Nit: Now that you've made this refcounted please make this protected.

@@ +30,4 @@
>  private:
> +  nsAutoRefCnt mRefCnt;
> +
> +  class nsCancelableRunnable : public nsRunnable

Nit: The 'ns' prefix made me think this was living somewhere global, not an inner class. Not a big deal, but if you don't care then let's remove it.

@@ +55,5 @@
> +    void Cancel() {
> +      mCancelled = true;
> +    }
> +
> +    bool IsCancelled() {

This appears to be unused.

@@ +64,5 @@
> +
> +  protected:
> +    nsRefPtr<DeviceStorageRequestParent> mParent;
> +  private:
> +    bool mCancelled;

Nit: mCanceled.

@@ +162,5 @@
>        nsString mPath;
>    };
>  
> +protected:
> +  void AddRunnable(class nsCancelableRunnable* aRunnable) {

Nit: is the 'class' needed here? (Or below?)

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +819,5 @@
>    {
>      NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
>  
> +    nsCOMPtr<nsIInputStream> stream;
> +    mBlob->GetInternalStream(getter_AddRefs(stream));

This can fail so you'd better check.

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 6

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3664e5ab11b3

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3664e5ab11b3