1359172 - RemoteInputStream doesn't create a correct inputStream when used in a separate process

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Andrea Marchesini [:baku]]

When RemoteBlobImpl::GetInternalStream is called from a non-owning thread, we create a PBlobChild actor on PBackground. In case this RemoteBlobImpl is a slice of another one, we create such PBlobChild actor of the parent blobImpl, but then, we return the inputStream of the parent, instead a slice of it.
This code has been buggy for a while, but it appears now because we are moving away from PBlob and now it's easier to recreate this scenario than before.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: fix.patch

Comment 2

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

Why blobImpl::GetInternalStream doesn't do the wrapping? Why this caller needs to know to use sliced stream?
The comment doesn't explain why sliced stream is needed.


s/strem/stream/

Comment 3

[Andrea Marchesini [:baku]]

Attachment: fix.patch

Better comments.

Comment 4

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

I'm still missing the reasoning.
_why_ we need to slice the input stream?
Why 'PBackgroundChild* manager = mozilla::ipc::BackgroundChild::GetForCurrentThread()' is so special case that in that case we need to slice the input stream?

Sorry, I'm lazy here to read all the relevant code to understand this.

Comment 5

[Andrea Marchesini [:baku]]

> I'm still missing the reasoning.
> _why_ we need to slice the input stream?
> Why 'PBackgroundChild* manager =
> mozilla::ipc::BackgroundChild::GetForCurrentThread()' is so special case
> that in that case we need to slice the input stream?

This is actually written in the comments:

+    // In case we are on a PBackground thread and this is not the owning thread,
+    // we need to create a new actor here. This actor must be created for the
+    // baseRemoteBlobImpl, which can be the current blobImpl or the parent one,
+    // in case we are dealing with a sliced blob.

...

+    // InputStream is the stream of the baseRemoteBlobImpl. If the current
+    // blobImpl is a slice of this baseRemoteBlobImpl, here we need to slice the
+    // inputStream.

Comment 6

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

(In reply to Andrea Marchesini [:baku] from comment #5)
> > I'm still missing the reasoning.
> > _why_ we need to slice the input stream?
> > Why 'PBackgroundChild* manager =
> > mozilla::ipc::BackgroundChild::GetForCurrentThread()' is so special case
> > that in that case we need to slice the input stream?
> 
> This is actually written in the comments:
> 
> +    // In case we are on a PBackground thread and this is not the owning
> thread,
> +    // we need to create a new actor here. This actor must be created for
> the
> +    // baseRemoteBlobImpl, which can be the current blobImpl or the parent
> one,
> +    // in case we are dealing with a sliced blob.
> 
This doesn't explain my question at all


> ...
> 
> +    // InputStream is the stream of the baseRemoteBlobImpl. If the current
> +    // blobImpl is a slice of this baseRemoteBlobImpl, here we need to
> slice the
> +    // inputStream.
This doesn't still explain why we need sliced stream

Comment 7

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

Comment on attachment 8861189 [details] [diff] [review]
fix.patch

>+    // InputStream is the stream of the baseRemoteBlobImpl. If the current
>+    // blobImpl is a slice of this baseRemoteBlobImpl, here we need to slice the
>+    // inputStream.
>+    if (mRemoteBlobImpl->IsSlice()) {
The comment is still very confusing. The comment talks about blobImpl being slice, but then code doesn't use
blobImpl at all, but mRemoteBlobImpl

Please clarify

Comment 8

[Andrea Marchesini [:baku]]

Attachment: fix.patch

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6065f64f9a
RemoteInputStream must create a correct inputStream when used and sliced in a separate process, r=smaug

Comment 10

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

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