566305 - e10s HTTP: Serialize nsIStringInputStream

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jae-Seong Lee-Russo]

Attachment: wip

Am I going to the right direction?  There is a better way to get data of nsIStringInputStream, right?

Comment 1

[Boris Zbarsky [:bzbarsky]]

That cast isn't safe.

What might be safe is QI to nsISupportsCString and then calling GetData.  It'll fail if the impl doesn't support that, but then you'll get an exception from QI instead of reading random memory.

Comment 2

[Jae-Seong Lee-Russo]

Attachment: patch, 0

Comment 3

[Boris Zbarsky [:bzbarsky]]

You can't assume that QI will succeed.

Comment 4

[Jae-Seong Lee-Russo]

Attachment: patch, 1

Comment 5

[Boris Zbarsky [:bzbarsky]]

So Write really can't be fallible here?

Comment 6

[Jae-Seong Lee-Russo]

nsStringInputStream is the only one that implements nsIStringInputStream, and nsStringInputStream implements nsISupportsCString, too.  GetData needs to handle NS_BASE_STREAM_CLOSED?  I am not quite sure what is missing, can you explain?

Comment 7

[Jae-Seong Lee-Russo]

Comment on attachment 445948 [details] [diff] [review]
patch, 1

Not review-ready.

Comment 8

[Boris Zbarsky [:bzbarsky]]

nsStringInputStream is the only in-tree implementation of nsIStringInputStream.  Nothing stops extensions, say, from creating other implementations.

Comment 9

[[e-mail]]

Comment on attachment 445674 [details] [diff] [review]
wip

Guessing you don't want review here yet, right?

Comment 10

[Jae-Seong Lee-Russo]

(In reply to comment #8)
> nsStringInputStream is the only in-tree implementation of nsIStringInputStream.
>  Nothing stops extensions, say, from creating other implementations.

What should I do besides printing an error message?  Can I add GetData to nsIStringInputStream.idl?

(In reply to comment #9)
> (From update of attachment 445674 [details] [diff] [review])
> Guessing you don't want review here yet, right?

You are right.

Comment 11

[Jason Duell]

>> nsStringInputStream is the only in-tree implementation of nsIStringInputStream.
>>  Nothing stops extensions, say, from creating other implementations.
>
> What should I do besides printing an error message?  Can I add GetData 
> to nsIStringInputStream.idl?

Using GetData is just an optimization, right?   So if we get some user-created nsIStringInputStream that doesn't support nsISupportsCString, you can just Read() the stream into a string instead of using GetData.  As per the discussion in bug 536273, for the types of streams POST and PUT get (no pun intended: GET it? groan :) it appears to be "safe" to assume we can just read Available() count bytes from the stream to get the entire contents, so just do that.  To prepare the string for the read, use SetLength and BeginWriting, as done in HttpChannelParent::OnDataAvailable().  (Except that we don't appear to need EndWriting(), and we *do* need to check if SetLength succeeded, by seeing if GetLength() == the length we tried to set.  I need to file a bug on that.  The docs for our String classes suck). 

By the way, printing an error message in ParamTraits is pretty much useless. If we return false, Bad Things will happen (like the content process dying), not just log warnings.  So if we needed to not support a particular stream implementation, for instance, we'd need to catch that in the HttpChannelChild logic before ParamTraits::Write gets invoked, so we can fail gracefully. I don't think we'll need that here.  We *will* return false if SetLength on the string fails, though (out of memory: and it's not caught automagically, 'cause the string classes don't use infallible malloc yet).

Comment 12

[Jae-Seong Lee-Russo]

Attachment: patch, 2

Comment 13

[Boris Zbarsky [:bzbarsky]]

Might be worth filing a followup bug to make stringstream be able to accept a stringbuffer so on the Read there we could just share the buffer, by the way.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 446475 [details] [diff] [review]
patch, 2

This looks ok assuming we really want the template methods to take  nsIStringInputStream and not on nsIStringInputStream*.

Comment 15

[Jae-Seong Lee-Russo]

Attachment: patch, 3

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 447483 [details] [diff] [review]
patch, 3

This looks good as long as you don't plan to ever seek the stream on the sending side after sending.  Is that the case?

Comment 17

[Jae-Seong Lee-Russo]

Attachment: patch, 4

Comment 18

[Boris Zbarsky [:bzbarsky]]

Hold on.  What's changed from patch 3 to patch 4?  Why were the changes made?

Comment 19

[Jae-Seong Lee-Russo]

I removed the Close statement so that the sender can seek the stream even after sending.  I also added the check for the null pointer.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 447957 [details] [diff] [review]
patch, 4

OK.  I just realized that your Read() never writes to aResult...  Was this tested at all?

Comment 21

[Jae-Seong Lee-Russo]

Attachment: test I did

I am sorry for not testing.

This test depends on Bug 536273.  I saw 0123456789 & its length sent/received successfully.

Comment 22

[Jae-Seong Lee-Russo]

Attachment: patch, 5

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 448066 [details] [diff] [review]
patch, 5

Looks good.  Do you need me to push this (to the e10s tree, right?), or are you all set?

Comment 24

[Jae-Seong Lee-Russo]

Push to the e10s please.

Comment 25

[Jae-Seong Lee-Russo]

Attachment: test

In the future, I want to change this to have an MultiplexInputStream, which contains a StringInputStream and a FileInputStream.

netwerk/test/unit_ipc tests fail on windows 7.  /unit tests are fine.  "Using observer service off the main thread!" (http://mxr.mozilla.org/projects-central/source/electrolysis/xpcom/ds/nsObserverService.cpp#128).  The tests run fine on Linux.  Is this a known problem?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 448358 [details] [diff] [review]
test

This seems fine, though I'd prefer that setupChannel just return the QI result directly if nsIHttpChannel is needed at all there.

Not pushing this for now pending you sorting out the test failure.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Pushed the fix as

Comment 28

[Boris Zbarsky [:bzbarsky]]

Er, pushed the fix as http://hg.mozilla.org/projects/electrolysis//rev/0292f7330100