806503 - [memory-backed blobs don't have a long enough lifetime for use in activities

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Comment 1

[David Flanagan [:djf]]

When an inline activity returns a memory backed blob as its result, the sending app will be closed shortly after that result is sent. The receiving app (the one that initiated the activity) will receive the blob, but it appears to become invalid when the Gaia window manager kills the app that handled the activity.

It seems to me that gecko needs to make a defensive copy of memory backed blobs that are transferred via activities.

I observed this bug with the Gaia contacts app picking an image from the Gaia camera app. If the camera sent the memory-backed blob it got from the camera hardware, then the contacts app would be able to display it, but later, when it needed to save the blob, it would crash.

When I changed the camera app to save the photo first and send the file-backed blob from device storage instead, the crash went away.

I haven't done any more testing than that, however.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

David: You didn't nominate this for blocking? Does this mean that you have a workaround in place which makes this non-critical for v1?

Bent: I thought that we did a "defensive copy" of memory backed blobs in this case?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bent: Need your input on if this is blocking.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bent: Need your input on if this is blocking.

Comment 5

[David Flanagan [:djf]]

I probably should have nominated it because it seems like it could be a source of really confusing bugs for third party apps. (Though maybe they're not allowed to implement activities)

I'm not aware of any places in gaia where this currently blocks us. It seems like developer education may be enough here to avoid the issue: either the sending app needs to make sure it sends a file-backed blob, or the receiving app needs to be sure that it uses the blob synchronously when it receives it (though that solution seems much more brittle.)

Comment 6

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

I think this should block based on our discussion earlier.

Comment 7

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 8

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 9

[Dietrich Ayala (:dietrich)]

No update in two weeks. Is this blocking for real? Is there a plan to fix it?

We're ~2 weeks out until C3 is over, and this is the kind of thing that tends to drag on.

Comment 10

[Andrew Overholt [:overholt]]

Andrea, can you take this with Ben reviewing?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Kyle: We know you're not the ideal assignee for this bug, but would you be able to give it a shot. There are few people that have the right chops to fix this and bent have bugs that are more urgent.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

I tried reproducing this bug to see if I could help but following the description in comment 1 I cannot get the contacts app to crash. The only thing I see in logcat is the following line printed out by the camera application:

I/Gecko   ( 1664): ###!!! [Child][RPCChannel] Error: Channel closing: too late to send/recv, messages will be lost

I'm not sure if it's related or not.

Comment 13

[David Flanagan [:djf]]

I worked around this bug as soon as I reported it, so comment 1 was never meant to be steps to reproduce.  I think you'd have to create a pair of test apps, to initiate and handle an activity and try to pass a memory-backed blob.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 15

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

Comment on attachment 699608 [details] [diff] [review]
Patch

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

Here are some things to clean up, but I'm not sure this is the simplest way to make this all work. I need to play around with it a little. More soon.

::: dom/ipc/Blob.cpp
@@ +826,5 @@
>        normalParams.length() = mLength;
>  
> +      BlobConstructorNoMultipartParams params(normalParams);
> +
> +      ActorType* newActor = ActorType::Create(params);

I think you can just pass 'normalParams' here and a temp BlobConstructorNoMultipartParams will be created?

@@ +837,5 @@
>        slicedParams.end() = mStart + mLength;
>        SetBlobOnParams(mActor, slicedParams);
>  
> +      BlobConstructorNoMultipartParams params2(slicedParams);
> +      if (mActor->ConstructPBlobOnManager(newActor, params2)) {

Same here, you should be able to pass slicedParams directly.

@@ +897,3 @@
>    {
>      MOZ_ASSERT(!aActor || !mActor);
> +    mActor = reinterpret_cast<ActorType*>(aActor);

Nit: static_cast

@@ +951,5 @@
>  };
>  
>  template <ActorFlavorEnum ActorFlavor>
> +class RemoteMemoryBlob : public nsDOMMemoryFile,
> +                         public nsIRemoteBlob

This stuff can be refactored a little... Can they all just inherit RemoteBlob?

Otherwise we should make a base class RemoteBlobBase that has the actor member and virtual destructor and nsIRemoteBlob impl, then make each of these inherit that.

@@ +962,5 @@
> +
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  RemoteMemoryBlob(void *aMemoryBuffer,

Nit: * on the left, below too.

@@ +1003,5 @@
> +  void
> +  SetActor(void* aActor) MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(!aActor || !mActor);
> +    mActor = reinterpret_cast<ActorType*>(aActor);

Nit: static_cast

@@ +1013,5 @@
> +    return static_cast<typename ActorType::ProtocolType*>(mActor);
> +  }
> +
> +  NS_IMETHOD
> +  GetLastModifiedDate(JSContext* cx, JS::Value* aLastModifiedDate)

Why are you needing to override this? Shouldn't the base class impl handle this?

@@ +1125,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsRefPtr<RemoteBlobType> remoteBlob;
> +  nsRefPtr<RemoteMemoryBlobType> remoteMemoryBlob;

Just move these inside the case statements and call SetRemoteBlob inside there now that this is all different.

@@ +1167,5 @@
> +          const NormalBlobConstructorParams& normalParams =
> +            internalParams.get_NormalBlobConstructorParams();
> +          MOZ_ASSERT(normalParams.length() == params.data().Length());
> +
> +          void* data = moz_xmalloc(params.data().Length());

This should be fallible in the parent process, need to figure out another way.

@@ +1311,5 @@
>    if (mRemoteBlob && mOwnsBlob) {
>      blob = dont_AddRef(mBlob);
>      mOwnsBlob = false;
>    }
> +  else if (mRemoteMemoryBlob && mOwnsBlob) {

What about mRemoteMultipartBlob?

@@ +1380,5 @@
> +  if (mBlobManager) {
> +    return mBlobManager->SendPBlobConstructor(aActor, aParams);
> +  }
> +
> +  MOZ_NOT_REACHED();

This macro needs an argument.

@@ +1404,5 @@
> +
> +  mBlobManager = aManager;
> +}
> +
> +

Nit: Extra newline

@@ +1586,5 @@
> +  Blob<ActorFlavor>* subBlobActor = static_cast<Blob<ActorFlavor>*>(subBlob);
> +
> +  if (!subBlobActor->ManagerIs(this)) {
> +    // Somebody screwed up!
> +    return false;

This should be debug-only, MOZ_ASSERT or something.

@@ +1601,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(mBlob);
>    MOZ_ASSERT(!mRemoteBlob);
> +  MOZ_ASSERT(!mRemoteMemoryBlob);

What about mRemoteMultipartBlob?

@@ +1657,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(mBlob);
>    MOZ_ASSERT(!mRemoteBlob);
> +  MOZ_ASSERT(!mRemoteMemoryBlob);

What about mRemoteMultipartBlob?

::: dom/ipc/Blob.h
@@ +95,5 @@
> +class RemoteMemoryBlob;
> +template <ActorFlavorEnum>
> +class RemoteMultipartBlob;
> +
> +bool GetParamsForBlob(nsIDOMBlob* aBlob, BlobConstructorParams* aOutParams);

Nit: bool on its own line

@@ +126,5 @@
> +  RemoteMultipartBlobType* mRemoteMultipartBlob;
> +
> +  ContentManagerType* mContentManager;
> +  BlobManagerType* mBlobManager;
> +  

Nit: whitespace

@@ +159,5 @@
>    SetMysteryBlobInfo(const nsString& aContentType, uint64_t aLength);
>  
> +  ProtocolType*
> +  ConstructPBlobOnManager(ProtocolType* aActor,
> +                          const BlobConstructorParams& aParams) NS_WARN_UNUSED_RESULT;

The NS_WARN_UNUSED_RESULT should only exist in the parent version, let's just lose it.

@@ +176,5 @@
> +
> +  bool
> +  HasManager() const
> +  {
> +    return !!mContentManager || !!mBlobManager;

This should always be true, right? All blobs must be managed by something. Let's just remove this.

@@ +182,5 @@
> +
> +  void
> +  SetManager(ContentManagerType* aManager);
> +  void
> +  SetManager(BlobManagerType* aManager);

These should be set in the constructor (er, Create() functions) as they're immutable.

@@ +195,5 @@
> +  // These constructors are called on the receiving side.
> +  Blob(const BlobConstructorNoMultipartParams& aParams);
> +  Blob(nsRefPtr<RemoteMultipartBlobType>& aBlob);
> +
> +  ~Blob() {}

Nit: remove

@@ +202,5 @@
>    SetRemoteBlob(nsRefPtr<RemoteBlobType>& aRemoteBlob);
> +  void
> +  SetRemoteBlob(nsRefPtr<RemoteMemoryBlobType>& aRemoteMemoryBlob);
> +  void
> +  SetRemoteBlob(nsRefPtr<RemoteMultipartBlobType>& aRemoteMemoryBlob);

This should be templatized, they all do the same thing.

::: dom/ipc/ContentChild.cpp
@@ +563,5 @@
> +
> +  BlobConstructorParams resultParams;
> +
> +  if (!blob->IsMemoryBacked() && (blob->IsSizeUnknown() ||
> +                                  blob->IsDateUnknown())) {

Nit:

  if (a &&
      (b || c)) {
  }

@@ +609,5 @@
> +      const nsDOMMemoryFile* memoryBlob = static_cast<const nsDOMMemoryFile*>(blob);
> +
> +      InfallibleTArray<uint8_t> data;
> +      data.AppendElements(reinterpret_cast<uint8_t*>(memoryBlob->GetData()),
> +                          memoryBlob->GetLength());

We should cheat here and use memcpy rather than AppendElements (nsTArray isn't smart enough to do that yet).

@@ +614,5 @@
> +
> +      MemoryBlobOrFileConstructorParams memoryParams(params, data);
> +      resultParams = memoryParams;
> +    }
> +    else if (const nsTArray<nsCOMPtr<nsIDOMBlob> >* subBlobs = blob->GetSubBlobs()) {

Nit: you don't use subBlobs here, just do |else if (blob->GetSubBlobs())|

@@ +659,5 @@
>  
> +  actor->SetManager(this);
> +
> +  if (const nsTArray<nsCOMPtr<nsIDOMBlob> >* subBlobs =
> +        static_cast<nsDOMFileBase*>(aBlob)->GetSubBlobs()) {

I'd prefer we only do this cast in one place. Can you change GetParamsForBlob() to take a nsDOMFileBase* and do this before calling?

@@ +667,5 @@
> +        return nullptr;
> +      }
> +
> +      BlobChild* subActor = BlobChild::Create(aBlob);
> +      if (!actor->SendPBlobConstructor(subActor, subParams)) {

In the child process this is infallible, so no need to check. (Bonus point possibility: fix the other SendPBlobConstructor above!)

::: dom/ipc/ContentChild.h
@@ +196,5 @@
>      uint64_t GetID() { return mID; }
>  
> +    bool GetParamsForBlob(nsIDOMBlob* aBlob, BlobConstructorParams* aOutParams);
> +    BlobChild* GetOrCreateActorForBlob(nsIDOMBlob* aBlob,
> +                                       BlobConstructorParams* aParams = nullptr);

Is this aParams ever non-null? I don't see where you use it.

::: dom/ipc/ContentParent.cpp
@@ +1347,5 @@
> +{
> +  // XXX This is only safe so long as all blob implementations in our tree
> +  //     inherit nsDOMFileBase. If that ever changes then this will need to grow
> +  //     a real interface or something.
> +  const nsDOMFileBase* blob = static_cast<nsDOMFileBase*>(aBlob);

Let's only do this once like in ContentChild.

@@ +1352,5 @@
> +
> +  BlobConstructorParams resultParams;
> +
> +  if (!blob->IsMemoryBacked() && (blob->IsSizeUnknown() ||
> +                                  blob->IsDateUnknown())) {

Same nit here from ContentChild.

@@ +1398,5 @@
> +      const nsDOMMemoryFile* memoryBlob = static_cast<const nsDOMMemoryFile*>(blob);
> +
> +      InfallibleTArray<uint8_t> data;
> +      data.AppendElements(reinterpret_cast<uint8_t*>(memoryBlob->GetData()),
> +                          memoryBlob->GetLength());

memcpy again

@@ +1403,5 @@
> +
> +      MemoryBlobOrFileConstructorParams memoryParams(params, data);
> +      resultParams = memoryParams;
> +    }
> +    else if (const nsTArray<nsCOMPtr<nsIDOMBlob> >* subBlobs = blob->GetSubBlobs()) {

Same nit here from ContentChild.

::: dom/ipc/ContentParent.h
@@ +126,5 @@
>      }
>  
> +    bool GetParamsForBlob(nsIDOMBlob* aBlob, BlobConstructorParams* aOutParams);
> +    BlobParent* GetOrCreateActorForBlob(nsIDOMBlob* aBlob,
> +                                        BlobConstructorParams* aParams = nullptr);

Same, why have an outparam here?

::: dom/ipc/DOMTypes.ipdlh
@@ +30,5 @@
>    uint64_t length;
>    uint64_t modDate;
>  };
>  
> +union BlobOrFileConstructorParams {

Nit: { on its own line

@@ +41,5 @@
> +  BlobOrFileConstructorParams constructorParams;
> +  uint8_t[] data;
> +};
> +
> +

Nit: extra newline

::: dom/ipc/PBlob.ipdl
@@ +29,5 @@
>    PBlobStream();
>  
>    ResolveMystery(ResolveMysteryParams params);
> +
> +  AddSubBlob(PBlob subBlob);

This message is unnecessary, you can move the sub blob logic to the PBlob constructor.

::: dom/ipc/nsIRemoteBlob.h
@@ +21,5 @@
>  
>    // This will either return a PBlobChild or PBlobParent.
>    virtual void* GetPBlob() = 0;
> +
> +  virtual void SetActor(void* aActor) = 0;

Nit: SetPBlob would be better.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ben says he has a better approach.

Comment 17

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

...in theory...

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to ben turner [:bent] from comment #15)
> Comment on attachment 699608 [details] [diff] [review]
> Patch
> 
> Review of attachment 699608 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here are some things to clean up, but I'm not sure this is the simplest way
> to make this all work. I need to play around with it a little. More soon.
> 
> ::: dom/ipc/Blob.cpp
> @@ +826,5 @@
> >        normalParams.length() = mLength;
> >  
> > +      BlobConstructorNoMultipartParams params(normalParams);
> > +
> > +      ActorType* newActor = ActorType::Create(params);
> 
> I think you can just pass 'normalParams' here and a temp
> BlobConstructorNoMultipartParams will be created?

c:/dev/mozilla-inbound/dom/ipc/Blob.cpp(830) : error C2665: 'mozilla::dom::ipc::
Blob<ActorFlavor>::Create' : none of the 2 overloads could convert all the argum
ent types
        with
        [
            ActorFlavor=Child
        ]
        c:\dev\mozilla-inbound\dom\ipc\Blob.h(137): could be 'mozilla::dom::ipc:
:Blob<ActorFlavor> *mozilla::dom::ipc::Blob<ActorFlavor>::Create(nsIDOMBlob *)'
        with
        [
            ActorFlavor=Child
        ]
        c:\dev\mozilla-inbound\dom\ipc\Blob.h(144): or       'mozilla::dom::ipc:
:Blob<ActorFlavor> *mozilla::dom::ipc::Blob<ActorFlavor>::Create(const mozilla::
dom::ipc::Blob<ActorFlavor>::BlobConstructorParams &)'
        with
        [
            ActorFlavor=Child
        ]
        while trying to match the argument list '(mozilla::dom::NormalBlobConstr
uctorParams)'

> @@ +837,5 @@
> >        slicedParams.end() = mStart + mLength;
> >        SetBlobOnParams(mActor, slicedParams);
> >  
> > +      BlobConstructorNoMultipartParams params2(slicedParams);
> > +      if (mActor->ConstructPBlobOnManager(newActor, params2)) {
> 
> Same here, you should be able to pass slicedParams directly.

c:/dev/mozilla-inbound/dom/ipc/Blob.cpp(841) : error C2664: 'mozilla::dom::ipc::
Blob<ActorFlavor>::ConstructPBlobOnManager' : cannot convert parameter 2 from 'm
ozilla::dom::SlicedBlobConstructorParams' to 'const mozilla::dom::ipc::Blob<Acto
rFlavor>::BlobConstructorParams &'
        with
        [
            ActorFlavor=Child
        ]
        Reason: cannot convert from 'mozilla::dom::SlicedBlobConstructorParams'
to 'const mozilla::dom::ipc::Blob<ActorFlavor>::BlobConstructorParams'
        with
        [
            ActorFlavor=Child
        ]
        No user-defined-conversion operator available that can perform this conv
ersion, or the operator cannot be called

> @@ +897,3 @@
> >    {
> >      MOZ_ASSERT(!aActor || !mActor);
> > +    mActor = reinterpret_cast<ActorType*>(aActor);
> 
> Nit: static_cast

Done.

> @@ +951,5 @@
> >  };
> >  
> >  template <ActorFlavorEnum ActorFlavor>
> > +class RemoteMemoryBlob : public nsDOMMemoryFile,
> > +                         public nsIRemoteBlob
> 
> This stuff can be refactored a little... Can they all just inherit
> RemoteBlob?

No, because we need different base classes for each.  We can't templatize the base class either because of the constructors.

> Otherwise we should make a base class RemoteBlobBase that has the actor
> member and virtual destructor and nsIRemoteBlob impl, then make each of
> these inherit that.

Done.

> @@ +962,5 @@
> > +
> > +public:
> > +  NS_DECL_ISUPPORTS_INHERITED
> > +
> > +  RemoteMemoryBlob(void *aMemoryBuffer,
> 
> Nit: * on the left, below too.

Done.

> @@ +1003,5 @@
> > +  void
> > +  SetActor(void* aActor) MOZ_OVERRIDE
> > +  {
> > +    MOZ_ASSERT(!aActor || !mActor);
> > +    mActor = reinterpret_cast<ActorType*>(aActor);
> 
> Nit: static_cast

Eliminated.

> @@ +1013,5 @@
> > +    return static_cast<typename ActorType::ProtocolType*>(mActor);
> > +  }
> > +
> > +  NS_IMETHOD
> > +  GetLastModifiedDate(JSContext* cx, JS::Value* aLastModifiedDate)
> 
> Why are you needing to override this? Shouldn't the base class impl handle
> this?

I don't know.  Why does RemoteBlob override this today?

> @@ +1125,5 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> >    nsRefPtr<RemoteBlobType> remoteBlob;
> > +  nsRefPtr<RemoteMemoryBlobType> remoteMemoryBlob;
> 
> Just move these inside the case statements and call SetRemoteBlob inside
> there now that this is all different.

Done.

> @@ +1167,5 @@
> > +          const NormalBlobConstructorParams& normalParams =
> > +            internalParams.get_NormalBlobConstructorParams();
> > +          MOZ_ASSERT(normalParams.length() == params.data().Length());
> > +
> > +          void* data = moz_xmalloc(params.data().Length());
> 
> This should be fallible in the parent process, need to figure out another
> way.



> @@ +1311,5 @@
> >    if (mRemoteBlob && mOwnsBlob) {
> >      blob = dont_AddRef(mBlob);
> >      mOwnsBlob = false;
> >    }
> > +  else if (mRemoteMemoryBlob && mOwnsBlob) {
> 
> What about mRemoteMultipartBlob?
> 
> @@ +1380,5 @@
> > +  if (mBlobManager) {
> > +    return mBlobManager->SendPBlobConstructor(aActor, aParams);
> > +  }
> > +
> > +  MOZ_NOT_REACHED();
> 
> This macro needs an argument.

Done.

> @@ +1404,5 @@
> > +
> > +  mBlobManager = aManager;
> > +}
> > +
> > +
> 
> Nit: Extra newline

Fixed.

> @@ +1586,5 @@
> > +  Blob<ActorFlavor>* subBlobActor = static_cast<Blob<ActorFlavor>*>(subBlob);
> > +
> > +  if (!subBlobActor->ManagerIs(this)) {
> > +    // Somebody screwed up!
> > +    return false;
> 
> This should be debug-only, MOZ_ASSERT or something.

No, we want to kill the child process if it messes this up because we could end up with use-after-frees in the parent when we tear down the actors in the wrong order.

> @@ +1601,5 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >    MOZ_ASSERT(mBlob);
> >    MOZ_ASSERT(!mRemoteBlob);
> > +  MOZ_ASSERT(!mRemoteMemoryBlob);
> 
> What about mRemoteMultipartBlob?

Fixed.

> @@ +1657,5 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >    MOZ_ASSERT(mBlob);
> >    MOZ_ASSERT(!mRemoteBlob);
> > +  MOZ_ASSERT(!mRemoteMemoryBlob);
> 
> What about mRemoteMultipartBlob?

Fixed.

> ::: dom/ipc/Blob.h
> @@ +95,5 @@
> > +class RemoteMemoryBlob;
> > +template <ActorFlavorEnum>
> > +class RemoteMultipartBlob;
> > +
> > +bool GetParamsForBlob(nsIDOMBlob* aBlob, BlobConstructorParams* aOutParams);
> 
> Nit: bool on its own line

This is actually an artifact of iteration on this patch.  Removed the declaration.

> @@ +126,5 @@
> > +  RemoteMultipartBlobType* mRemoteMultipartBlob;
> > +
> > +  ContentManagerType* mContentManager;
> > +  BlobManagerType* mBlobManager;
> > +  
> 
> Nit: whitespace

Removed.

> @@ +159,5 @@
> >    SetMysteryBlobInfo(const nsString& aContentType, uint64_t aLength);
> >  
> > +  ProtocolType*
> > +  ConstructPBlobOnManager(ProtocolType* aActor,
> > +                          const BlobConstructorParams& aParams) NS_WARN_UNUSED_RESULT;
> 
> The NS_WARN_UNUSED_RESULT should only exist in the parent version, let's
> just lose it.

Ok.

> @@ +176,5 @@
> > +
> > +  bool
> > +  HasManager() const
> > +  {
> > +    return !!mContentManager || !!mBlobManager;
> 
> This should always be true, right? All blobs must be managed by something.
> Let's just remove this.

It returns false if someone screws up setting a manager on the blob.  We use it for assertions.  I'll make it debug only.

> @@ +182,5 @@
> > +
> > +  void
> > +  SetManager(ContentManagerType* aManager);
> > +  void
> > +  SetManager(BlobManagerType* aManager);
> 
> These should be set in the constructor (er, Create() functions) as they're
> immutable.

Followup.
 
> @@ +195,5 @@
> > +  // These constructors are called on the receiving side.
> > +  Blob(const BlobConstructorNoMultipartParams& aParams);
> > +  Blob(nsRefPtr<RemoteMultipartBlobType>& aBlob);
> > +
> > +  ~Blob() {}
> 
> Nit: remove

Done.

> @@ +202,5 @@
> >    SetRemoteBlob(nsRefPtr<RemoteBlobType>& aRemoteBlob);
> > +  void
> > +  SetRemoteBlob(nsRefPtr<RemoteMemoryBlobType>& aRemoteMemoryBlob);
> > +  void
> > +  SetRemoteBlob(nsRefPtr<RemoteMultipartBlobType>& aRemoteMemoryBlob);
> 
> This should be templatized, they all do the same thing.

Except they don't.

> ::: dom/ipc/ContentChild.cpp
> @@ +563,5 @@
> > +
> > +  BlobConstructorParams resultParams;
> > +
> > +  if (!blob->IsMemoryBacked() && (blob->IsSizeUnknown() ||
> > +                                  blob->IsDateUnknown())) {
> 
> Nit:
> 
>   if (a &&
>       (b || c)) {
>   }

Fixed.

> @@ +609,5 @@
> > +      const nsDOMMemoryFile* memoryBlob = static_cast<const nsDOMMemoryFile*>(blob);
> > +
> > +      InfallibleTArray<uint8_t> data;
> > +      data.AppendElements(reinterpret_cast<uint8_t*>(memoryBlob->GetData()),
> > +                          memoryBlob->GetLength());
> 
> We should cheat here and use memcpy rather than AppendElements (nsTArray
> isn't smart enough to do that yet).

Fixed.

> @@ +614,5 @@
> > +
> > +      MemoryBlobOrFileConstructorParams memoryParams(params, data);
> > +      resultParams = memoryParams;
> > +    }
> > +    else if (const nsTArray<nsCOMPtr<nsIDOMBlob> >* subBlobs = blob->GetSubBlobs()) {
> 
> Nit: you don't use subBlobs here, just do |else if (blob->GetSubBlobs())|

Fixed.

> @@ +659,5 @@
> >  
> > +  actor->SetManager(this);
> > +
> > +  if (const nsTArray<nsCOMPtr<nsIDOMBlob> >* subBlobs =
> > +        static_cast<nsDOMFileBase*>(aBlob)->GetSubBlobs()) {
> 
> I'd prefer we only do this cast in one place. Can you change
> GetParamsForBlob() to take a nsDOMFileBase* and do this before calling?

Done.

> @@ +667,5 @@
> > +        return nullptr;
> > +      }
> > +
> > +      BlobChild* subActor = BlobChild::Create(aBlob);
> > +      if (!actor->SendPBlobConstructor(subActor, subParams)) {
> 
> In the child process this is infallible, so no need to check. (Bonus point
> possibility: fix the other SendPBlobConstructor above!)

Done.

> ::: dom/ipc/ContentChild.h
> @@ +196,5 @@
> >      uint64_t GetID() { return mID; }
> >  
> > +    bool GetParamsForBlob(nsIDOMBlob* aBlob, BlobConstructorParams* aOutParams);
> > +    BlobChild* GetOrCreateActorForBlob(nsIDOMBlob* aBlob,
> > +                                       BlobConstructorParams* aParams = nullptr);
> 
> Is this aParams ever non-null? I don't see where you use it.

Artifact of iterating. Fixed.

> ::: dom/ipc/ContentParent.cpp
> @@ +1347,5 @@
> > +{
> > +  // XXX This is only safe so long as all blob implementations in our tree
> > +  //     inherit nsDOMFileBase. If that ever changes then this will need to grow
> > +  //     a real interface or something.
> > +  const nsDOMFileBase* blob = static_cast<nsDOMFileBase*>(aBlob);
> 
> Let's only do this once like in ContentChild.

Fixed.

> @@ +1352,5 @@
> > +
> > +  BlobConstructorParams resultParams;
> > +
> > +  if (!blob->IsMemoryBacked() && (blob->IsSizeUnknown() ||
> > +                                  blob->IsDateUnknown())) {
> 
> Same nit here from ContentChild.

Fixed.

> @@ +1398,5 @@
> > +      const nsDOMMemoryFile* memoryBlob = static_cast<const nsDOMMemoryFile*>(blob);
> > +
> > +      InfallibleTArray<uint8_t> data;
> > +      data.AppendElements(reinterpret_cast<uint8_t*>(memoryBlob->GetData()),
> > +                          memoryBlob->GetLength());
> 
> memcpy again

Fixed.

> @@ +1403,5 @@
> > +
> > +      MemoryBlobOrFileConstructorParams memoryParams(params, data);
> > +      resultParams = memoryParams;
> > +    }
> > +    else if (const nsTArray<nsCOMPtr<nsIDOMBlob> >* subBlobs = blob->GetSubBlobs()) {
> 
> Same nit here from ContentChild.

Fixed.

> ::: dom/ipc/ContentParent.h
> @@ +126,5 @@
> >      }
> >  
> > +    bool GetParamsForBlob(nsIDOMBlob* aBlob, BlobConstructorParams* aOutParams);
> > +    BlobParent* GetOrCreateActorForBlob(nsIDOMBlob* aBlob,
> > +                                        BlobConstructorParams* aParams = nullptr);
> 
> Same, why have an outparam here?

Fixed.

> ::: dom/ipc/DOMTypes.ipdlh
> @@ +30,5 @@
> >    uint64_t length;
> >    uint64_t modDate;
> >  };
> >  
> > +union BlobOrFileConstructorParams {
> 
> Nit: { on its own line

Fixed.

> @@ +41,5 @@
> > +  BlobOrFileConstructorParams constructorParams;
> > +  uint8_t[] data;
> > +};
> > +
> > +
> 
> Nit: extra newline

Fixed.

> ::: dom/ipc/PBlob.ipdl
> @@ +29,5 @@
> >    PBlobStream();
> >  
> >    ResolveMystery(ResolveMysteryParams params);
> > +
> > +  AddSubBlob(PBlob subBlob);
> 
> This message is unnecessary, you can move the sub blob logic to the PBlob
> constructor.

I don't understand what this means.

> ::: dom/ipc/nsIRemoteBlob.h
> @@ +21,5 @@
> >  
> >    // This will either return a PBlobChild or PBlobParent.
> >    virtual void* GetPBlob() = 0;
> > +
> > +  virtual void SetActor(void* aActor) = 0;
> 
> Nit: SetPBlob would be better.

Done.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 20

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

Comment on attachment 700226 [details] [diff] [review]
Patch

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

::: dom/ipc/Blob.cpp
@@ +1125,5 @@
> +          const NormalBlobConstructorParams& normalParams =
> +            internalParams.get_NormalBlobConstructorParams();
> +          MOZ_ASSERT(normalParams.length() == params.data().Length());
> +
> +          void* data = moz_xmalloc(params.data().Length());

I think you missed this comment from last time but xmalloc shouldn't be used on the parent side. You can add static functions to the traits classes to work around this.

@@ +1263,5 @@
>    if (mRemoteBlob && mOwnsBlob) {
>      blob = dont_AddRef(mBlob);
>      mOwnsBlob = false;
>    }
> +  else if (mRemoteMemoryBlob && mOwnsBlob) {

This still looks wrong. I think you want:

  if (mOwnsBlob && (mRemoteBlob || mRemoteMemoryBlob || mRemoteMultipartBlob))

@@ +1623,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!ProtocolType::RecvPBlobConstructor(aActor, aParams)) {
> +    return false;

Nit: No need to call the base class here, it's always an empty method.

::: dom/ipc/ContentParent.cpp
@@ +1420,5 @@
> +      const nsDOMMemoryFile* memoryBlob =
> +        static_cast<const nsDOMMemoryFile*>(aBlob);
> +
> +      InfallibleTArray<uint8_t> data;
> +      data.SetLength(memoryBlob->GetLength());

This should be fallible in the parent.

::: dom/ipc/nsIRemoteBlob.h
@@ +21,5 @@
>  
>    // This will either return a PBlobChild or PBlobParent.
>    virtual void* GetPBlob() = 0;
> +
> +  virtual void SetPBlob(void* aActor) = 0;

I don't see why we need this... As far as I can tell we only ever call it on internal classes where we know the concrete type. Followup fodder if you want.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 22

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

Comment on attachment 700314 [details] [diff] [review]
Patch

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

::: dom/ipc/Blob.cpp
@@ +1128,5 @@
> +          const NormalBlobConstructorParams& normalParams =
> +            internalParams.get_NormalBlobConstructorParams();
> +          MOZ_ASSERT(normalParams.length() == params.data().Length());
> +
> +          void* data = moz_xmalloc(params.data().Length());

Missed this allocation.

@@ +1143,5 @@
> +            internalParams.get_FileBlobConstructorParams();
> +          MOZ_ASSERT(fbParams.length() == params.data().Length());
> +
> +          void* data =
> +            BlobTraits<ActorFlavor>::Allocate(params.data().Length());

Now that it's sometimes fallible you have to check for null.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1391e924e9ee

Comment 24

[Ed Morley [:emorley]]

Backed out for:
43768 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_ipc_messagemanager_blob.html | Test timed out.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1391e924e9ee

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed308815b4a7

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Stupid mistake, relanded

http://hg.mozilla.org/integration/mozilla-inbound/rev/2b3dd950e66f

Comment 26

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ae7cce14f365

Comment 27

[Johnny Stenback (:jst)]

Also, https://hg.mozilla.org/releases/mozilla-b2g18/rev/5e906ef58f46

Comment 28

[Alexandre Poirot [:ochameau]]

The last revision still breaks gecko-18 builds:
https://tbpl.mozilla.org/?tree=Mozilla-B2g18&noignore=1

https://tbpl.mozilla.org/php/getParsedLog.php?id=18688935&tree=Mozilla-B2g18

15:28:17    ERROR -  ../../../gecko/dom/ipc/ContentParent.cpp:1498: error: no matching function for call to 'mozilla::dom::MemoryBlobOrFileConstructorParams::MemoryBlobOrFileConstructorParams(mozilla::dom::BlobOrFileConstructorParams&, FallibleTArray<unsigned char>&)'

Comment 29

[Johnny Stenback (:jst)]

Backed out of b2g18 :(

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4f76130d77f5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/865c4ce63c17

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Relanded.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7300ed89eb14

Comment 31

[Andreas Gal :gal]

Landed. We are done here.

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b3dd950e66f
https://hg.mozilla.org/mozilla-central/rev/901864570d8e

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b3dd950e66f
https://hg.mozilla.org/mozilla-central/rev/901864570d8e