Multi-process support for MediaStorage - Use PBlob

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

33.65 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
+++ 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.
Depends on: 778968
No longer depends on: 778968
(Assignee)

Comment 1

5 years ago
Created attachment 648454 [details] [diff] [review]
patch v.1
Attachment #648454 - Flags: review?(bent.mozilla)
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.
Attachment #648454 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 3

5 years ago
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.
Attachment #648454 - Attachment is obsolete: true
Attachment #648791 - Flags: review?(bent.mozilla)
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.
(Assignee)

Comment 5

5 years ago
Created attachment 650686 [details] [diff] [review]
patch v.2
Attachment #648791 - Attachment is obsolete: true
Attachment #648791 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Attachment #650686 - Flags: review?(bent.mozilla)
Attachment #650686 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3664e5ab11b3
https://hg.mozilla.org/mozilla-central/rev/3664e5ab11b3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.