Make it more ergonomic to serialize nsIInputStreams over IPC

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Nika, Assigned: Nika)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

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.
MozReview-Commit-ID: dfZJH1cWnW
Attachment #8953604 - Flags: review?(amarchesini)
MozReview-Commit-ID: 1Ab49lZtxTI
Attachment #8953605 - Flags: review?(amarchesini)
MozReview-Commit-ID: 32gMROoF1qU
Attachment #8953606 - Flags: review?(amarchesini)
MozReview-Commit-ID: Ifw5M5KhyfS
Attachment #8953607 - Flags: review?(amarchesini)
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 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.
Attachment #8953604 - Flags: review?(amarchesini) → review+
Attachment #8953605 - Flags: review?(amarchesini) → review+
Attachment #8953606 - Flags: review?(amarchesini) → review+
Attachment #8953607 - Flags: review?(amarchesini) → review+
p1 -> owned, active
Priority: -- → P1
Attachment #8953604 - Attachment is obsolete: true
Attachment #8953605 - Attachment is obsolete: true
Attachment #8953606 - Attachment is obsolete: true
Attachment #8953607 - Attachment is obsolete: true
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?
Flags: needinfo?(amarchesini)
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.
Pushed by nika@thelayzells.com:
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
> 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.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.