Closed
Bug 1440771
Opened 6 years ago
Closed 6 years ago
Make it more ergonomic to serialize nsIInputStreams over IPC
Categories
(Core :: IPC, enhancement, P1)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
Details
Attachments
(4 files, 4 obsolete files)
4.95 KB,
patch
|
Details | Diff | Splinter Review | |
3.54 KB,
patch
|
Details | Diff | Splinter Review | |
6.70 KB,
patch
|
Details | Diff | Splinter Review | |
4.75 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
MozReview-Commit-ID: dfZJH1cWnW
Attachment #8953604 -
Flags: review?(amarchesini)
Assignee | ||
Comment 2•6 years ago
|
||
MozReview-Commit-ID: 1Ab49lZtxTI
Attachment #8953605 -
Flags: review?(amarchesini)
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: 32gMROoF1qU
Attachment #8953606 -
Flags: review?(amarchesini)
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: Ifw5M5KhyfS
Attachment #8953607 -
Flags: review?(amarchesini)
Assignee | ||
Comment 5•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1603c515928360b5ba25c15e164d631b1116600a
Comment 6•6 years ago
|
||
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•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8953605 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8953606 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8953607 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 9•6 years ago
|
||
MozReview-Commit-ID: dfZJH1cWnW
Assignee | ||
Updated•6 years ago
|
Attachment #8953604 -
Attachment is obsolete: true
Attachment #8953605 -
Attachment is obsolete: true
Attachment #8953606 -
Attachment is obsolete: true
Attachment #8953607 -
Attachment is obsolete: true
Assignee | ||
Comment 10•6 years ago
|
||
MozReview-Commit-ID: 1Ab49lZtxTI
Assignee | ||
Comment 11•6 years ago
|
||
MozReview-Commit-ID: 32gMROoF1qU
Assignee | ||
Comment 12•6 years ago
|
||
MozReview-Commit-ID: COnMiadbCjn
Assignee | ||
Comment 13•6 years ago
|
||
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)
Assignee | ||
Comment 14•6 years ago
|
||
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•6 years ago
|
||
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
Comment 16•6 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 17•6 years ago
|
||
> 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.
Description
•