1350386 - SessionStore Utils.serializeInputStream() seems buggy

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Andrea Marchesini [:baku]]

First of all, I don't know in which component I should file this bug. Probably we need to move it.

Utils.serializeInputStream uses: NetUtil.readInputStreamToString(aStream, aStream.available()).
Then, often, the serialized inputStream is sent via IPC. This means that:

1. if the inputStream is big enough, we could reach the limit of size for IPC messages. Then we crash.
2. If the inputStream is a nsIFileInputStream, we end up doing I/O on the mainthread(?).

What I suggest is: we implement the support of nsIInputStream in the StructuredCloneHolder class exactly as we support nsIPrincipal, Blob, and many other object types. In this way, instead serializing the full content of the nsInputStream, we can create a PChildToParentStream (or PParentToChildStream) actor. Nice feature, if we do so, we send data across processes only when the stream is read for real.

Comment 1

[:Gijs (he/him)]

Mike, was this already on your session store improvement radar?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrea Marchesini [:baku] from comment #0)
> What I suggest is: we implement the support of nsIInputStream in the
> StructuredCloneHolder class exactly as we support nsIPrincipal, Blob, and
> many other object types. In this way, instead serializing the full content
> of the nsInputStream, we can create a PChildToParentStream (or
> PParentToChildStream) actor. Nice feature, if we do so, we send data across
> processes only when the stream is read for real.

In the situation where we're handing a stream which we got from the parent process back to the parent process (as in we're streaming data from parent -> child, and then passing the handle back to the parent), is there some way we could make the perf overhead of via-ing all of the data through the child go away in this situation? That would be pretty awesome.

Comment 3

[Andrea Marchesini [:baku]]

Not yet. But I'm implementing something like that for PBlob.

Comment 4

[Andrea Marchesini [:baku]]

I'm introducing the serialization of inputStreams in StructuredCloneAlgorithm.
For now we can use PChildToParentStream and PStreamToChildStream. We can be smarter in a second step.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: StructuredCloneAlgorithm + nsIInputStream

This patch allows the sending of an inputStream cross processes.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8854449 [details] [diff] [review]
StructuredCloneAlgorithm + nsIInputStream

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

::: dom/base/StructuredCloneHolder.cpp
@@ +1071,5 @@
> +  MOZ_ASSERT(aWriter);
> +  MOZ_ASSERT(aInputStream);
> +  MOZ_ASSERT(aHolder);
> +
> +  // We store the position of the blobImpl in the array as index.

This is an array of nsIInputStreams, not blobImpls.

::: dom/base/test/browser_inputStream_structuredClone.js
@@ +22,5 @@
> +  };
> +
> +  is(data.inputStream.available(), input.length, "The length of the inputStream matches: " + input.length);
> +
> +  let dataBack = yield ContentTask.spawn(browser, data, function(data) {

It would be nice to assert that we can see the inputStream in the content process as well as in the parent process, with an assert such as `ok(data.inputStream instanceof Ci.nsIInputStream)`. It would also be nice to potentially read the stream in the other process to confirm that the data is in fact transmitted correctly.

::: dom/ipc/StructuredCloneData.cpp
@@ +49,5 @@
> +StructuredCloneData::operator=(StructuredCloneData&& aOther)
> +{
> +  mExternalData = Move(aOther.mExternalData);
> +  mSharedData.swap(aOther.mSharedData);
> +  mIPCStreams.SwapElements(aOther.mIPCStreams);

The fact that we're swapping elements and pointers here instead of moving means that aOther will be left with random pieces of this StructuredCloneData's internal state at time of assignment. This seems undesirable. I feel like it would be nicer to explicitly move the data over to not leave the other StructuredCloneData in an odd state.

@@ -208,5 @@
>      aClonedData.identfiers().AppendElements(aData.PortIdentifiers());
>    }
>  
>    const nsTArray<RefPtr<BlobImpl>>& blobImpls = aData.BlobImpls();
> -

Why remove this line?

@@ +263,5 @@
> +    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);

Why does this array exist? From reading the documentation on AutoIPCStream, it seems like this loop could have just been written with this type on the stack, like:

AutoIPCStream autoStream;
autoStream->Serialize(inputStreams[i], aManager);
streams.AppendElement(autoStream->TakeValue());

TakeValue does return a reference into the AutoIPCStream, but `streams` is of type `nsTArray<IPCStream>&`, and thus will trigger a copy. I don't think that we need to keep the AutoIPCStream alive past the creation of the IPCStream.

Comment 7

[Andrea Marchesini [:baku]]

> Why does this array exist? From reading the documentation on AutoIPCStream,
> it seems like this loop could have just been written with this type on the
> stack, like:

AutoIPCStream must exist when the data is sent. In the DTOR it starts sending data. This must happen after the sending of the actor otherwise it can happen that that actor. We have similar pattern in PBlob, Cache API and so on.
I'm planning to change this, but so far, we have to keep this setup.

I'll apply the other comments.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrea Marchesini [:baku] from comment #7)
> AutoIPCStream must exist when the data is sent. In the DTOR it starts
> sending data. This must happen after the sending of the actor otherwise it
> can happen that that actor. We have similar pattern in PBlob, Cache API and
> so on.
> I'm planning to change this, but so far, we have to keep this setup.

That's super gross :S. Looking forward to not having this problem in the future :).

Comment 9

[Andrea Marchesini [:baku]]

> It would be nice to assert that we can see the inputStream in the content
> process as well as in the parent process, with an assert such as
> `ok(data.inputStream instanceof Ci.nsIInputStream)`. It would also be nice
> to potentially read the stream in the other process to confirm that the data
> is in fact transmitted correctly.

This cannot be done unfortunately. nsIInputStream.read() is not scriptable and in web content we don't have Components.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: serialize_IS.patch

Comment 11

[:Gijs (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #9)
> > It would be nice to assert that we can see the inputStream in the content
> > process as well as in the parent process, with an assert such as
> > `ok(data.inputStream instanceof Ci.nsIInputStream)`. It would also be nice
> > to potentially read the stream in the other process to confirm that the data
> > is in fact transmitted correctly.
> 
> This cannot be done unfortunately. nsIInputStream.read() is not scriptable
> and in web content we don't have Components.

It's a browser test, it and functions you pass to ContentTask are system-privileged and have a Components object.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8854516 [details] [diff] [review]
serialize_IS.patch

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

::: dom/base/test/browser_inputStream_structuredClone.js
@@ +23,5 @@
> +
> +  is(data.inputStream.available(), input.length, "The length of the inputStream matches: " + input.length);
> +
> +  let dataBack = yield ContentTask.spawn(browser, data, function(data) {
> +    return {

I'm pretty sure that Ci is available in the ContentTask. The code in the ContentTask is evaluated here: http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/testing/mochitest/BrowserTestUtils/content/content-task.js#52 which is where the scope gets methods like `ok` from. That scope has access to both `Components` and bindings like `Ci`, which makes me think that we can do the instanceof comparison in the content process.

::: dom/ipc/StructuredCloneData.cpp
@@ +265,5 @@
> +    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);

Perhaps add a comment here explaining the reason why the AutoIPCStream array is required. If there's a bug on file for fixing that issue, perhaps reference that bug in the comment too :).

Comment 13

[Andrea Marchesini [:baku]]

Right, I can use instanceof Ci.nsIInputStream but only for about:about URL.
So, when this code is in m-i/m-c, we can proceed with the rest of the bug.
It's important to know if sessionStore and the rest of the browser UI is able to work with non-blocking nsIInputStream.
Can you maybe test it creating a big postData (big means more than 1mb) and see if everything works correctly?

Comment 14

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrea Marchesini [:baku] from comment #13)
> Right, I can use instanceof Ci.nsIInputStream but only for about:about URL.

Ci.nsIInputStream is avaliable even for non-privileged URLs. ContentTasks are always run as privileged JS with access to Components.

> So, when this code is in m-i/m-c, we can proceed with the rest of the bug.
> It's important to know if sessionStore and the rest of the browser UI is
> able to work with non-blocking nsIInputStream.
> Can you maybe test it creating a big postData (big means more than 1mb) and
> see if everything works correctly?

I'll ni? :bobowen who was working on the other patch

Comment 15

[Bob Owen (:bobowen)]

(In reply to Michael Layzell [:mystor] from comment #14)
> (In reply to Andrea Marchesini [:baku] from comment #13)
> > Right, I can use instanceof Ci.nsIInputStream but only for about:about URL.
> 
> Ci.nsIInputStream is avaliable even for non-privileged URLs. ContentTasks
> are always run as privileged JS with access to Components.
> 
> > So, when this code is in m-i/m-c, we can proceed with the rest of the bug.
> > It's important to know if sessionStore and the rest of the browser UI is
> > able to work with non-blocking nsIInputStream.
> > Can you maybe test it creating a big postData (big means more than 1mb) and
> > see if everything works correctly?
> 
> I'll ni? :bobowen who was working on the other patch

I don't have a specific test like that, but there's an obsoleted test patch on bug 1344465 that you could possibly use as a starting point to create one.

I'm getting more concerned about compatibility issues when directly linked browsing contexts aren't in the same process, so I'm probably going to flip the pref anyway to fix the POST from file->http(s) issue.

Chrome seems to keep it in the same process even if you type a new URL as long as it is same origin as the initially linked document. It maintains these relationships across history navigation as well.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb56d0860cb6
Make nsInputStream structuredCloneable, r=mystor

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb56d0860cb6

Comment 18

[Andrea Marchesini [:baku]]

Attachment: stream.patch

Comment 19

[:Gijs (he/him)]

Comment on attachment 8868460 [details] [diff] [review]
stream.patch

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

Are there already tests for these codepaths that pass 'real' inputstreams?

::: browser/base/content/browser.js
@@ +1019,5 @@
>          return;
>        }
>  
> +      if (postData && !(postData instanceof Ci.nsIInputStream)) {
> +        throw new Error("_loadURIWithFlags cannot accept a non nsIInputStream postData.");

How sure are you about there not being consumers that do this?

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:baku, maybe it's time to close this bug?

Comment 21

[Andrea Marchesini [:baku]]

Yes, let's close this bug. If there is something else to work on, we can file new bugs.