1359087 - Use IPCBlob in FileHandle

Status: RESOLVED FIXED

(Core :: DOM: File, enhancement)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - PPendingIndexedDBBlob protocol

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - FileHandle and IPCBlob

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - Changes in IndexedDB StreamWrapper

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - FileHelper::SyncCopy must work with asyncInputStream

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8861815 [details] [diff] [review]
part 4 - FileHelper::SyncCopy must work with asyncInputStream

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

I think this change nicely moots the DatabaseFile::GetBlockingInputStream changes made in bug 1319531.  Specifically, I think GetBlockingInputStream can be renamed GetInputStream again and lose most of its comment and pretty much everything but the call to GetInternalStream because there is no longer any need to wrap things in a blocking pipe.  I would suggest making that change in this bug rather than in a follow-up because I expect the existing logic is causing this new logic not to be used, at least in the cases where we've seen intermittent failures because of failure to AsyncWait previously.

(When I had chosen to go the pipe wrapping way over something like this I was worried about duplicating the logic nsPipe already provided, but this is quite small and clean and is definitely an efficiency win!  I do feel a little redeemed over my concerns about duplicating already proven threading logic though... ;)

::: dom/indexedDB/ActorsParent.cpp
@@ +29791,5 @@
> +  if (!asyncStream) {
> +    return rv;
> +  }
> +
> +  RefPtr<ReadCallback> callback = new ReadCallback();

FileHelper is a non-static MOZ_STACK_CLASS (which is therefore only ever used on a single thread).  It seems like you could cache the callback as a member once you create it to save a bunch of instance churn.

@@ +29804,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  callback->Wait();

I don't think this is sound.  Based on my understanding of condition variables and the underlying pthread_cond_wait/pthread_cond_signal primitives used, signaling only wakes up current waiters and has no effect on future waiters.  ReadCallback::OnInputStreamReady could be invoked before ReadCallback::Wait() is reached leading to a hang as the wait waits for another notify that will never come.  I think a change like one of the following is required:
- The mutex needs to be acquired prior to calling AsyncWait() and ReadCallback::Notify() needs to acquire the mutex prior to invoking mCondVar::Notify().  This guarantees that the notify call cannot happen prior to mCondvar.Wait() being invoked, at which point the mutex will be released and the notify can happen.  I think this technically might violate rules about not holding a mutex while making calls out of the module, but it seems safe enough.
- A state variable like mInputAvailable needs to be added to ReadCallback.  OnInputStreamReady sets it (while holding a mutex so we can assume the proper memory barriers happen), ReadCallback::Wait() checks it in a loop with the mutex held like `while (!mInputAvailable) mCondVar.Wait();`.  The loop probably isn't strictly required, but the spec for pthread_cond_wait() says that spurious wakeups can happen, in which case the loop is appropriate.  (Having said that, I think sprurious wakeups are likely to only be a problem with multiple waiters under certain implementations taking advantage of the leeway given to them by POSIX).

Race-wise, there's nothing stopping ReadCallback::OnInputStreamReady from having run before AsyncWait() returns control flow to the calling code here.  In particular, nsPipeInputStream's AsyncWait can end up dispatching the event as its nsPipeEvents destructor is called on returning, or slightly before that when its mPipe->ReentrantMonitor returns.

Note: Obviously if you do reuse the ReadCallback instances and use a flag, then the flag needs to be cleared by a successful Wait().

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - FileHelper::SyncCopy must work with asyncInputStream

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8862011 [details] [diff] [review]
part 4 - FileHelper::SyncCopy must work with asyncInputStream

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

Awesome cleanup work here and on the whole IPCBlob patch series.  Especially happy about part 1 of this bug meaning that mystery blobs will soon be a thing of the past!  (They were very confusing :)

::: dom/indexedDB/ActorsParent.cpp
@@ +29679,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    mInputAvailable = false;

nit, not required: Because of the mutex it's safe to have the `mInputAvailable = false` here, but I'd argue it would be more intuitive to read if it happened just prior to the AsyncWait.  As it stands, I end up backtracking up to the mutex and then back down to the wait.  Again, not a big deal.

@@ +29730,5 @@
> +  }
> +
> +  // We just need any thread with an event loop for receiving the
> +  // OnInputStreamReady callback. Let's use the I/O thread.
> +  nsCOMPtr<nsIEventTarget> target =

Potential optimization, not required: Since the ReadCallback is cached and it bakes in some assumptions about threads, it could make sense to just fold the target into ReadCallback as mTarget and avoid getting the stream transport service every time.

Comment 8

[Jan Varga [:janv]]

Comment on attachment 8860995 [details] [diff] [review]
part 1 - PPendingIndexedDBBlob protocol

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

Well FileHandle is designed to be used by other storage APIs, not just IndexedDB.
So IndexedDB in PPendingIndexedDBBlob should be changed to something else from this point of view.
I wouldn't mind if this stuff lived in dom/indexedDB, but we want to use it also in dom/filehandle.
Anyway, it's not a big deal, not worth the additional work at this moment.

::: dom/file/ipc/PendingIndexedDBBlobChild.cpp
@@ +19,5 @@
> +{}
> +
> +already_AddRefed<BlobImpl>
> +PendingIndexedDBBlobChild::SetPendingInfoAndDeleteActor(const nsString& aName,
> +                                                        const nsString& aContentType,

Nit: over 80 chars

@@ +36,5 @@
> +  return blobImpl.forget();
> +}
> +
> +already_AddRefed<BlobImpl>
> +PendingIndexedDBBlobChild::SetPendingInfoAndDeleteActor(const nsString& aContentType,

Nit: over 80 chars

::: dom/file/ipc/PendingIndexedDBBlobChild.h
@@ +13,5 @@
> +namespace dom {
> +
> +class BlobImpl;
> +
> +class PendingIndexedDBBlobChild final : public mozilla::ipc::PPendingIndexedDBBlobChild

Nit: over 80 chars

@@ +16,5 @@
> +
> +class PendingIndexedDBBlobChild final : public mozilla::ipc::PPendingIndexedDBBlobChild
> +{
> +public:
> +  PendingIndexedDBBlobChild(const IPCBlob& aBlob);

|explicit|, but it seems you already fixed it for try

::: dom/file/ipc/PendingIndexedDBBlobParent.cpp
@@ +39,5 @@
> +  : mBlobImpl(aBlobImpl)
> +{}
> +
> +IPCResult
> +PendingIndexedDBBlobParent::Recv__delete__(const PendingIndexedDBBlobData& aData)

Nit: over 80 chars

::: dom/file/ipc/PendingIndexedDBBlobParent.h
@@ +18,5 @@
> +namespace dom {
> +
> +class BlobImpl;
> +
> +class PendingIndexedDBBlobParent final : public mozilla::ipc::PPendingIndexedDBBlobParent

Nit: over 80 chars

@@ +25,5 @@
> +  static PendingIndexedDBBlobParent*
> +  Create(PBackgroundParent* aManager, BlobImpl* aBlobImpl);
> +
> +  virtual void
> +  ActorDestroy(ActorDestroyReason aWhy) override

|virtual| not needed when there's |override|
here and below

Btw, why do you need to override this with empty {} ?

@@ +32,5 @@
> +  virtual mozilla::ipc::IPCResult
> +  Recv__delete__(const PendingIndexedDBBlobData& aData) override;
> +
> +private:
> +  PendingIndexedDBBlobParent(BlobImpl* aBlobImpl);

|explicit|, but it seems you already fixed it for try

::: ipc/glue/BackgroundChildImpl.cpp
@@ +237,5 @@
> +  return new mozilla::dom::PendingIndexedDBBlobChild(aBlob);
> +}
> +
> +bool
> +BackgroundChildImpl::DeallocPPendingIndexedDBBlobChild(PPendingIndexedDBBlobChild* aActor)

Nit: over 80 chars

::: ipc/glue/BackgroundChildImpl.h
@@ +85,5 @@
> +  virtual PPendingIndexedDBBlobChild*
> +  AllocPPendingIndexedDBBlobChild(const IPCBlob& aBlob) override;
> +
> +  virtual bool
> +  DeallocPPendingIndexedDBBlobChild(PPendingIndexedDBBlobChild* aActor) override;

Nit: over 80 chars

::: ipc/glue/BackgroundParentImpl.cpp
@@ +295,5 @@
> +  MOZ_CRASH("PPendingIndexedDBBlobParent actors should be manually constructed!");
> +}
> +
> +bool
> +BackgroundParentImpl::DeallocPPendingIndexedDBBlobParent(PPendingIndexedDBBlobParent* aActor)

Nit: over 80 chars

::: ipc/glue/BackgroundParentImpl.h
@@ +78,5 @@
> +  virtual PPendingIndexedDBBlobParent*
> +  AllocPPendingIndexedDBBlobParent(const IPCBlob& aBlob) override;
> +
> +  virtual bool
> +  DeallocPPendingIndexedDBBlobParent(PPendingIndexedDBBlobParent* aActor) override;

Nit: Over 80 chars

Comment 9

[Andrea Marchesini [:baku]]

I renamed it PendingIPCBlob.

Comment 10

[Jan Varga [:janv]]

Ok, other reviews coming really soon.

Comment 11

[Jan Varga [:janv]]

Comment on attachment 8860996 [details] [diff] [review]
part 2 - FileHandle and IPCBlob

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

::: dom/filehandle/ActorsChild.cpp
@@ +316,5 @@
>    const FileRequestLastModified& lastModified = metadata.lastModified();
>    MOZ_ASSERT(lastModified.type() == FileRequestLastModified::Tint64_t);
>  
> +  RefPtr<BlobImpl> blobImpl =
> +    actor->SetPendingInfoAndDeleteActor(mutableFile->Name(),

The name of this method is not bad. The method sets the pending info, finishes the IPC protocol and returns blob impl by moving the reference from internal member before the object is destroyed. It doesn't make sense to put all these actions into the method. I think the most important part is that the IPC communication will be closed. So something like actor->FinishProtocol/Actor() would be a bit better.
Feel free to keep SetPendingInfoAndDeleteActor().

Comment 12

[Jan Varga [:janv]]

Comment on attachment 8860997 [details] [diff] [review]
part 3 - Changes in IndexedDB StreamWrapper

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

::: dom/indexedDB/FileSnapshot.cpp
@@ +253,5 @@
> +                                     IsIPCSerializableInputStream())
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIAsyncInputStream,
> +                                     IsAsyncInputStream())
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIInputStreamCallback,
> +                                     IsAsyncInputStream())

Should these entries follow the declaration order too ?

@@ -217,5 @@
>  
>  NS_IMETHODIMP
>  StreamWrapper::Close()
>  {
> -  MOZ_ASSERT(!IsOnOwningThread());

So now it's ok, to call these I/O methods on the owning thread ?

@@ +277,1 @@
>    // the content length property).

If you are removing all the owning thread asserts, then you need to remove this comment here too, in Available()

@@ +330,5 @@
>    return Nothing();
>  }
>  
> +NS_IMETHODIMP
> +StreamWrapper::GetCloneable(bool* aCloneable)

Nit: GetCloneable() and Clone() should be placed after OnInputStreamReady() to match the declaration order.

@@ +333,5 @@
> +NS_IMETHODIMP
> +StreamWrapper::GetCloneable(bool* aCloneable)
> +{
> +  nsCOMPtr<nsICloneableInputStream> stream = do_QueryInterface(mInputStream);
> +  if (stream) {

Hm, this method is only exposed if the stream is cloneable, right ?
So either MOZ_ASSERT(stream) or do:
if (!stream) {
  return NS_NO_INTERFACE;
}

Other methods should follow the same pattern too I think.

Comment 13

[Andrea Marchesini [:baku]]

> So now it's ok, to call these I/O methods on the owning thread ?

Well, this is what FileReaderSync does, right?

Comment 14

[Jan Varga [:janv]]

(In reply to Andrea Marchesini [:baku] from comment #13)
> > So now it's ok, to call these I/O methods on the owning thread ?
> 
> Well, this is what FileReaderSync does, right?

GetInternalStream() is called first, usually on the main thread (FileHandle is not supported on workers) and then the stream is read on a background I/O thread. That's why I put those asserts there, to catch possible main thread I/O.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9294be2d9f63
Use IPCBlob in FileHandle - part 1 - PPendingIPCBlob protocol, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fafc2b84cf
Use IPCBlob in FileHandle - part 2 - FileHandle and IPCBlob, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a5a1808b5f
Use IPCBlob in FileHandle - part 3 - Changes in IndexedDB StreamWrapper, r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/c41c78005666
Use IPCBlob in FileHandle - part 4 - FileHelper::SyncCopy must work with asyncInputStream, r=asuth

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9294be2d9f63
https://hg.mozilla.org/mozilla-central/rev/66fafc2b84cf
https://hg.mozilla.org/mozilla-central/rev/f4a5a1808b5f
https://hg.mozilla.org/mozilla-central/rev/c41c78005666