If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 21

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: djf, Assigned: khuey)

Tracking

Trunk
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
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.
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
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?

Updated

5 years ago
blocking-basecamp: --- → ?
Bent: Need your input on if this is blocking.
Flags: needinfo?(bent.mozilla)
Bent: Need your input on if this is blocking.
(Reporter)

Comment 5

5 years ago
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.)
I think this should block based on our discussion earlier.
Flags: needinfo?(bent.mozilla)

Updated

5 years ago
Assignee: nobody → bent.mozilla
blocking-basecamp: ? → +
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1

Comment 8

5 years ago
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.
Target Milestone: --- → B2G C3 (12dec-1jan)
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.
Andrea, can you take this with Ben reviewing?
Assignee: bent.mozilla → amarchesini
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.
Assignee: amarchesini → khuey

Updated

5 years ago
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
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.
(Reporter)

Comment 13

5 years ago
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.
Created attachment 699608 [details] [diff] [review]
Patch
Attachment #699608 - Flags: review?(bent.mozilla)
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.
Ben says he has a better approach.
Assignee: khuey → bent.mozilla
...in theory...
Assignee: bent.mozilla → khuey
(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.
Assignee: khuey → bent.mozilla
Created attachment 700226 [details] [diff] [review]
Patch
Assignee: bent.mozilla → khuey
Status: NEW → ASSIGNED
Attachment #700226 - Flags: review?(bent.mozilla)
Attachment #699608 - Attachment is obsolete: true
Attachment #699608 - Flags: review?(bent.mozilla)
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.
Created attachment 700314 [details] [diff] [review]
Patch
Attachment #700226 - Attachment is obsolete: true
Attachment #700226 - Flags: review?(bent.mozilla)
Attachment #700314 - Flags: review?(bent.mozilla)
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.
Attachment #700314 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1391e924e9ee
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
Stupid mistake, relanded

http://hg.mozilla.org/integration/mozilla-inbound/rev/2b3dd950e66f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ae7cce14f365
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
Also, https://hg.mozilla.org/releases/mozilla-b2g18/rev/5e906ef58f46
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>&)'
Backed out of b2g18 :(

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4f76130d77f5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/865c4ce63c17
Status: RESOLVED → REOPENED
status-b2g18: fixed → ---
Resolution: FIXED → ---
Relanded.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7300ed89eb14
status-b2g18: --- → fixed
status-firefox21: --- → fixed

Comment 31

5 years ago
Landed. We are done here.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/2b3dd950e66f
https://hg.mozilla.org/mozilla-central/rev/901864570d8e
https://hg.mozilla.org/mozilla-central/rev/2b3dd950e66f
https://hg.mozilla.org/mozilla-central/rev/901864570d8e
Blocks: 982779
You need to log in before you can comment on or make changes to this bug.