Last Comment Bug 773798 - Multi-process support for MediaStorage - Use PBlob
: Multi-process support for MediaStorage - Use PBlob
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Doug Turner (:dougt)
:
Mentors:
Depends on: 759427 761930 767894 767905 768654
Blocks: e10s-device-apis b2g-e10s-work
  Show dependency treegraph
 
Reported: 2012-07-13 14:33 PDT by Doug Turner (:dougt)
Modified: 2012-08-10 01:58 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
patch v.1 (20.81 KB, patch)
2012-08-02 13:03 PDT, Doug Turner (:dougt)
bent.mozilla: review+
Details | Diff | Splinter Review
patch v.2 (33.72 KB, patch)
2012-08-03 11:49 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.2 (33.65 KB, patch)
2012-08-09 14:38 PDT, Doug Turner (:dougt)
bent.mozilla: review+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2012-07-13 14:33:42 PDT
+++ 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) 2012-08-02 13:03:08 PDT
Created attachment 648454 [details] [diff] [review]
patch v.1
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-02 22:41:21 PDT
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) 2012-08-03 11:49:34 PDT
Created attachment 648791 [details] [diff] [review]
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!) 2012-08-07 14:59:22 PDT
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) 2012-08-09 14:38:07 PDT
Created attachment 650686 [details] [diff] [review]
patch v.2
Comment 7 Ed Morley [:emorley] 2012-08-10 01:58:25 PDT
https://hg.mozilla.org/mozilla-central/rev/3664e5ab11b3

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