1353629 - [meta] PBlob refactoring

Status: RESOLVED FIXED

(Core :: DOM: File, enhancement, P3)

Description

[Andrea Marchesini [:baku]]

The main goal of this meta bug is:

1. to remove the blocking-thread method for the creation of remoteInputStream (see bug  1350644)
2. rewrite PBlob on top of PChildToParentStream and PParentToChildStream
3. improve the use of nsIInputStream for PBlob and gecko in general when sent via IPC.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 0 - IPCBlob

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - AutoIPCStream should not have a copy CTOR

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - IPCStream must be available on PBackground

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 3 - IPCBlob in ClonedMessageData

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 4 - IPCBlobInputStream

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 5 - IPCBlobInputStreamStorage

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 6 - IPCBlobInputStream serialization

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 7 - IPCBlobInputStream must implement nsIAsyncInputStream

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 8 - FileReader should use nsIAsyncInputStream if available

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 9 - PBlob should use IPCStream in case it is dealing with an IPCBlobInputStream

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 10 - nsFileStream should be reusable after Serialize()

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 6 - IPCBlobInputStream serialization

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 10 - Make nsFileInputStream cloneable

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I don't like patch/commit messages to be in form "I'll describe something later". Those messages really should be about the actual patch.

But ok, thanks for comments anyhow :)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857488 [details] [diff] [review]
part 0 - IPCBlob

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  f40e24f40b4c4556944c762d4764eace261297f5
>Bug 1353629 - PBlob refactoring - part 0 - IPCBlob, r?smaug
>
>This first patch introduces the basic idea of the refactoring. Instead of
>having a PBlob protocol, here we have a simple IPC struct: IPCBlob.
>When a Blob is sent to a different process, we move an IPCBlob struct with
>type, size, some specificy File properties, and an inputStream as IPCStream.
specificy?

>
>The key point of this refactoring is how we manage inputStreams across
>processes: the inputStream is immediately sent but it is managed in a
>completely different way compared with the previous PBlob implementation.
>I'll describe it more in the following patches.
This is odd kind of comment for a patch.

>+++ b/dom/file/ipc/IPCBlobHelper.h
...
>+nsresult
>+Serialize(BlobImpl* aBlobImpl, IPCBlob& aIPCBlob,
>+          nsIContentChild* aManager, AutoIPCStream& aIPCStream);
>+
>+nsresult
>+Serialize(BlobImpl* aBlobImpl, IPCBlob& aIPCBlob,
>+          PBackgroundChild* aManager, AutoIPCStream& aIPCStream);
>+
>+nsresult
>+Serialize(BlobImpl* aBlobImpl, IPCBlob& aIPCBlob,
>+          nsIContentParent* aManager, AutoIPCStream& aIPCStream);
>+
>+nsresult
>+Serialize(BlobImpl* aBlobImpl, IPCBlob& aIPCBlob,
>+          PBackgroundParent* aManager, AutoIPCStream& aIPCStream);
>+
It is unclear from this code whether aIPCBlob is in or out param, same with aIPCStream.
Apparently at least aIPCBlob is out param, so it should be after in params.
I guess aIPCStream isn't out param, but it isn't clear from this patch how it is supposed to be used.
This could use some comments in code and out params moved to be last.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857491 [details] [diff] [review]
part 2 - IPCStream must be available on PBackground


>-  // A source can be used on any thread, but we only support IPCStream on
>-  // main thread and Worker threads right now.  This is due to the requirement
>+  // A source can be used on any thread. This is due to the requirement
>   // that the thread be guaranteed to live long enough to receive messages.
>   // We can enforce this guarantee with a feature on worker threads, but not
>   // other threads.
This comment makes no sense anymore. What enforces the correct lifetime management on non-worker threads?
The comment needs to explain how this is works with PBackground.
(since I have no idea how this works. Per the comment this doesn't work)

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857492 [details] [diff] [review]
part 3 - IPCBlob in ClonedMessageData

>-BroadcastChannelParent::Deliver(const ClonedMessageData& aData)
>+BroadcastChannelParent::Deliver(const ClonedMessageData& aData,
>+                                const nsTArray<RefPtr<BlobImpl>>& aBlobImpls)
I don't understand why ClonedMessageData doesn't anymore contain blobs.

>+  if (NS_WARN_IF(!ipcStreams.SetLength(aBlobImpls.Length(), fallible))) {
>+    return;
>+  }
>+
>+  if (!aBlobImpls.IsEmpty()) {
>+    // Serialize Blob objects for this message.
>+    for (uint32_t i = 0, len = aBlobImpls.Length(); i < len; ++i) {
>+      nsresult rv =
>+        IPCBlobHelper::Serialize(aBlobImpls[i], newData.blobs()[i], Manager(),
>+                                 ipcStreams[i]);
aha, here we construct parts of the ClonedMessageData, since the second param to Serialize is out param, I think.
This setup feels a bit odd. Should ClonedMessageData be constructed properly before calling this method.
Or at least the method name should somehow hint that it is still constructing the sent ClonedMessageData - 
but might be more understandable to split the method to two pieces. One just delivers and the other constructs ClonedMessageData

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857493 [details] [diff] [review]
part 4 - IPCBlobInputStream


>+  // Raw pointers because these streams keep this actor alive. When the last
>+  // stream is unregister, the actor will be deleted. This list is protected by
>+  // mutex.
>+  nsTArray<IPCBlobInputStream*> mStreams;
>+
>+  const nsID mID;
>+  const uint64_t mSize;
>+
>+  Mutex mMutex;
What is this mutex for?
Oddly mutex is not next to mStreams which is using it.

>+
>+  // false when ActorDestroy() is called.
>+  bool mActorAlive;
>+
>+  nsCOMPtr<nsIThread> mOwningThread;
>+};
>+/* static */ IPCBlobInputStreamParent*
>+IPCBlobInputStreamParent::Create(nsIInputStream* aInputStream, nsresult* aRv)
>+{
>+  MOZ_ASSERT(aInputStream);
>+  MOZ_ASSERT(aRv);
>+
>+  nsID id;
>+  *aRv = nsContentUtils::GenerateUUIDInPlace(id);
>+  if (NS_WARN_IF(NS_FAILED(*aRv))) {
>+    return nullptr;
>+  }
>+
>+  // We cannot fail because of the stream has a invalid size. This happens
>+  // normally for nsFileStreams pointing to non-existing files.
>+  uint64_t size;
>+  *aRv = aInputStream->Available(&size);
>+  if (NS_WARN_IF(NS_FAILED(*aRv))) {
>+    size = 0;
>+  }
>+
>+  // TODO: register to a service.
>+
>+  return new IPCBlobInputStreamParent(id, size);
>+}
>+
>+IPCBlobInputStreamParent::IPCBlobInputStreamParent(const nsID& aID,
>+                                                   uint64_t aSize)
>+  : mID(aID)
>+  , mSize(aSize)
>+{}
>+
>+void
>+IPCBlobInputStreamParent::ActorDestroy(IProtocol::ActorDestroyReason aReason)
>+{
>+  // TODO: unregister
Ok, need to remember to check that these TODOs are fixed in some other patch.

>+PIPCBlobInputStreamChild*
>+BackgroundChildImpl::AllocPIPCBlobInputStreamChild(const nsID& aID,
>+                                                   const uint64_t& aSize)
>+{
>+  RefPtr<mozilla::dom::IPCBlobInputStreamChild> actor =
>+    new mozilla::dom::IPCBlobInputStreamChild(aID, aSize);
>+  return actor.forget().take();
This needs a comment about where the refcnt is decreased.
Same with nsIContentChild::AllocPIPCBlobInputStreamChild



In general, this bug needs a good comment of the overall setup. Reading commit messages from several patches makes it rather hard to follow everything.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857496 [details] [diff] [review]
part 7 - IPCBlobInputStream must implement nsIAsyncInputStream

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  e807db006497648eafe3006e6353ae6804942bfe
>Bug 1353629 - PBlob refactoring - part 7 - IPCBlobInputStream must implement nsIAsyncInputStream, r?smaug
>
>There is only 1 way to use an IPCBlobInputStream: nsIAsyncInputStream.
Weird comment. I don't even know how to comment, since I don't quite understand what this means.
>+IPCBlobInputStream::AsyncWait(nsIInputStreamCallback* aCallback,
>+                              uint32_t aFlags, uint32_t aRequestedCount,
>+                              nsIEventTarget* aEventTarget)
>+{
>+  switch (mState) {
>+  case eInit:
>+    MOZ_ASSERT(mActor);
>+
>+    mCallback = aCallback;
>+    mCallbackEventTarget = aEventTarget;
>+    mState = ePending;
>+
>+    mActor->StreamNeeded(this);
>+    return NS_OK;
>+
>+  case ePending:
>+    if (mCallback && aCallback) {
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    mCallback = aCallback;
>+    mCallbackEventTarget = aEventTarget;
>+    return NS_OK;
>+
>+  case eRunning:
>+    if (aCallback) {
>+      CallbackRunnable::Execute(aCallback, aEventTarget, this);
>+    }
>+    return NS_OK;
>+
>+  default:
>+    MOZ_ASSERT(mState == eClosed);
>+    return NS_BASE_STREAM_CLOSED;
>+  }
>+}
This method needs good comments. Like difference between eInit and ePending

>   RefPtr<IPCBlobInputStreamChild> mActor;
>+
>+  enum {
>+    eInit,
>+    ePending,
>+    eRunning,
>+    eClosed,
>+  } mState;
These states need good comments

>+
>+  // Only 1 of these 2 is set.
>+  nsIContentParent* mContentManager;
>+  mozilla::ipc::PBackgroundParent* mPBackgroundManager;
This needs a comment why it is guaranteed that these raw member variables don't ever point to deleted objects.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857497 [details] [diff] [review]
part 8 - FileReader should use nsIAsyncInputStream if available

>@@ -777,16 +794,18 @@ FileReader::Shutdown()
> {
>   mReadyState = DONE;
> 
>   if (mAsyncStream) {
>     mAsyncStream->Close();
>     mAsyncStream = nullptr;
>   }
> 
>+  mBufferedStream = nullptr;
>+
Why we don't need to call Close() on mBufferedStream?
I guess stream's destructor does it anyhow, but what if something else is keeping a ref to the stream.
Is there a reason to not close?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857498 [details] [diff] [review]
part 9 - PBlob should use IPCStream in case it is dealing with an IPCBlobInputStream


>+union BlobDataStream
>+{
>+  MemoryBlobDataStream;
>+
>+  // InputStreamParams is used only when we are _sure_ that the serialized size
>+  // if lower than 1 Mb. Otherwise we use MemoryBlobDataStream.
s/if/is/

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857540 [details] [diff] [review]
part 6 - IPCBlobInputStream serialization


>This patch also implements the use if IPCBlobInputStream in IPCBlob.
What does this comment mean?

>+union IPCBlobStream
>+{
>+  // Parent to Child: The child will receive just a mock inputStream. Nothing
>+  // can be done with it except retrieving the size.
"mock" doesn't describe the stream very well.



>+SerializeInternalInputStreamParent(nsIInputStream* aInputStream,
>+                                   IPCBlob& aIPCBlob, M* aManager,
>+                                   AutoIPCStream& aIPCStream)
>+{
>+  // Parent to Child we always send a IPCBlobInputStream.
>+  MOZ_ASSERT(XRE_IsParentProcess());
>+
>+  nsresult rv;
>+  IPCBlobInputStreamParent* mockActor =
>+    IPCBlobInputStreamParent::Create(aInputStream, &rv);
Mock? This isn't any mock code, like for testing or anything.




>+template<typename M>
>+nsresult
>+SerializeInternalInputStreamChild(nsIInputStream* aInputStream,
>+                                   IPCBlob& aIPCBlob, M* aManager,
>+                                   AutoIPCStream& aIPCStream)
>+{
>+  if (!aIPCStream.Serialize(aInputStream, aManager)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  aIPCBlob.inputStream() = aIPCStream.TakeValue();
>+  return NS_OK;
>+}
>+
>+nsresult
>+SerializeInternalInputStream(nsIInputStream* aInputStream, IPCBlob& aIPCBlob,
>+                             nsIContentParent* aManager,
>+                             AutoIPCStream& aIPCStream)
>+{
>+  return SerializeInternalInputStreamParent(aInputStream, aIPCBlob, aManager,
>+                                            aIPCStream);
>+}
>+
>+nsresult
>+SerializeInternalInputStream(nsIInputStream* aInputStream, IPCBlob& aIPCBlob,
>+                             PBackgroundParent* aManager,
>+                             AutoIPCStream& aIPCStream)
>+{
>+  return SerializeInternalInputStreamParent(aInputStream, aIPCBlob, aManager,
>+                                            aIPCStream);
>+}
>+
>+nsresult
>+SerializeInternalInputStream(nsIInputStream* aInputStream, IPCBlob& aIPCBlob,
>+                             nsIContentChild* aManager,
>+                             AutoIPCStream& aIPCStream)
>+{
>+  return SerializeInternalInputStreamChild(aInputStream, aIPCBlob, aManager,
>+                                           aIPCStream);
>+}
>+
>+nsresult
>+SerializeInternalInputStream(nsIInputStream* aInputStream, IPCBlob& aIPCBlob,
>+                             PBackgroundChild* aManager,
>+                             AutoIPCStream& aIPCStream)
>+{
>+  return SerializeInternalInputStreamChild(aInputStream, aIPCBlob, aManager,
>+                                           aIPCStream);
>+}
>+
I don't understand the naming here. Are we serializing some internal input stream, or are these serialize methods internal?

>+void
>+IPCBlobInputStreamStorage::GetStream(const nsID& aID,
>+                                     nsIInputStream** aInputStream)
>+{
>+  mozilla::StaticMutexAutoLock lock(gMutex);
>+  nsCOMPtr<nsIInputStream> stream = mStorage.Get(aID);
>+  if (!stream) {
>+    stream.forget(aInputStream);
>+    return;
>+  }
This doesn't make sense. Don't you want tjust *aInputStream = nullptr;

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 0 - IPCBlob

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

I really want to explanation here about the whole setup. Maybe even some ascii art which could be then put to source code. And the code needs better comments. Commit messages aren't that.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 2 - IPCStream must be available on PBackground

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 3 - IPCBlob in ClonedMessageData

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

I will re-review after I've seen a comment here explaining the whole setup.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: part 0 - IPCBlob

IPCBlobHelper -> IPCBlobUtils

Comment 29

[Andrea Marchesini [:baku]]

Attachment: last part - comments

Comment 30

[Andrea Marchesini [:baku]]

Attachment: part 3 - IPCBlob in ClonedMessageData

Comment 31

[Andrea Marchesini [:baku]]

Attachment: part 4 - IPCBlobInputStream

Comment 32

[Andrea Marchesini [:baku]]

Attachment: part 3 - IPCBlob in ClonedMessageData

Comment 33

[Andrea Marchesini [:baku]]

Attachment: part 5 - IPCBlobInputStreamStorage

Comment 34

[Andrea Marchesini [:baku]]

Attachment: part 6 - IPCBlobInputStream serialization

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 7 - IPCBlobInputStream must implement nsIAsyncInputStream

Comment 36

[Andrea Marchesini [:baku]]

Attachment: part 8 - FileReader should use nsIAsyncInputStream if available

Comment 37

[Andrea Marchesini [:baku]]

Attachment: part 9 - PBlob should use IPCStream in case it is dealing with an IPCBlobInputStream

Comment 38

[Andrea Marchesini [:baku]]

Blobs and IPC
~~~~~~~~~~~~~

Simplifying, DOM Blob objects are chunks of data with a content type and a
size. DOM Files are Blobs with a name. They are are used in many APIs and, of
course, they can be cloned and sent cross threads and cross processes.

If we see Blobs from a platform point of view, the main (and often, the only)
interesting part is how to retrieve data from it. This is done via
nsIInputStream and, except for a couple of important details, this stream is
used in the parent process.

For this reason, when we consider the serialization of a blob via IPC
messages, the biggest effort is put in how to manage the nsInputStream
correctly. To serialize, we use the IPCBlob data struct: basically, the blob
properties (size, type, name if it's a file) and the nsIInputStream.

Before talking about the nsIInputStream it's important to say that we have
different kinds of Blobs, based on the different kinds of sources. A non
exaustive list is:
- a memory buffer: MemoryBlobImpl
- a string: StringBlobImpl
- a real OS file: FileBlobImpl
- a temporary OS file: TemporaryBlobImpl
- a generic nsIInputStream: StreamBlobImpl
- an empty blob: EmptyBlobImpl
- more blobs combined together: MultipartBlobImpl
Each one of these implementations has a custom ::GetInternalStream method.
So, basically, each one has a different kind of nsIInputStream (nsFileStream,
nsIStringInputStream, SlicedInputStream, and so on).

Another important point to keep in mind is that a Blob can be created on the
content process (for example: |new Blob([123])|) or it can be created on the
parent process and sent to content (a FilePicker creates Blobs and it runs on
the parent process).

Child to Parent Blob Serialization
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

When a document creates a blob, this can be sent, for different reasons to
the parent process. For instance it can be sent as part of a FormData, or it
can be converted to a BlobURL and broadcasted to any other existing
processes.

When this happens, we use the IPCStream data struct for the serialization
of the nsIInputStream. This means that, if the stream is fully serializable
and its size is lower than 1Mb, we are able to recreate the stream completely
on the parent side. This happens, basically with any kind of child-to-parent
stream except for huge memory streams. In this case we end up using
PChildToParentStream. See more information in IPCStreamUtils.h.

In order to populate IPCStream correctly, we use AutoIPCStream as documented
in IPCStreamUtils.h. Note that we use the 'delayed start' feature because,
often, the stream doesn't need to be read on the parent side.

Parent to Child Blob Serialization
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This scenario is extremely common when we talk about Blobs pointing to real
files: HTMLInputElement (type=file), or Entries API, DataTransfer and so on.
But we also have this scenario when a content process creates a Blob and it
broadcasts it because of a BlobURL or because BroadcastChannel API is used.

The approach here is this: normally, the content process doesn't really read
data from the blob nsIInputStream. The content process needs to have the
nsIInputStream and be able to send it back to the parent process when the
"real" work needs to be done. This is true except for 2 usecases: FileReader
API and BlobURL usage. So, if we ignore these 2, normally, the parent sends a
blob nsIInputStream to a content process, and then, it will receive it back
in order to do some networking, or whatever.

For this reason, IPCBlobUtils uses a particular protocol for serializing
nsIInputStream parent to child: PIPCBlobInputStream. This protocol keeps the
original nsIInputStream alive on the parent side, and gives its size and a
UUID to the child side. The child side creates a IPCBlobInputStream and that
is incapsulated into a StreamBlobImpl.

The UUID is useful when the content process sends the same nsIInputStream
back to the parent process because, the only information it has to share is
the UUID. Each nsIInputStream sent via PIPCBlobInputStream, is registered
into the IPCBlobInputStreamStorage.

On the content process side, IPCBlobInputStream is a special inputStream:
the only reliable methods are:
- nsIInputStream.available() - the size is shared by PIPCBlobInputStream
  actor.
- nsIIPCSerializableInputStream.serialize() - we can give back this stream to
  the parent because we know its UUID.
- nsICloneableInputStream.cloneable() and nsICloneableInputStream.clone() -
  this stream can be cloned. We just need to have a reference of the
  PIPCBlobInputStream actor and its UUID.
- nsIAsyncInputStream.asyncWait() - see next section.

Any other method (read, readSegment and so on) will fail. Basically, this
inputStream cannot be used synchronously for any 'real' reading operation.

When the parent receives the serialization of a IPCBlobInputStream, it is
able to retrieve the correct nsIInputStream using the UUID and
IPCBlobInputStreamStorage.

Parent to Child Streams, FileReader and BlobURL
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The FileReader and BlobURL scenarios are described here.

When content process needs to read data from a Blob sent from the parent
process, it must do it asynchously using IPCBlobInputStream as a
nsIAsyncInputStream stream. This happens calling
IPCBlobInputStream.asyncWait(). At that point, the child actor will send a
StreamNeeded() IPC message to the parent side. When this is received, the
parent retrieves the 'real' stream from IPCBlobInputStreamStorage using the
UUID, it will serialize the 'real' stream, and it will send it to the chils
side.

When the 'real' stream is received (RecvStreamReady()), the asyncWait
callback will be executed and, from that moment, any IPCBlobInputStream
method will be forwarded to the 'real' stream ones. This means that the
reading will be available.

Comment 39

[Andrea Marchesini [:baku]]

Comment 38 describes the "big picture" of this refactoring.
It is a the first version of this description and it will be improved during the review process.
When the code lands, this comment will be found in dom/file/ipc/IPCBlobUtils.h

Comment 40

[Andrea Marchesini [:baku]]

Attachment: last part - comments

Comment 41

[Andrea Marchesini [:baku]]

Attachment: part 11 - nsInputStreamPump and MediaResource

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

We need to ensure different kinds of blobs are tested cross multiple child processes.
Sending them using broadcastchannel and/or (I guess 'and') blob urls etc.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859512 [details] [diff] [review]
last part - comments

>+ * Simplifying, DOM Blob objects are chunks of data with a content type and a
>+ * size. DOM Files are Blobs with a name. They are are used in many APIs and, of
>+ * course, they can be cloned and sent cross threads and cross processes.
Nit, ', of course,' sounds a bit weird when talking about some API. I would just drop it.



>+ * Parent to Child Blob Serialization
>+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>+ * 
>+ * This scenario is extremely common when we talk about Blobs pointing to real
drop "extremely". Doesn't really add anything.


>+ * When content process needs to read data from a Blob sent from the parent
>+ * process, it must do it asynchously using IPCBlobInputStream as a
asynchronously

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859587 [details] [diff] [review]
part 11 - nsInputStreamPump and MediaResource

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  01cb5510772084bad3e10638a16f042e3e7ec9bf
>Bug 1353629 - PBlob refactoring - part 11 - nsInputStreamPump should use BufferedStreams, r?smaug
>
>nsInputStreamPump should use the stream as nsIAsyncInputStream if available.
>In order to do so, we need to wrap a BufferedStream about it.
about? or around

> nsInputStreamPump::PeekStream(PeekSegmentFun callback, void* closure)
> {
>   ReentrantMonitorAutoEnter mon(mMonitor);
> 
>   NS_ASSERTION(mAsyncStream, "PeekStream called without stream");
> 
>+  nsresult rv = CreateBufferedStreamIfNeeded();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   // See if the pipe is closed by checking the return of Available.
>   uint64_t dummy64;
>-  nsresult rv = mAsyncStream->Available(&dummy64);
>+  rv = mBufferedStream->Available(&dummy64);
>   if (NS_FAILED(rv))
>     return rv;
>   uint32_t dummy = (uint32_t)std::min(dummy64, (uint64_t)UINT32_MAX);
> 
>   PeekData data(callback, closure);
>-  return mAsyncStream->ReadSegments(CallPeekFunc,
>-                                    &data,
>-                                    nsIOService::gDefaultSegmentSize,
>-                                    &dummy);
>+  return mBufferedStream->ReadSegments(CallPeekFunc,
>+                                       &data,
>+                                       nsIOService::gDefaultSegmentSize,
>+                                       &dummy);
Does this change cause us to allocate lots of more stream objects? Have you done any measurement how often a new stream is created and how much
more this might use memory? And how many more memcpy calls is involved. 
Please do some testing.


Almost r+, but  memory usage overhead and memcpy usage needs to be measured in common cases like starting browser.
Are there cases when this could regress performance?

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859492 [details] [diff] [review]
part 3 - IPCBlob in ClonedMessageData

>   for (uint32_t i = 0; i < parents->Length(); ++i) {
>     BroadcastChannelParent* parent = parents->ElementAt(i);
>     MOZ_ASSERT(parent);
> 
>-    if (parent != aParent) {
>-      parent->Deliver(aData);
>+    if (parent == aParent) {
>+      continue;
>     }
>+
>+    // We need to have a copy of the data for this parent.
>+    ClonedMessageData newData(aData);
>+    MOZ_ASSERT(blobImpls.Length() == newData.blobs().Length());
>+
>+    if (!blobImpls.IsEmpty()) {
>+      // Serialize Blob objects for this message.
>+      for (uint32_t i = 0, len = blobImpls.Length(); i < len; ++i) {
>+        nsresult rv = IPCBlobUtils::Serialize(blobImpls[i], parent->Manager(),
>+                                              newData.blobs()[i]);
>+        if (NS_WARN_IF(NS_FAILED(rv))) {
>+          return;
>+        }
>+      }
>+    }
>+
>+    Unused << parent->SendNotify(newData);
>   }
> }
> 
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/ipc/DOMTypes.ipdlh b/dom/ipc/DOMTypes.ipdlh
>--- a/dom/ipc/DOMTypes.ipdlh
>+++ b/dom/ipc/DOMTypes.ipdlh
>@@ -2,16 +2,17 @@
> /* vim: set sw=4 ts=8 et tw=80 ft=cpp : */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> include protocol PBlob;
> include protocol PMemoryStream;
> 
>+include IPCBlob;
> include IPCStream;
> include ProtocolTypes;
> 
> using struct mozilla::void_t
>   from "ipc/IPCMessageUtils.h";
> 
> using struct mozilla::SerializedStructuredCloneBuffer
>   from "ipc/IPCMessageUtils.h";
>@@ -36,17 +37,17 @@ struct MessagePortIdentifier
>  * Cross-process representation for postMessage() style payloads where Blobs may
>  * be referenced/"cloned" and (optionally) messageports transferred.  Use
>  * StructuredCloneData in your code to convert between this wire representation
>  * and the StructuredCloneData StructuredCloneHolder-subclass.
>  */
> struct ClonedMessageData
> {
>   SerializedStructuredCloneBuffer data;
>-  PBlob[] blobs;
>+  IPCBlob[] blobs;
>   IPCStream[] inputStreams;
>   MessagePortIdentifier[] identfiers;
> };
> 
> struct BlobDataStream
> {
>   PMemoryStream stream;
>   uint64_t length;
>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
>--- a/dom/ipc/PBrowser.ipdl
>+++ b/dom/ipc/PBrowser.ipdl
>@@ -15,16 +15,18 @@ include protocol PFilePicker;
> include protocol PIndexedDBPermissionRequest;
> include protocol PRenderFrame;
> include protocol PPluginWidget;
> include protocol PRemotePrintJob;
> include protocol PChildToParentStream;
> include protocol PParentToChildStream;
> include protocol PFileDescriptorSet;
> include DOMTypes;
>+include IPCBlob;
>+include IPCStream;
> include JavaScriptTypes;
> include URIParams;
> include PPrintingTypes;
> include PTabContext;
> 
> 
> using class IPC::Principal from "mozilla/dom/PermissionMessageUtils.h";
> using class mozilla::gfx::Matrix from "mozilla/gfx/Matrix.h";
>diff --git a/dom/ipc/StructuredCloneData.cpp b/dom/ipc/StructuredCloneData.cpp
>--- a/dom/ipc/StructuredCloneData.cpp
>+++ b/dom/ipc/StructuredCloneData.cpp
>@@ -8,16 +8,17 @@
> 
> #include "nsIDOMDOMException.h"
> #include "nsIMutable.h"
> #include "nsIXPConnect.h"
> 
> #include "ipc/IPCMessageUtils.h"
> #include "mozilla/dom/BindingUtils.h"
> #include "mozilla/dom/BlobBinding.h"
>+#include "mozilla/dom/IPCBlobUtils.h"
> #include "mozilla/dom/File.h"
> #include "mozilla/ipc/IPCStreamUtils.h"
> #include "nsContentUtils.h"
> #include "nsJSEnvironment.h"
> #include "MainThreadUtils.h"
> #include "StructuredCloneTags.h"
> #include "jsapi.h"
> 
>@@ -137,102 +138,19 @@ enum ActorFlavorEnum {
>   Child,
> };
> 
> enum ManagerFlavorEnum {
>   ContentProtocol = 0,
>   BackgroundProtocol
> };
> 
>-template <ActorFlavorEnum>
>-struct BlobTraits
>-{ };
>-
>-template <>
>-struct BlobTraits<Parent>
>-{
>-  typedef mozilla::dom::BlobParent BlobType;
>-  typedef mozilla::dom::PBlobParent ProtocolType;
>-};
>-
>-template <>
>-struct BlobTraits<Child>
>-{
>-  typedef mozilla::dom::BlobChild BlobType;
>-  typedef mozilla::dom::PBlobChild ProtocolType;
>-};
>-
>-template <ActorFlavorEnum, ManagerFlavorEnum>
>-struct ParentManagerTraits
>-{ };
>-
>-template<>
>-struct ParentManagerTraits<Parent, ContentProtocol>
>-{
>-  typedef mozilla::dom::nsIContentParent ConcreteContentManagerType;
>-};
>-
>-template<>
>-struct ParentManagerTraits<Child, ContentProtocol>
>-{
>-  typedef mozilla::dom::nsIContentChild ConcreteContentManagerType;
>-};
>-
>-template<>
>-struct ParentManagerTraits<Parent, BackgroundProtocol>
>-{
>-  typedef mozilla::ipc::PBackgroundParent ConcreteContentManagerType;
>-};
>-
>-template<>
>-struct ParentManagerTraits<Child, BackgroundProtocol>
>-{
>-  typedef mozilla::ipc::PBackgroundChild ConcreteContentManagerType;
>-};
>-
>-template<ActorFlavorEnum>
>-struct DataBlobs
>-{ };
>-
>-template<>
>-struct DataBlobs<Parent>
>-{
>-  typedef BlobTraits<Parent>::ProtocolType ProtocolType;
>-
>-  static InfallibleTArray<ProtocolType*>& Blobs(ClonedMessageData& aData)
>-  {
>-    return aData.blobsParent();
>-  }
>-
>-  static const InfallibleTArray<ProtocolType*>& Blobs(const ClonedMessageData& aData)
>-  {
>-    return aData.blobsParent();
>-  }
>-};
>-
>-template<>
>-struct DataBlobs<Child>
>-{
>-  typedef BlobTraits<Child>::ProtocolType ProtocolType;
>-
>-  static InfallibleTArray<ProtocolType*>& Blobs(ClonedMessageData& aData)
>-  {
>-    return aData.blobsChild();
>-  }
>-
>-  static const InfallibleTArray<ProtocolType*>& Blobs(const ClonedMessageData& aData)
>-  {
>-    return aData.blobsChild();
>-  }
>-};
>-
>-template<ActorFlavorEnum Flavor, ManagerFlavorEnum ManagerFlavor>
>-static bool
>-BuildClonedMessageData(typename ParentManagerTraits<Flavor, ManagerFlavor>::ConcreteContentManagerType* aManager,
>-                       StructuredCloneData& aData,
>+template<typename M>
>+bool
>+BuildClonedMessageData(M* aManager, StructuredCloneData& aData,
>                        ClonedMessageData& aClonedData)
> {
>   SerializedStructuredCloneBuffer& buffer = aClonedData.data();
>   auto iter = aData.Data().Iter();
>   size_t size = aData.Data().Size();
>   bool success;
>   buffer.data = aData.Data().Borrow<js::SystemAllocPolicy>(iter, size, &success);
>   if (NS_WARN_IF(!success)) {
>@@ -240,33 +158,36 @@ BuildClonedMessageData(typename ParentMa
>   }
>   if (aData.SupportsTransferring()) {
>     aClonedData.identfiers().AppendElements(aData.PortIdentifiers());
>   }
> 
>   const nsTArray<RefPtr<BlobImpl>>& blobImpls = aData.BlobImpls();
> 
>   if (!blobImpls.IsEmpty()) {
>-    typedef typename BlobTraits<Flavor>::BlobType BlobType;
>-    typedef typename BlobTraits<Flavor>::ProtocolType ProtocolType;
>-    InfallibleTArray<ProtocolType*>& blobList = DataBlobs<Flavor>::Blobs(aClonedData);
>-    uint32_t length = blobImpls.Length();
>-    blobList.SetCapacity(length);
>-    for (uint32_t i = 0; i < length; ++i) {
>-      BlobType* protocolActor = BlobType::GetOrCreate(aManager, blobImpls[i]);
>-      if (!protocolActor) {
>+    if (NS_WARN_IF(!aClonedData.blobs().SetLength(blobImpls.Length(), fallible))) {
>+      return false;
>+    }
>+
>+    for (uint32_t i = 0; i < blobImpls.Length(); ++i) {
>+      nsresult rv = IPCBlobUtils::Serialize(blobImpls[i], aManager,
>+                                            aClonedData.blobs()[i]);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>         return false;
>       }
>-      blobList.AppendElement(protocolActor);
>     }
>   }
> 
>   const nsTArray<nsCOMPtr<nsIInputStream>>& inputStreams = aData.InputStreams();
>+  if (!inputStreams.IsEmpty()) {
>+    if (NS_WARN_IF(!aData.IPCStreams().SetCapacity(inputStreams.Length(),
>+                                                   fallible))) {
>+      return false;
>+    }
> 
>-  if (!inputStreams.IsEmpty()) {
>     InfallibleTArray<IPCStream>& streams = aClonedData.inputStreams();
>     uint32_t length = inputStreams.Length();
>     streams.SetCapacity(length);
>     for (uint32_t i = 0; i < length; ++i) {
>       AutoIPCStream* stream = aData.IPCStreams().AppendElement(fallible);
>       if (NS_WARN_IF(!stream)) {
>         return false;
>       }
>@@ -281,41 +202,41 @@ BuildClonedMessageData(typename ParentMa
>   return true;
> }
> 
> bool
> StructuredCloneData::BuildClonedMessageDataForParent(
>   nsIContentParent* aParent,
>   ClonedMessageData& aClonedData)
> {
>-  return BuildClonedMessageData<Parent, ContentProtocol>(aParent, *this, aClonedData);
>+  return BuildClonedMessageData(aParent, *this, aClonedData);
> }
> 
> bool
> StructuredCloneData::BuildClonedMessageDataForChild(
>   nsIContentChild* aChild,
>   ClonedMessageData& aClonedData)
> {
>-  return BuildClonedMessageData<Child, ContentProtocol>(aChild, *this, aClonedData);
>+  return BuildClonedMessageData(aChild, *this, aClonedData);
> }
> 
> bool
> StructuredCloneData::BuildClonedMessageDataForBackgroundParent(
>   PBackgroundParent* aParent,
>   ClonedMessageData& aClonedData)
> {
>-  return BuildClonedMessageData<Parent, BackgroundProtocol>(aParent, *this, aClonedData);
>+  return BuildClonedMessageData(aParent, *this, aClonedData);
> }
> 
> bool
> StructuredCloneData::BuildClonedMessageDataForBackgroundChild(
>   PBackgroundChild* aChild,
>   ClonedMessageData& aClonedData)
> {
>-  return BuildClonedMessageData<Child, BackgroundProtocol>(aChild, *this, aClonedData);
>+  return BuildClonedMessageData(aChild, *this, aClonedData);
> }
> 
> // See the StructuredCloneData class block comment for the meanings of each val.
> enum MemoryFlavorEnum {
>   BorrowMemory = 0,
>   CopyMemory,
>   StealMemory
> };
>@@ -368,35 +289,30 @@ struct MemoryTraits<StealMemory>
> // and Child/BackgroundChild in this implementation.  The calling methods,
> // however, do maintain the distinction for code-reading purposes and are backed
> // by assertions to enforce there is no misuse.
> template<MemoryFlavorEnum MemoryFlavor, ActorFlavorEnum Flavor>
> static void
> UnpackClonedMessageData(typename MemoryTraits<MemoryFlavor>::ClonedMessageType& aClonedData,
>                         StructuredCloneData& aData)
> {
>-  typedef typename BlobTraits<Flavor>::ProtocolType ProtocolType;
>-  const InfallibleTArray<ProtocolType*>& blobs = DataBlobs<Flavor>::Blobs(aClonedData);
>   const InfallibleTArray<MessagePortIdentifier>& identifiers = aClonedData.identfiers();
> 
>   MemoryTraits<MemoryFlavor>::ProvideBuffer(aClonedData, aData);
> 
>   if (aData.SupportsTransferring()) {
>     aData.PortIdentifiers().AppendElements(identifiers);
>   }
> 
>+  const nsTArray<IPCBlob>& blobs = aClonedData.blobs();
>   if (!blobs.IsEmpty()) {
>     uint32_t length = blobs.Length();
>     aData.BlobImpls().SetCapacity(length);
>     for (uint32_t i = 0; i < length; ++i) {
>-      auto* blob =
>-        static_cast<typename BlobTraits<Flavor>::BlobType*>(blobs[i]);
>-      MOZ_ASSERT(blob);
>-
>-      RefPtr<BlobImpl> blobImpl = blob->GetBlobImpl();
>+      RefPtr<BlobImpl> blobImpl = IPCBlobUtils::Deserialize(blobs[i]);
>       MOZ_ASSERT(blobImpl);
> 
>       aData.BlobImpls().AppendElement(blobImpl);
>     }
>   }
> 
>   const InfallibleTArray<IPCStream>& streams = aClonedData.inputStreams();
>   if (!streams.IsEmpty()) {
>diff --git a/dom/ipc/StructuredCloneData.h b/dom/ipc/StructuredCloneData.h
>--- a/dom/ipc/StructuredCloneData.h
>+++ b/dom/ipc/StructuredCloneData.h
>@@ -198,18 +198,18 @@ public:
> 
>   void Write(JSContext* aCx,
>              JS::Handle<JS::Value> aValue,
>              JS::Handle<JS::Value> aTransfers,
>              ErrorResult &aRv);
> 
>   // Actor-varying methods to convert the structured clone stored in this holder
>   // by a previous call to Write() into ClonedMessageData IPC representation.
>-  // (Blobs are represented in IPC by PBlob actors, so we need the parent to be
>-  // able to create them.)
>+  // (Blobs are represented in IPC by IPCBlob actors, so we need the parent to
>+  // be able to create them.)
>   bool BuildClonedMessageDataForParent(nsIContentParent* aParent,
>                                        ClonedMessageData& aClonedData);
>   bool BuildClonedMessageDataForChild(nsIContentChild* aChild,
>                                       ClonedMessageData& aClonedData);
>   bool BuildClonedMessageDataForBackgroundParent(mozilla::ipc::PBackgroundParent* aParent,
>                                                  ClonedMessageData& aClonedData);
>   bool BuildClonedMessageDataForBackgroundChild(mozilla::ipc::PBackgroundChild* aChild,
>                                                 ClonedMessageData& aClonedData);

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859499 [details] [diff] [review]
part 7 - IPCBlobInputStream must implement nsIAsyncInputStream

>+  // This is the list of possible states.
>+  enum {
>+    // The initial state. Only ::Available() can be used without receiving an
>+    // error. The available size is known by the actor.
>+    eInit,
>+
>+    // AsyncWait() has been called for the first time. SendStreamNeeded() has
>+    // been called and we are waiting for the 'real' inputStream. We are in this
>+    // state.
"We are in this state." ?

>@@ -58,15 +67,23 @@ private:
>   Mutex mMutex;
You must document which all member variables this protects. This patch makes mActorAlive to be protected by the mutex.
And mPendingOperations


>+
>+  // Only 1 of these 2 is set.
>+  nsIContentParent* mContentManager;
>+  mozilla::ipc::PBackgroundParent* mPBackgroundManager;
This is missing to address review comment about the raw pointers as member variables. Please add a comment why those are safe.


Please don't land these patches without some testing, like the one I mentioned earlier, memory usage and also check whether we do a lot more memcpying.
(I assume memusage doesn't go up in any meaningful way, but we may do memcpying more)

And we need to ensure we have automated tests for blob sending across processes using difference methods.

Comment 47

[Andrea Marchesini [:baku]]

We use the nsBufferedStream wrapper only if the original stream does not support ReadSegments and this happens only with this new code. In theory this code should not add any performance regression.

About testing, I'll add a separate patch just for them.

Comment 48

[Andrea Marchesini [:baku]]

Attachment: part 12 - IPCBlobInputStream must support nsIAsyncInputStreams

This was the last bit missing to make try happy again.

Comment 49

[Andrea Marchesini [:baku]]

Attachment: part 12 - IPCBlobInputStream must support nsIAsyncInputStreams

Comment 50

[Andrea Marchesini [:baku]]

Attachment: part 12 - IPCBlobInputStream must support nsIAsyncInputStreams

Comment 51

[Andrea Marchesini [:baku]]

Attachment: part 13 - tests

Comment 52

[Andrea Marchesini [:baku]]

Attachment: part 14 - labeling dom/file/ipc

Comment 53

[Andrea Marchesini [:baku]]

Attachment: part 13 - tests

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859934 [details] [diff] [review]
part 13 - tests

>+// More than 1mb memory blob childA-parent-childB: BroadcastChannel
>+add_task(function* test_CtoPtoC_bc_big() {
>+  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, BASE_URI);
>+  let browser1 = gBrowser.getBrowserForTab(tab1);
>+
>+  yield ContentTask.spawn(browser1, null, function() {
>+    var bc = new content.BroadcastChannel('test');
>+    bc.onmessage = function() {
>+      bc.postMessage(new Blob([new Array(1024*1024).join('123456789ABCDEF')]));
>+    }
>+  });
>+
>+  let tab2 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, BASE_URI);
>+  let browser2 = gBrowser.getBrowserForTab(tab2);
>+
>+  let status = yield ContentTask.spawn(browser2, null, function() {
>+    return new Promise(resolve => {
>+      var bc = new content.BroadcastChannel('test');
>+      bc.onmessage = function(e) {
>+        let fr = new content.FileReader();
>+        fr.readAsText(e.data);
>+        fr.onloadend = function() {
>+          resolve(fr.result == new Array(1024*1024).join('123456789ABCDEF'));
>+        }
>+      }
>+
>+      bc.postMessage("GO!");
>+    });
>+  });
Looks like a racy test. What guarantees browser2 is there when browser1 sends the message.
In fact, I'm surprised if this doesn't fail on try, since if it doesn't, does it mean that we keep messages queued for too long?
Or perhaps this just happens to rely on something being a bit slow on child side when opening a tab.
But please add the event listener before sending a message. (and please also check that we don't keep messages queued for too long)


>+add_task(function* test_CtoPtoC_bc_small() {
Same with this.

Please ensure that the opened browser1 and browser2 are in separate processes.

Comment 55

[Andrea Marchesini [:baku]]

> Looks like a racy test. What guarantees browser2 is there when browser1
> sends the message.

Well, the tab is still opened. I don't close it. Why the browser should be gone?

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859921 [details] [diff] [review]
part 14 - labeling dom/file/ipc

I don't understand this at all. This isn't labeling anything.

And you want bevis to review this kind of change since I don't understand undocumented AbstractThread usage for labeling.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859934 [details] [diff] [review]
part 13 - tests

ah, bad review. There is ping-pong.

But still add checks that browsers run in different processes, in case multiple child processes are enabled.

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859934 [details] [diff] [review]
part 13 - tests

Do we have tests for blob URIs across processes? If not, please add.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859915 [details] [diff] [review]
part 12 - IPCBlobInputStream must support nsIAsyncInputStreams

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  039a5d6f2ae734a01601a5758162b96491bc715f
>Bug 1353629 - PBlob refactoring - part 12 - IPCBlobInputStream should support remote nsIAsyncInputStream, r?smaug
>
>If a child-to-parent IPCBlob is more than 1mb, we end up using a pipe stream.
>If that ipcBlob is sent to a different process, we need to implement asyncWait
>correctly.
This doesn't explain what "correctly" means.

Please improve the comment a bit.

Comment 60

[Andrea Marchesini [:baku]]

Comment on attachment 8859921 [details] [diff] [review]
part 14 - labeling dom/file/ipc

I move this patch to bug 1358105

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cdd50772eec
PBlob refactoring - part 0 - IPCBlob, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f738217ff5
PBlob refactoring - part 1 - AutoIPCStream should not have a copy CTOR, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a490c41305c6
PBlob refactoring - part 2 - IPCStream must be available on PBackground, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1024114af681
PBlob refactoring - part 3 - IPCBlob in ClonedMessageData, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b53bdd61d64
PBlob refactoring - part 4 - IPCBlobInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4315a6ac52a4
PBlob refactoring - part 5 - IPCBlobInputStreamStorage, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/695957269f58
PBlob refactoring - part 6 - IPCBlobInputStream serialization, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/71785db3c382
PBlob refactoring - part 7 - IPCBlobInputStream must implement nsIAsyncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea2ce5d3c17
PBlob refactoring - part 8 - FileReader should use nsIAsyncInputStream if available, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c930526369c2
PBlob refactoring - part 9 - PBlob should use IPCStream in case it is dealing with an IPCBlobInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b225ae4bfc09
PBlob refactoring - part 10 - Make nsFileInputStream cloneable, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/607b25ac6c31
PBlob refactoring - part 11 - Comments, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/747a31f9b7b9
PBlob refactoring - part 12 - nsInputStreamPump should use BufferedStreams, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/30953613fff7
PBlob refactoring - part 13 - IPCBlobInputStream should support remote nsIAsyncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ab7cc36551
PBlob refactoring - part 14 - tests, r=smaug

Comment 64

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1353629 and bug 1353475:

Bug 1353475:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b728b818169c35e1dc65b71d424e9542abb52b0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e574e7afdf5fbdb5620c483164e997c1ea4d959f

Bug 1353629:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8a7f3baf26ff74d94ec1eec64d04b8051a3d41e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67dc5007a113d3eeaf6d4fe2e85bbe1a9a4533a
https://hg.mozilla.org/integration/mozilla-inbound/rev/802cbff80adaa3051cf40621099887523a27c439
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab390bce545cceed9f0faed321427d014bcead1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca68d3ea9eb9a53133c13bf40fa8e120737c6788
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbd6a09cdeef9fd9df5dcdf40a65d459681e57c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f58b24833af45056c7d54bc8fd7f234f903da55
https://hg.mozilla.org/integration/mozilla-inbound/rev/b69c86f4235ebc6bef174baebe2cf940ad275b77
https://hg.mozilla.org/integration/mozilla-inbound/rev/76e6ae0b86fb49653ee65a9b1b8baffc20e13f62
https://hg.mozilla.org/integration/mozilla-inbound/rev/564bd22ce8898109f80ef3b421e9c5329cc86dc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/cabdccfa10f0ed0a934b395c60408ed5de6d2306
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb12febc369226d617febbf08af6b4178ce46f29
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c48858c3812645a72ed9896202a41849281666c
https://hg.mozilla.org/integration/mozilla-inbound/rev/86f4670afa32d1d5f7e759295c718277d81dc03d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e8c3d77689601007dff98d61b3149d3f461660

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d4ab7cc36551d885b6640e180ead7093bf610a4c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=93042867&repo=mozilla-inbound

08:30:25     INFO - TEST-START | dom/filesystem/compat/tests/test_formSubmission.html
08:30:25     INFO - GECKO(5220) | Unable to read VR Path Registry from C:\Users\cltbld\AppData\Local\openvr\openvrpaths.vrpath
08:30:25     INFO - GECKO(5220) | JavaScript error: http://mochi.test:8888/tests/dom/filesystem/compat/tests/script_entries.js, line 12: NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.createUnique]
08:35:53     INFO - TEST-INFO | started process screenshot
08:35:53     INFO - TEST-INFO | screenshot: exit 0
08:35:53     INFO - TEST-UNEXPECTED-FAIL | dom/filesystem/compat/tests/test_formSubmission.html | Test timed out. 
08:35:53     INFO -     reportError@SimpleTest/TestRunner.js:121:7
08:35:53     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:380:5
08:35:53     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
08:35:53     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
08:35:53     INFO -     hookupTests@SimpleTest/setup.js:266:5
08:35:53     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
08:35:53     INFO -     hookup@SimpleTest/setup.js:246:5
08:35:53     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cusers%5Ccltbld%5Cappdata%5Clocal%5Ctemp&cleanupCrashes=true:11:1

08:35:54     INFO - TEST-START | dom/filesystem/compat/tests/test_no_dnd.html
08:35:54     INFO - GECKO(5220) | JavaScript error: http://mochi.test:8888/tests/dom/filesystem/compat/tests/script_entries.js, line 12: NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.createUnique]
08:36:17     INFO -  JavaScript error: resource://gre/modules/FormHistory.jsm, line 373: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]
08:36:17     INFO -  JavaScript error: resource://gre/modules/PlacesUtils.jsm, line 1988: NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]
08:41:23     INFO - Not taking screenshot here: see the one that was previously logged
08:41:23     INFO - TEST-UNEXPECTED-FAIL | dom/filesystem/compat/tests/test_no_dnd.html | Test timed out. 
08:41:23     INFO -     reportError@SimpleTest/TestRunner.js:121:7
08:41:23     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:380:5
08:41:23     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
08:41:23     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
08:41:23     INFO -     hookupTests@SimpleTest/setup.js:266:5
08:41:23     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
08:41:23     INFO -     hookup@SimpleTest/setup.js:246:5
08:41:23     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cusers%5Ccltbld%5Cappdata%5Clocal%5Ctemp&cleanupCrashes=true:11:1

Comment 65

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please also check these media test issues which also start with that push and only apply to e10s. The previous 10+ pushes don't have the issue, all after that have:
https://treeherder.mozilla.org/logviewer.html#?job_id=93025942&repo=mozilla-inbound

[task 2017-04-20T13:54:50.118389Z] 13:54:50     INFO - TEST-PASS | dom/media/test/test_buffered.html | seek.ogv: Fetch succeeded, status=200 
[task 2017-04-20T13:54:50.119194Z] 13:54:50     INFO - Buffered messages finished
[task 2017-04-20T13:54:50.120058Z] 13:54:50     INFO - TEST-UNEXPECTED-FAIL | dom/media/test/test_buffered.html | seek.ogv: Should be buffered in one range - got 2, expected 1
[task 2017-04-20T13:54:50.120794Z] 13:54:50     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:310:5
[task 2017-04-20T13:54:50.121569Z] 13:54:50     INFO -     testBuffered@dom/media/test/test_buffered.html:31:3
[task 2017-04-20T13:54:50.122287Z] 13:54:50     INFO -     EventListener.handleEvent*onfetched@dom/media/test/test_buffered.html:102:5
[task 2017-04-20T13:54:50.131574Z] 13:54:50     INFO -     loaded@dom/media/test/test_buffered.html:82:7
[task 2017-04-20T13:54:50.132305Z] 13:54:50     INFO -     EventListener.handleEvent*fetch@dom/media/test/test_buffered.html:88:3
[task 2017-04-20T13:54:50.133050Z] 13:54:50     INFO -     startTest@dom/media/test/test_buffered.html:107:3
[task 2017-04-20T13:54:50.133762Z] 13:54:50     INFO -     EventListener.handleEvent*onfetched@dom/media/test/test_buffered.html:102:5
[task 2017-04-20T13:54:50.134475Z] 13:54:50     INFO -     loaded@dom/media/test/test_buffered.html:82:7
[task 2017-04-20T13:54:50.135201Z] 13:54:50     INFO -     EventListener.handleEvent*fetch@dom/media/test/test_buffered.html:88:3
[task 2017-04-20T13:54:50.135914Z] 13:54:50     INFO -     startTest@dom/media/test/test_buffered.html:107:3
[task 2017-04-20T13:54:50.136641Z] 13:54:50     INFO -     MediaTestManager/this.nextTest@dom/media/test/manifest.js:1726:7
[task 2017-04-20T13:54:50.137345Z] 13:54:50     INFO -     MediaTestManager/this.runTests/<@dom/media/test/manifest.js:1648:7

Comment 66

[Andrea Marchesini [:baku]]

Attachment: part 14 - media tests

This makes try happy again.

Comment 67

[Andrea Marchesini [:baku]]

Attachment: part 14 - FileMediaResource is used for in-process blobURL

Comment 68

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8860411 [details] [diff] [review]
part 14 - FileMediaResource is used for in-process blobURL

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

::: dom/media/MediaResource.cpp
@@ +1525,2 @@
>    }
> +

I would try to avoid overly nested if statements whenever possible:

nsCOMPtr<nsIInputStream> stream;
nsCOMPtr<nsISeekableStream> seekableStream;
if (IsBlobURI(uri) &&
    NS_SUCCEEDED(NS_GetStreamForBlobURI(uri, getter_AddRefs(stream))) &&
    (seekableStream = do_QueryInterface(stream))) {
  // ...
}

::: dom/media/MediaResource.h
@@ +315,5 @@
>    /**
>     * Create a resource, reading data from the channel. Call on main thread only.
>     * The caller must follow up by calling resource->Open().
>     */
> +  static MediaResource*

Any reason to change the return type? already_AddRefed<> is good to avoid leaks.

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5325e5a922
PBlob refactoring - part 0 - IPCBlob, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3eec2909b7
PBlob refactoring - part 1 - AutoIPCStream should not have a copy CTOR, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/989411dbd27c
PBlob refactoring - part 2 - IPCStream must be available on PBackground, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e257576d44a4
PBlob refactoring - part 3 - IPCBlob in ClonedMessageData, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9400156bf73
PBlob refactoring - part 4 - IPCBlobInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fd6cad1a8d9
PBlob refactoring - part 5 - IPCBlobInputStreamStorage, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8496fe603461
PBlob refactoring - part 6 - IPCBlobInputStream serialization, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ff18334e6b
PBlob refactoring - part 7 - IPCBlobInputStream must implement nsIAsyncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cee81d4988a5
PBlob refactoring - part 8 - FileReader should use nsIAsyncInputStream if available, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d9349c6184
PBlob refactoring - part 9 - PBlob should use IPCStream in case it is dealing with an IPCBlobInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c786e61d5c8
PBlob refactoring - part 10 - Make nsFileInputStream cloneable, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2d2d49d3e57
PBlob refactoring - part 11 - Comments, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a2c881ccf41
PBlob refactoring - part 12 - nsInputStreamPump should use BufferedStreams, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6586412c7f64
PBlob refactoring - part 13 - IPCBlobInputStream should support remote nsIAsyncInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b93b33542bcf
PBlob refactoring - part 14 - tests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e18a14aa213
PBlob refactoring - part 15 - FileMediaResource is used for in-process blobURL, r=jwwang

Comment 70

[Joel Maher ( :jmaher ) (UTC -8)]

this appeared to cause a talos regression when it landed:
== Change summary for alert #6116 (as of April 20 2017 13:57 UTC) ==

Regressions:

  3%  bloom_basic http: windows8-64 opt e10s     698.63 -> 722.73
  3%  bloom_basic_ref http: windows8-64 opt e10s 706.37 -> 729.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6116

the regression went away- this is a new test, we are still getting used to the patterns of it- but might be worth running talos against this on try when you push again.

Comment 71

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c5325e5a922
https://hg.mozilla.org/mozilla-central/rev/1b3eec2909b7
https://hg.mozilla.org/mozilla-central/rev/989411dbd27c
https://hg.mozilla.org/mozilla-central/rev/e257576d44a4
https://hg.mozilla.org/mozilla-central/rev/e9400156bf73
https://hg.mozilla.org/mozilla-central/rev/8fd6cad1a8d9
https://hg.mozilla.org/mozilla-central/rev/8496fe603461
https://hg.mozilla.org/mozilla-central/rev/a1ff18334e6b
https://hg.mozilla.org/mozilla-central/rev/cee81d4988a5
https://hg.mozilla.org/mozilla-central/rev/04d9349c6184
https://hg.mozilla.org/mozilla-central/rev/1c786e61d5c8
https://hg.mozilla.org/mozilla-central/rev/a2d2d49d3e57
https://hg.mozilla.org/mozilla-central/rev/3a2c881ccf41
https://hg.mozilla.org/mozilla-central/rev/6586412c7f64
https://hg.mozilla.org/mozilla-central/rev/b93b33542bcf
https://hg.mozilla.org/mozilla-central/rev/6e18a14aa213

Comment 72

[The 8472]

Is it possible that this bug has broken files added via drag&drop? I'm seeing some issues where files added to a form in that way occasionally stuck uploading (via XHR+Formdata) while adding via <input type=file> works reliably.

Comment 73

[Olli Pettay [:smaug][[e-mail]]]

Yes. Please file a bug.


And baku, sounds like there are now bad enough regressions that we need some backouts asap.

Comment 74

[Andrea Marchesini [:baku]]

I have uploaded patches for bug 1361315 and bug 1360340

Comment 82

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.