Closed
Bug 566305
Opened 15 years ago
Closed 15 years ago
e10s HTTP: Serialize nsIStringInputStream
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
People
(Reporter: lusian, Assigned: lusian)
References
Details
Attachments
(3 files, 6 obsolete files)
11.32 KB,
patch
|
Details | Diff | Splinter Review | |
2.14 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | 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)
![]() |
||
Comment 1•15 years ago
|
||
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•15 years ago
|
||
![]() |
||
Comment 3•15 years ago
|
||
You can't assume that QI will succeed.
Assignee | ||
Comment 4•15 years ago
|
||
Attachment #445918 -
Attachment is obsolete: true
Attachment #445948 -
Flags: review?
![]() |
||
Comment 5•15 years ago
|
||
So Write really can't be fallible here?
Assignee | ||
Comment 6•15 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•15 years ago
|
||
Comment on attachment 445948 [details] [diff] [review]
patch, 1
Not review-ready.
Attachment #445948 -
Flags: review?
![]() |
||
Comment 8•15 years ago
|
||
nsStringInputStream is the only in-tree implementation of nsIStringInputStream. Nothing stops extensions, say, from creating other implementations.
Comment 9•15 years ago
|
||
Comment on attachment 445674 [details] [diff] [review]
wip
Guessing you don't want review here yet, right?
Assignee | ||
Comment 10•15 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.
Comment 11•15 years ago
|
||
>> 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•15 years ago
|
||
Attachment #445674 -
Attachment is obsolete: true
Attachment #445948 -
Attachment is obsolete: true
Attachment #445674 -
Flags: feedback?(jduell.mcbugs)
Attachment #445674 -
Flags: feedback?(dwitte)
![]() |
||
Comment 13•15 years ago
|
||
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•15 years ago
|
Attachment #446475 -
Flags: review?(bzbarsky)
![]() |
||
Comment 14•15 years ago
|
||
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•15 years ago
|
||
Attachment #446475 -
Attachment is obsolete: true
Attachment #447483 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #447483 -
Flags: review? → review?(bzbarsky)
![]() |
||
Comment 16•15 years ago
|
||
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•15 years ago
|
||
Attachment #447483 -
Attachment is obsolete: true
Attachment #447957 -
Flags: review?(bzbarsky)
![]() |
||
Comment 18•15 years ago
|
||
Hold on. What's changed from patch 3 to patch 4? Why were the changes made?
Assignee | ||
Comment 19•15 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 20•15 years ago
|
||
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•15 years ago
|
||
I am sorry for not testing.
This test depends on Bug 536273. I saw 0123456789 & its length sent/received successfully.
Assignee | ||
Comment 22•15 years ago
|
||
Attachment #447957 -
Attachment is obsolete: true
Attachment #448066 -
Flags: review?(bzbarsky)
![]() |
||
Comment 23•15 years ago
|
||
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•15 years ago
|
||
Push to the e10s please.
Assignee | ||
Comment 25•15 years ago
|
||
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 26•15 years ago
|
||
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+
![]() |
||
Comment 28•15 years ago
|
||
Er, pushed the fix as http://hg.mozilla.org/projects/electrolysis//rev/0292f7330100
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
![]() |
||
Updated•15 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•