Closed
Bug 806503
Opened 12 years ago
Closed 11 years ago
[memory-backed blobs don't have a long enough lifetime for use in activities
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
People
(Reporter: djf, Assigned: khuey)
References
Details
Attachments
(1 file, 2 obsolete files)
55.93 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•12 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.
Updated•12 years ago
|
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•12 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•12 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•12 years ago
|
Assignee: nobody → bent.mozilla
blocking-basecamp: ? → +
Comment 7•12 years ago
|
||
Setting priority based on triage discussions. Feel free to decrease priority if you disagree.
Priority: -- → P1
Comment 8•12 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)
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Comment 12•12 years ago
|
||
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•12 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.
Assignee | ||
Comment 14•11 years ago
|
||
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.
...in theory...
Assignee: bent.mozilla → khuey
Assignee | ||
Comment 18•11 years ago
|
||
(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
Assignee | ||
Comment 19•11 years ago
|
||
Assignee: bent.mozilla → khuey
Status: NEW → ASSIGNED
Attachment #700226 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•11 years ago
|
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.
Assignee | ||
Comment 21•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1391e924e9ee
Comment 24•11 years ago
|
||
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
Assignee | ||
Comment 25•11 years ago
|
||
Stupid mistake, relanded http://hg.mozilla.org/integration/mozilla-inbound/rev/2b3dd950e66f
Comment 26•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ae7cce14f365
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Comment 27•11 years ago
|
||
Also, https://hg.mozilla.org/releases/mozilla-b2g18/rev/5e906ef58f46
Comment 28•11 years ago
|
||
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•11 years ago
|
||
Backed out of b2g18 :( https://hg.mozilla.org/releases/mozilla-b2g18/rev/4f76130d77f5 https://hg.mozilla.org/releases/mozilla-b2g18/rev/865c4ce63c17
Assignee | ||
Comment 30•11 years ago
|
||
Relanded. https://hg.mozilla.org/releases/mozilla-b2g18/rev/7300ed89eb14
status-b2g18:
--- → fixed
status-firefox21:
--- → fixed
Comment 31•11 years ago
|
||
Landed. We are done here.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 32•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2b3dd950e66f https://hg.mozilla.org/mozilla-central/rev/901864570d8e
Comment 33•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2b3dd950e66f https://hg.mozilla.org/mozilla-central/rev/901864570d8e
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•