1440771 - Make it more ergonomic to serialize nsIInputStreams over IPC

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P1)

Description

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

Currently you have to make an AutoIPCStream, serialize the nsIInputStream into it, and then send it over IPC.

With the changes in bug 1440511, it's now possible to write a ParamTraits implementation for nsCOMPtr<nsIInputStream>, which should make it much nicer to use. These patches implement that and add some features.

Comment 1

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

Attachment: Part 1: Add a ParamTraits impl for nsCOMPtr<nsIInputStream>

MozReview-Commit-ID: dfZJH1cWnW

Comment 2

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

Attachment: Part 2: Use nsCOMPtr<nsIInputStream> directly in PCacheStreamControl

MozReview-Commit-ID: 1Ab49lZtxTI

Comment 3

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

Attachment: Part 3: Use nsCOMPtr<nsIInputStream> directly in PContent

MozReview-Commit-ID: 32gMROoF1qU

Comment 4

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

Attachment: Part 4: Use nsCOMPtr<nsIInputStream> directly in PNecko

MozReview-Commit-ID: Ifw5M5KhyfS

Comment 5

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1603c515928360b5ba25c15e164d631b1116600a

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Can you add some comments somewhere describing when you would use AutoIPCStream vs this support now?

(I think its cases where you might have a failure between setting the stream on a value and actually sending it across the wire.)

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8953604 [details] [diff] [review]
Part 1: Add a ParamTraits impl for nsCOMPtr<nsIInputStream>

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

::: ipc/glue/IPCStreamUtils.cpp
@@ +682,5 @@
> +  WriteParam(aMsg, autoStream.TakeOptionalValue());
> +}
> +
> +bool
> +ParamTraits<nsCOMPtr<nsIInputStream>>::Read(const Message* aMsg, PickleIterator* aIter, nsCOMPtr<nsIInputStream>* aResult)

80chars max per line.

Comment 8

[Jim Mathies [:jimm]]

p1 -> owned, active

Comment 9

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

Attachment: Part 1: Add a ParamTraits impl for nsCOMPtr<nsIInputStream>

MozReview-Commit-ID: dfZJH1cWnW

Comment 10

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

Attachment: Part 2: Use nsCOMPtr<nsIInputStream> directly in PCacheStreamControl

MozReview-Commit-ID: 1Ab49lZtxTI

Comment 11

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

Attachment: Part 3: Use nsCOMPtr<nsIInputStream> directly in PContent

MozReview-Commit-ID: 32gMROoF1qU

Comment 12

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

Attachment: Part 4: Use nsCOMPtr<nsIInputStream> directly in PNecko

MozReview-Commit-ID: COnMiadbCjn

Comment 13

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

These patches effectively assert that the AutoIPCStream::Serialize call didn't fail. It would be nice to be able to use the improved ergonomics of the new API in more places, such as in the implementation of serialization for blobs and other data types, however if these failures are meaningful we can't really do that.

I found only 2 places where a failure actually could become reported, which are, I believe, unreasonable to handle.

One is when the internal buffer is initialized twice, which it has no chance to be: https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/xpcom/io/nsSegmentedBuffer.cpp#14. 

And the other is if dispatching events to the StreamTransportService fails, which I believe should only happen very late during shutdown (xpcom-shutdown-threads). At that point I believe all actors are probably already dead. (The nsThreadPool can also produce an error, but again it can only occur late during shutdown).

I'm inclined to accept that these Serialize methods will never fail, and switch as many consumers as possible over to the new system. We could potentially remove AutoIPCStream and instead just perform the necessary work directly inside of the IPDLParamTraits implementation.

This would also let us serialize other important types, such as blobs, directly over IPC.

baku, what are your opinions on this idea?

Comment 14

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

A related question to the above is what we want to do with other types which can be serialized over IPC, such as BlobImpls. Should we have it crash? Send some sort of dummy object to represent the invalid BlobImpl?

For example, I want to be able to hold a blob in an object I want to send over IPC, and I don't think I want it to crash the browser if serializing that object doesn't work, and I think what I'd just do is basically throw out the blob if I can't send it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d506f9eb0db6
Part 1: Add a ParamTraits impl for nsCOMPtr<nsIInputStream>, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/945995a0a8db
Part 2: Use nsCOMPtr<nsIInputStream> directly in PCacheStreamControl, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/02049d16a352
Part 3: Use nsCOMPtr<nsIInputStream> directly in PContent, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd28092ce59a
Part 4: Use nsCOMPtr<nsIInputStream> directly in PNecko, r=baku

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/d506f9eb0db6
https://hg.mozilla.org/mozilla-central/rev/945995a0a8db
https://hg.mozilla.org/mozilla-central/rev/02049d16a352
https://hg.mozilla.org/mozilla-central/rev/cd28092ce59a

Comment 17

[Andrea Marchesini [:baku]]

> This would also let us serialize other important types, such as blobs,
> directly over IPC.

Sorry for the delay. Actually you already implemented part of this.
Would be really nice to support a fail error propagation because is kind-of needed for the fuzzy project where we try to break things passing invalid sequence of bytes from child to parent process.