Closed Bug 566305 Opened 15 years ago Closed 15 years ago

e10s HTTP: Serialize nsIStringInputStream

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: lusian, Assigned: lusian)

References

Details

Attachments

(3 files, 6 obsolete files)

Attached patch wip (obsolete) — Splinter Review
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.
Attached patch patch, 0 (obsolete) — Splinter Review
You can't assume that QI will succeed.
Attached patch patch, 1 (obsolete) — Splinter Review
Attachment #445918 - Attachment is obsolete: true
Attachment #445948 - Flags: review?
So Write really can't be fallible here?
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 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 on attachment 445674 [details] [diff] [review] wip Guessing you don't want review here yet, right?
(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).
Attached patch patch, 2 (obsolete) — Splinter Review
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.
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+
Attached patch patch, 3 (obsolete) — Splinter Review
Attachment #446475 - Attachment is obsolete: true
Attachment #447483 - Flags: review?
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+
Attached patch patch, 4 (obsolete) — Splinter Review
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?
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-
Attached patch test I didSplinter Review
I am sorry for not testing. This test depends on Bug 536273. I saw 0123456789 & its length sent/received successfully.
Attached patch patch, 5Splinter Review
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+
Push to the e10s please.
Attached patch testSplinter Review
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: