e10s HTTP: Serialize nsIStringInputStream

RESOLVED FIXED

Status

()

Core
Networking: HTTP
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Jae-Seong Lee-Russo, Assigned: Jae-Seong Lee-Russo)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 445674 [details] [diff] [review]
wip

Am I going to the right direction?  There is a better way to get data of nsIStringInputStream, right?
Attachment #445674 - Flags: feedback?(jduell.mcbugs)
Attachment #445674 - Flags: feedback?(dwitte)
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.
(Assignee)

Comment 2

8 years ago
Created attachment 445918 [details] [diff] [review]
patch, 0
You can't assume that QI will succeed.
(Assignee)

Comment 4

8 years ago
Created attachment 445948 [details] [diff] [review]
patch, 1
Attachment #445918 - Attachment is obsolete: true
Attachment #445948 - Flags: review?
So Write really can't be fallible here?
(Assignee)

Comment 6

8 years ago
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?
(Assignee)

Comment 7

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

Not review-ready.
Attachment #445948 - Flags: review?
nsStringInputStream is the only in-tree implementation of nsIStringInputStream.  Nothing stops extensions, say, from creating other implementations.

Comment 9

8 years ago
Comment on attachment 445674 [details] [diff] [review]
wip

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

Comment 10

8 years ago
(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.
>> 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).
(Assignee)

Comment 12

8 years ago
Created attachment 446475 [details] [diff] [review]
patch, 2
Attachment #445674 - Attachment is obsolete: true
Attachment #445948 - Attachment is obsolete: true
Attachment #445674 - Flags: feedback?(jduell.mcbugs)
Attachment #445674 - Flags: feedback?(dwitte)
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.
(Assignee)

Updated

8 years ago
Attachment #446475 - Flags: review?(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*.
Attachment #446475 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

8 years ago
Created attachment 447483 [details] [diff] [review]
patch, 3
Attachment #446475 - Attachment is obsolete: true
Attachment #447483 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #447483 - Flags: review? → review?(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?
Attachment #447483 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

8 years ago
Created attachment 447957 [details] [diff] [review]
patch, 4
Attachment #447483 - Attachment is obsolete: true
Attachment #447957 - Flags: review?(bzbarsky)
Hold on.  What's changed from patch 3 to patch 4?  Why were the changes made?
(Assignee)

Comment 19

8 years ago
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 on attachment 447957 [details] [diff] [review]
patch, 4

OK.  I just realized that your Read() never writes to aResult...  Was this tested at all?
Attachment #447957 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 21

8 years ago
Created attachment 448065 [details] [diff] [review]
test I did

I am sorry for not testing.

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

Comment 22

8 years ago
Created attachment 448066 [details] [diff] [review]
patch, 5
Attachment #447957 - Attachment is obsolete: true
Attachment #448066 - Flags: review?(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?
Attachment #448066 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 24

8 years ago
Push to the e10s please.
(Assignee)

Comment 25

8 years ago
Created attachment 448358 [details] [diff] [review]
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?
Attachment #448358 - Flags: review?(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.
Attachment #448358 - Flags: review?(bzbarsky) → review+
Pushed the fix as
Assignee: nobody → lusian
Er, pushed the fix as http://hg.mozilla.org/projects/electrolysis//rev/0292f7330100
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.