Last Comment Bug 708415 - Review SPDY protocol patches
: Review SPDY protocol patches
Status: RESOLVED FIXED
[spdy]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: SPDY
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-07 14:21 PST by Honza Bambas (:mayhemer)
Modified: 2012-01-27 15:13 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
All patches landed for bug 528288 merged to a single one [for reference only] (240.97 KB, patch)
2011-12-07 14:21 PST, Honza Bambas (:mayhemer)
honzab.moz: review-
Details | Diff | Splinter Review
All patches as re-landed after 3GB PGO build linker memory overflow [for reference only] (251.20 KB, patch)
2011-12-17 07:13 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
spdystream 0 (25.14 KB, patch)
2012-01-17 14:04 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
spdy session review comment updates v0 (79.02 KB, patch)
2012-01-19 10:38 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
frame writes should be atomic v0 (33.66 KB, patch)
2012-01-19 10:40 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
review comment updates for nshttp* pieces v0 (26.63 KB, patch)
2012-01-19 14:52 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
spdy stream 1 (25.18 KB, patch)
2012-01-24 14:54 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
spdy session review comment updates v1 (82.04 KB, patch)
2012-01-24 14:55 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
frame writes should be atomic v1 (33.70 KB, patch)
2012-01-24 14:56 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
review comment updats for nshttp* pieces v1 (28.45 KB, patch)
2012-01-24 14:57 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2011-12-07 14:21:07 PST
Created attachment 579835 [details] [diff] [review]
All patches landed for bug 528288 merged to a single one [for reference only]

SPDY has landed reviewed only with respect to spdy.enabled = false.  Turning the pref on means to use a highly experimental code that haven't even be reviewed according the standard procedures.

That approach had been chosen to speed up landing the feature on the tree and let brave ones out there test it.

Purpose of this bug is to report followups on:
- first, all immediate issues, if found, best fixed before next Aurora merge on Dec 20th
- second, do all the best to make the code able to be pref'ed on by default
- then, think of some tweaks on the architectural level about any obvious needs for code clean up, if necessary (I've seen some places that could be potentially improved)
Comment 1 Honza Bambas (:mayhemer) 2011-12-17 07:13:26 PST
Created attachment 582529 [details] [diff] [review]
All patches as re-landed after 3GB PGO build linker memory overflow [for reference only]

This patch includes the following bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=706236
https://bugzilla.mozilla.org/show_bug.cgi?id=707173
https://bugzilla.mozilla.org/show_bug.cgi?id=707662
https://bugzilla.mozilla.org/show_bug.cgi?id=708305
Comment 2 Honza Bambas (:mayhemer) 2011-12-17 07:16:06 PST
Additional bugs to review:
https://bugzilla.mozilla.org/show_bug.cgi?id=710310
Comment 3 Honza Bambas (:mayhemer) 2012-01-16 15:34:34 PST
Comment on attachment 579835 [details] [diff] [review]
All patches landed for bug 528288 merged to a single one [for reference only]

Review of attachment 579835 [details] [diff] [review]:
-----------------------------------------------------------------

I wanted to set some flag (my first time I do a postmortem review) here to indicate the work has been done, it is more - then +.  There is some work that should be done here.

::: netwerk/protocol/http/SpdySession.cpp
@@ +105,5 @@
> +  mOutputQueueBuffer = new char[mOutputQueueSize];
> +  zlibInit();
> +  
> +  mSendingChunkSize =
> +    Preferences::GetInt("network.http.spdy.chunk-size", kSendingChunkSize);

This is called off the main thread.  This API isn't thread safe AFAIK.  This should go the usual way through the httpHandler.

@@ +110,5 @@
> +  AddStream(aHttpTransaction, firstPriority);
> +}
> +
> +PLDHashOperator
> +SpdySession::Shutdown(nsAHttpTransaction *key,

Call this ShutdownEnumerator.  The name is misleading.

@@ +117,5 @@
> +{
> +  SpdySession *self = static_cast<SpdySession *>(closure);
> +  
> +  if (self->mCleanShutdown &&
> +      self->mGoAwayID < stream->StreamID())

Maybe add a comment what this condition means.  That mGoAwayID is id of the last stream the peer has seen, all other will be ignored.

And having the condition backwards (stream->StreamID() > self->mGoAwayID) would be more illustrative.

@@ +176,5 @@
> +    LOG4(("%s", linebuf));
> +  }
> +}
> +
> +typedef nsresult  (*Control_FX) (SpdySession *self);

I'm really against calling a method by indexing array using something we have read from the network even though it has been sanitized.  This may open a risk for security bugs, IMO.

I'd rather see handling the control code as a switch.

@@ +266,5 @@
> +}
> +
> +void
> +SpdySession::ActivateStream(SpdyStream *stream)
> +{

Should add NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");

@@ +284,5 @@
> +}
> +
> +void
> +SpdySession::ProcessPending()
> +{

Should add NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");

@@ +296,5 @@
> +  }
> +}
> +
> +void
> +SpdySession::SetWriteCallbacks(nsAHttpTransaction *aTrans)

This method's name isn't quit telling me what it does.  I suggest to rename it to "[Conditionally]StartAsyncWrite" or ConditionallyResumeSend or so.

On one place in this code you call this with NULL as an arg.  Is it really necessary for this method to have an arg at all?  This always ends up in nsHttpConnection::ResumeSend that doesn't use this arg.

Actually, some solution to NOT HAVE an argument to ResumeSend would be nice.  Probably letting a stream wrap its session and let transaction communicate through the stream.  Not sure how much work that would be.  For any future consumers it may not be always possible to pass the calling transaction through.  Even you in this file sometimes have to pass just null.

@@ +345,5 @@
> +void
> +SpdySession::DontReuse()
> +{
> +  mShouldGoAway = true;
> +  if(!mStreamTransactionHash.Count())

if (!

@@ +350,5 @@
> +    Close(NS_OK);
> +}
> +
> +PRUint32
> +SpdySession::WriteQueueSize()

GetWriteQueueSize() please.

@@ +368,5 @@
> +  LOG3(("SpdyStream::ChangeDownstreamState() %p from %X to %X",
> +        this, mDownstreamState, newState));
> +  mDownstreamState = newState;
> +
> +  if (mDownstreamState == BUFFERING_FRAME_HEADER) {

I feel a bit strange about this.  I'd rather see a separate method to clean the current data stream manually.  Or have a method RestartState that reverts to BUFFERING_FRAME_HEADER and cleans the frame.

@@ +370,5 @@
> +  mDownstreamState = newState;
> +
> +  if (mDownstreamState == BUFFERING_FRAME_HEADER) {
> +    if (mFrameDataLast && mFrameDataStream) {
> +      mFrameDataLast = 0;

= false;

@@ +395,5 @@
> +    return;
> +  
> +  objSize = newSize;
> +  nsAutoArrayPtr<char> tmp(new char[objSize]);
> +  memcpy (tmp, buf, preserve);

memcpy(tmp, buf, preserve);  You have mem* ( on other places, please check them all.

My first encounter of call of this method was realloc from 16000 to 16012.  Quit wasting I'd say.  You should enlarge the newSize up to some meaningful value.  I think there is some heuristic in nsTArray to grow the buffer up.  You may check it and use the grow logic here.  I think you should grow by at least the current size of the buffer till some reasonable threshold of course to prevent too many reallocations and heap fragmentation.


Also, I would like you to encapsulate your buffer logic to a class/struct.  As I understand you have 4 buffers that share most of the logic (or could be made to share).  It is very hard to track and check all places you are doing all buffer operations at.  Raises potential of a well hidden security issue.  Take this as a review request.

@@ +483,5 @@
> +    if (lastHeaderByte < nvpair + 2 + nameLen)
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    nsDependentCSubstring nameString =
> +      Substring (reinterpret_cast<const char *>(nvpair) + 2,
> +                 reinterpret_cast<const char *>(nvpair) + 2 + nameLen);

Space: Substring (

@@ +502,5 @@
> +nsresult
> +SpdySession::ConvertHeaders(nsDependentCSubstring &status,
> +                            nsDependentCSubstring &version)
> +{
> +

No blank line

@@ +544,5 @@
> +
> +    // a null in the name string is particularly wrong because it will
> +    // break the fix-up-nulls-in-value-string algorithm.
> +    if (nameString.FindChar(0) != -1)
> +      return NS_ERROR_ILLEGAL_VALUE;

You may move this check bellow to the check for upper case letters to not walk the name twice.  I'm not sure the iterator will not stop on nul, worth to check.

@@ +550,5 @@
> +    if (lastHeaderByte < nvpair + 4 + nameLen)
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    PRUint16 valueLen = (nvpair[2 + nameLen] << 8) + nvpair[3 + nameLen];
> +    if (lastHeaderByte < nvpair + 4 + nameLen + valueLen)
> +      return NS_ERROR_ILLEGAL_VALUE;

You should first do sanity check on the name prior to reading the value length.  Also because these checks should be just before reading the value for better code readability.

@@ +601,5 @@
> +        valueString.Replace(valueIndex, 1, replacement);
> +      }
> +
> +      mFlatHTTPResponseHeaders.Append(valueString);
> +      mFlatHTTPResponseHeaders.Append(NS_LITERAL_CSTRING("\r\n"));

I think it would be better to append the parts separated with '\0' directly to the flat headers string.  Replace involves lot of mem moves while we grow the buffer.  When I was checking whether this code worked well, the first encounter was cookies of some 1kB length split to some 4 or 5 bits.  So, we were copying the tail 3 or 4 times to get the final string that had again been copied.

@@ +702,5 @@
> +  nsresult abortCode = NS_OK;
> +
> +  if (!aStream->RecvdFin() && aStream->StreamID()) {
> +    LOG3(("Stream had not processed recv FIN, sending RST"));
> +    GenerateRstStream(RST_CANCEL, aStream->StreamID());

Are you sure this is the proper code to send?  Is it even necessary to send a RST in this case?

@@ +718,5 @@
> +    mPartialFrame = nsnull;
> +  }
> +  
> +  // Check if partial frame reader
> +  if (aStream == mFrameDataStream) {

Please be consistent on the order: (mFrameDataStream == aStream).

@@ +725,5 @@
> +    mFrameDataStream = nsnull;
> +  }
> +
> +  // check the streams blocked on write, this is linear but the list
> +  // should be pretty short.

Not sure this comment is correct.

@@ +905,5 @@
> +    PR_ntohl(reinterpret_cast<PRUint32 *>(self->mFrameBuffer.get())[2]);
> +
> +  self->mDownstreamRstReason =
> +    PR_ntohl(reinterpret_cast<PRUint32 *>(self->mFrameBuffer.get())[3]);
> +

Also check the flags == 0.

@@ +923,5 @@
> +          "0x%X failed", self, streamID));
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +    
> +  self->ChangeDownstreamState(PROCESSING_CONTROL_RST_STREAM);

I don't much understand why you need a state to handle this kind of packet.

Actually, what you need to do is to cleanup the stream with given streamID and immediately stop sending any more data on the stream.  But with what you have written you have to wait for reading from the socket first before you actually reset the stream in WriteSegments, right?  It may take a while before it gets called again..

@@ +956,5 @@
> +        self, numEntries));
> +
> +  for (PRUint32 index = 0; index < numEntries; ++index) {
> +    // To clarify the v2 spec:
> +    // Each entry is a 24 bits of a little endian id

Are you sure about little endian?  The comment says the spec might be wrong, can you confirm this?  Was this communicated to Google?

@@ +1047,5 @@
> +  LOG3(("SpdySession::HandlePing %p PING ID 0x%X.", self, pingID));
> +
> +  if (pingID & 0x01) {
> +    // We never expect to see an odd PING beacuse we never generate PING.
> +      // The spec mandates ignoring this

Indention of the comment.

@@ +1078,5 @@
> +  self->mCleanShutdown = true;
> +  
> +  LOG3(("SpdySession::HandleGoAway %p GOAWAY Last-Good-ID 0x%X.",
> +        self, self->mGoAwayID));
> +  self->ResumeRecv(self);

Resuming because want to catch the socket close after go away?  Please add a comment why you resume here.

@@ +1130,5 @@
> +  PRUint64 progress;
> +};
> +
> +static PLDHashOperator
> +StreamTransportStatus(nsAHttpTransaction *key,

StreamTransportStatusEnumerator please

@@ +1221,5 @@
> +  if (reader)
> +    mSegmentReader = reader;
> +  rv = stream->ReadSegments(this, count, countRead);
> +
> +  FlushOutputQueue();

Please add a comment why isn't this flush redundant.

@@ +1249,5 @@
> +
> +    // call readsegments again if there are other streams ready
> +    // to run in this session
> +    if (WriteQueueSize())
> +      rv = NS_OK;

Here countRead may be 0 and this way you tell the connection that all has been sent out (rv == NS_OK, n == 0) and it then sends WAITING_FOR status and switches to wait for read (not that it would be necessarily wrong, though).

Correct should be to return WOULD_BLOCK in both cases IMO.

@@ +1252,5 @@
> +    if (WriteQueueSize())
> +      rv = NS_OK;
> +    else
> +      rv = NS_BASE_STREAM_WOULD_BLOCK;
> +    SetWriteCallbacks(stream->Transaction());

You are not setting mPartialFrame here, why?  I would expect to finish sending this frame to not break the frame consistency.

@@ +1270,5 @@
> +    LOG3(("SpdySession::ReadSegments %p stream=%p generated end of frame %d",
> +          this, stream, *countRead));
> +    mReadyForWrite.Push(stream);
> +    SetWriteCallbacks(stream->Transaction());
> +    return rv;

I need some explanation of how it is about that a stream generated end of frame is indicated by *countRead > 0.

Isn't the log title wrong?

@@ +1280,5 @@
> +  // in normal http this is done by nshttpconnection, but that class does not
> +  // know which http transaction has made this state transition.
> +  stream->Transaction()->
> +    OnTransportStatus(mSocketTransport, nsISocketTransport::STATUS_WAITING_FOR,
> +                      LL_ZERO);

This may be solved by the status patch already:


If I understand correctly, you push the stream to the end of the low level write queue (mReadyForWrite) to check on next access to it it has written all data (*countRead == 0).   If there is a lot of streams in the queue before this one, we may get here after the stream got data back, i.e. sending WAITING_FOR after RECEIVEING_FROM.

Is that possible?  If not, add a comment why.

@@ +1282,5 @@
> +  stream->Transaction()->
> +    OnTransportStatus(mSocketTransport, nsISocketTransport::STATUS_WAITING_FOR,
> +                      LL_ZERO);
> +  /* we now want to recv data */
> +  mConnection->ResumeRecv(stream->Transaction());

Also, as written above, isn't this called quit late when there is a lot of concurrent streams scheduled?
You should call (this->)ResumeRecv, you are not checking mConnection != null

@@ +1317,5 @@
> +
> +  if (mClosed)
> +    return NS_ERROR_FAILURE;
> +
> +  SetWriteCallbacks(nsnull);

I don't understand why is this call here.

@@ +1400,5 @@
> +        LOG3(("SpdySession::WriteSegments %p lookup streamID 0x%X failed. "
> +              "Next = 0x%x", this, streamID, mNextStreamID));
> +        if (streamID >= mNextStreamID)
> +          GenerateRstStream(RST_INVALID_STREAM, streamID);
> +          ChangeDownstreamState(DISCARD_DATA_FRAME);

Not sure how this should be indented or added to a block.  As I understand, the indention is wrong here.  You should also add a blank line before this one or put GenerateRestStream to a block for if.

This type of mistake is big argument to put braces always around executive command of if/for/while.

@@ +1472,5 @@
> +
> +    if (!count) {
> +      ChangeDownstreamState(BUFFERING_FRAME_HEADER);
> +      *countWritten = 1;
> +      return NS_OK;

Where from the '1' comes here?  You didn't actually read from the socket.

What you want is to let WriteSegments be called again, right?  Then you have to return 0 (this breaks the byte-read counters in nsHttpConnection) and NS_OK.  That will loop for you.

@@ +1493,5 @@
> +    return rv;
> +  }
> +  
> +  NS_ABORT_IF_FALSE(mDownstreamState == BUFFERING_CONTROL_FRAME,
> +                    "Not in Bufering Control Frame State");

Because of readability and also since I'm not a big fan of checking just by NS_ABORT_* I'd rather see this as:

if (mDownstreamState == BUFFERING_CONTROL_FRAME) {
  ... do stuff ...
}
else {
  NS_ABORT_IF_FALSE(false, "Bad state....");
}

@@ +1538,5 @@
> +
> +  mClosed = true;
> +  mStreamTransactionHash.Enumerate(Shutdown, this);
> +  GenerateGoAway();
> +  mConnection = nsnull;

Probably worthless to send GenerateGoAway when NS_FAILED(aReason).

You have to drop mSegmentReader and Writer here - if holding them is not actually a bug (!), since from now the connection could go away and you will hold (however unlikely to use) bad pointers.

@@ +1553,5 @@
> +
> +  // need to find the stream and call CleanupStream() on it.
> +  SpdyStream *stream = mStreamTransactionHash.Get(aTransaction);
> +  if (!stream) {
> +    LOG3(("SpdySession::CloseTransaction %p %p %x - not found.",

Probably worth to NS_ERROR here, this would be a code logic error.

@@ +1561,5 @@
> +  LOG3(("SpdySession::CloseTranscation probably a cancel. "
> +        "this=%p, trans=%p, result=%x, streamID=0x%X stream=%p",
> +        this, aTransaction, aResult, stream->StreamID(), stream));
> +  CleanupStream(stream, aResult);
> +  ResumeRecv(this);

Why?  Because you want to read the rest of the data of the stream and discard it?

@@ +1599,5 @@
> +  memcpy(mOutputQueueBuffer.get() + mOutputQueueUsed, buf, count);
> +  mOutputQueueUsed += count;
> +  *countRead = count;
> +
> +  FlushOutputQueue();

Can you please document with a comment for this method why you have chosen this sending and buffering strategy?  

I think you may save memcpy when the queue was emptied after flush and attempt to write the buffer directly again.  What had not been sent should be put to the output queue, logically.  I understand it is very hard, if even possible, avoid large buffer moves with a multiplexing protocol.  Just spot this place it could be potentially improved.  This is not a review requirement.


...mind flow continues: since you always have just a single stream scheduled to write, you could avoid the output queue completely and write the actual data of the stream directly w/o a memcpy to output queue.  But this is the preference between packet fragmentation and doing buffer moves.  That's why I would like you to ask you for some analyzes of your preferences documented here.

@@ +1622,5 @@
> +
> +    if (mFrameDataLast &&
> +        mFrameDataRead == mFrameDataSize) {
> +      // This will result in Close() being called
> +      mNeedsCleanup = mFrameDataStream;

You should add NS_ABORT_IF_FALSE(!mNeedsCleanup); before this line.

@@ +1735,5 @@
> +SpdySession::Transport()
> +{
> +    if (!mConnection)
> +        return nsnull;
> +    return mConnection->Transport();

Indention

@@ +1804,5 @@
> +nsAHttpConnection *
> +SpdySession::Connection()
> +{
> +    NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread");
> +    return mConnection;

Indention

::: netwerk/protocol/http/SpdySession.h
@@ +135,5 @@
> +  const static PRUint32 kDefaultBufferSize = 2000;
> +
> +  const static PRUint32 kDefaultQueueSize =  16000;
> +  const static PRUint32 kQueueTailRoom    =  4000;
> +  const static PRUint32 kSendingChunkSize = 4000;

Any reason why these are not power of 2?

@@ +162,5 @@
> +  enum stateType {
> +    BUFFERING_FRAME_HEADER,
> +    BUFFERING_CONTROL_FRAME,
> +    PROCESSING_DATA_FRAME,
> +    DISCARD_DATA_FRAME,

DISCARDING_DATA_FRAME ?

@@ +219,5 @@
> +  // by the nsClassHashtable implementation when they are removed from
> +  // there.
> +  nsDataHashtable<nsUint32HashKey, SpdyStream *>      mStreamIDHash;
> +  nsClassHashtable<nsPtrHashKey<nsAHttpTransaction>,
> +                   SpdyStream>                        mStreamTransactionHash;

I'm a bit scarred of mapping by an instance pointer.  I have seen bugs where the pointer was recycled and link in the hash table had been that way broken and able to link objects that were no longer in relation.

@@ +229,5 @@
> +  // be removed. But they will be reintroduced for v3, so we will leave
> +  // this queue in place to ease that transition.
> +  nsDeque           mUrgentForWrite;
> +
> +  // If we block while wrting out a frame then this points to the stream

writing

@@ +259,5 @@
> +  // to the stream.
> +  SpdyStream          *mFrameDataStream;
> +  
> +  // A state variable to cleanup a closed stream after the stack has unwound.
> +  SpdyStream          *mNeedsCleanup;

Please describe better how this is used, why you need it, why you need just a single pointer and not an array and how it's ensured you won't miss the cleanup (won't leak).

As I understand, you don't want to call CleanupStream while you are in SpdySession::OnWriteSegment that could cause unpredictable reentrancy or whatever.  You cannot relay on the rv of mFrameDataStream->WriteSegments() since the NS_BASE_STREAM_CLOSED error is eaten by nsPipeOutputStream::WriteSegments (or potentially any other implementation of WriteSegments that appears on the stack), right?

@@ +280,5 @@
> +  nsCString            mFlatHTTPResponseHeaders;
> +  PRUint32             mFlatHTTPResponseHeadersOut;
> +
> +  // when set, the session will go away when it reaches 0 streams
> +  bool                 mShouldGoAway;

Add a comment when it is set, I have found the following cases:
- we are out of outgoing stream ids
- peer is out of stream ids
- we got RST frame with RST_REFUSED_STREAM or RST_CANCEL
- we got GOAWAY frame
- someone called DontReuse()

@@ +318,5 @@
> +};
> +
> +}} // namespace mozilla::net
> +
> +#endif // mozilla_net_SpdySession_h

Some members should include the information in their names whether used for receive or send.

Sending:
- mPartialFrame

Receive:
- mFrameBuffer*
- mFrameData*
- mFrameDataStream

::: netwerk/protocol/http/SpdyStream.cpp
@@ +126,5 @@
> +
> +    if (NS_SUCCEEDED(rv) &&
> +        mUpstreamState == GENERATING_SYN_STREAM &&
> +        !mSynFrameComplete)
> +      mBlockedOnWrite = 1;

Can you comment why this is here?

@@ +129,5 @@
> +        !mSynFrameComplete)
> +      mBlockedOnWrite = 1;
> +    
> +    // Mark that we are blocked on read if we the http transaction
> +    // is going to get us going again.

Typos

@@ +135,5 @@
> +      mRequestBlockedOnRead = 1;
> +
> +    if (!mBlockedOnWrite && NS_SUCCEEDED(rv) && (!*countRead)) {
> +      LOG3(("ReadSegments %p Send Request data complete from %x",
> +            this, mUpstreamState));

This is quit unreadable.  Please add a comment to this branch or change the log to something like:
"ReadSegments %p: Sending request data complete, mUpstreamState=%x"

BTW, could we check for mTransaction->Available() rather?  As pipelining code does.  The check for !*countRead is logical, but means to do one more loop to call mTransaction->ReadSegments again and get info about EOF of the request stream, if I'm right, though.

@@ +254,5 @@
> +  // We can use the simple double crlf because firefox is the
> +  // only client we are parsing
> +  PRInt32 endHeader = mFlatHttpRequestHeaders.Find("\r\n\r\n");
> +  
> +  if (endHeader == -1) {

s/-1/kNotFound/

@@ +330,5 @@
> +
> +  mTxInlineFrame[17] = 0;                         /* unused */
> +  
> +//  nsCString methodHeader;
> +//  mTransaction->RequestHead()->Method()->ToUTF8String(methodHeader);

Worth removing this commented out code?

@@ +409,5 @@
> +  
> +  mTxInlineFrameSize = 18;
> +
> +  LOG3(("http request headers to encode are: \n%s",
> +        mFlatHttpRequestHeaders.get()));

This exposes Auth header in the log, no?

@@ +440,5 @@
> +  // For methods other than POST and PUT, we will set the fin bit
> +  // right on the syn stream packet.
> +
> +  if (mTransaction->RequestHead()->Method() != nsHttp::Post &&
> +      mTransaction->RequestHead()->Method() != nsHttp::Put) {

Not sure OPTIONS is allowed with SPDY but should be here as well.

@@ +442,5 @@
> +
> +  if (mTransaction->RequestHead()->Method() != nsHttp::Post &&
> +      mTransaction->RequestHead()->Method() != nsHttp::Put) {
> +    mSentFinOnData = 1;
> +    mTxInlineFrame[4] = SpdySession::kFlag_Data_FIN;

Where is this set when there is no body when requesting with one of the above listed methods?

@@ +521,5 @@
> +  PRUint32 avail =  mTxStreamFrameSize - mTxStreamFrameSent;
> +
> +  while (avail) {
> +    NS_ABORT_IF_FALSE(countUsed, "null countused pointer in a stream context");
> +    rv = mSegmentReader->OnReadSegment(buf + offset, avail, &transmittedCount);

You should assert or rather return with NS_ERROR log when avail > 0 and buf == null.  You also don't check on the countUsed arg being non-null.  It is not very safe to expect the mTxStreamFrame will be always correct when we change anything.

@@ +579,5 @@
> +  NS_ABORT_IF_FALSE(!mTxStreamFrameSize, "stream frame not empty");
> +  NS_ABORT_IF_FALSE(!mTxStreamFrameSent, "stream partial send not 0");
> +  NS_ABORT_IF_FALSE(!(dataLength & 0xff000000), "datalength > 24 bits");
> +  
> +  (reinterpret_cast<PRUint32 *>(mTxInlineFrame.get()))[0] = PR_htonl(mStreamID);

Maybe wrap after the =.

@@ +696,5 @@
> +  if (len > 0xffff)
> +    len = 0xffff;
> +
> +  PRUint16 networkLen = len;
> +  networkLen = PR_htons(len);

Which one is correct? I presume the letter one :)

@@ +760,5 @@
> +      NS_ABORT_IF_FALSE(mTxInlineFrameSize,
> +                        "OnReadSegment SynFrameComplete 0b");
> +      rv = TransmitFrame(nsnull, nsnull);
> +      if (rv == NS_BASE_STREAM_WOULD_BLOCK && *countRead)
> +        rv = NS_OK;

Can you add a comment why this is needed, also on all other places you are doing this.

@@ +764,5 @@
> +        rv = NS_OK;
> +      if (mTxInlineFrameSize)
> +        ChangeState(SENDING_SYN_STREAM);
> +      else
> +        ChangeState(GENERATING_REQUEST_BODY);

Comment on this code please with something like this:

"mTxInlineFrameSize > 0 means the current frame is in progress of sending.   mTxInlineFrameSize is dropped to 0 after both the frame and its payload (if any) are completely sent out.  Here during GENERATING_SYN_STREAM state we are sending just the headers.  So, only when the frame is completely sent out, we proceed to GENERATING_REQUEST_BODY state."

@@ +809,5 @@
> +        ChangeState(GENERATING_REQUEST_BODY);
> +    break;
> +
> +  case SENDING_SYN_STREAM:
> +    rv = NS_BASE_STREAM_WOULD_BLOCK;

Hmm.. should you ever get here?

::: netwerk/protocol/http/SpdyStream.h
@@ +76,5 @@
> +  {
> +    bool result = !mFullyOpen;
> +    mFullyOpen = 1;
> +    return result;
> +  }

This would be better and more readable as GetFullyOpen and SetFullyOpen defined and used separately.

@@ +109,5 @@
> +  static PLDHashOperator hdrHashEnumerate(const nsACString &,
> +                                          nsAutoPtr<nsCString> &,
> +                                          void *);
> +
> +  void     ChangeState(enum stateType );

Space after stateType

@@ +153,5 @@
> +
> +  // Flag is set when there is more request data to send and the
> +  // stream needs to be rescheduled for writing. Sometimes this
> +  // is done as a matter of fairness, not really due to blocking
> +  PRUint32                     mBlockedOnWrite       : 1;

Also according the comment, maybe would be worth to rename this flag to something like mHasDataToWrite or something.

@@ +174,5 @@
> +  // The InlineFrame and associated data is used for composing control
> +  // frames and data frame headers.
> +  nsAutoArrayPtr<char>         mTxInlineFrame;
> +  PRUint32                     mTxInlineFrameAllocation;
> +  PRUint32                     mTxInlineFrameSize;

To be consistent with nsSpdySession member naming, shouldn't you

s/mTxInlineFrameAllocation/mTxInlineFrameSize/
s/mTxInlineFrameSize/mTxInlineFrameUsed/  

?

Another potential reason to encapsulate this to a buffer-like class.

@@ +181,5 @@
> +  // mTxStreamFrameSize and mTxStreamFrameSent track the progress of
> +  // transmitting a request body data frame. The data frame itself
> +  // is never copied into the spdy layer.
> +  PRUint32                     mTxStreamFrameSize;
> +  PRUint32                     mTxStreamFrameSent;

I still fight these a bit...  The names, also according the spec, are not the best chosen IMO.  TxInlineFrame* should just be TxFrame* and TxStream* should be TxDataPayload*.

@@ +194,5 @@
> +  // place the fin flag on the last data packet instead of waiting
> +  // for a stream closed indication. Relying on stream close results
> +  // in an extra 0-length runt packet and seems to have some interop
> +  // problems with the google servers.
> +  PRInt64                      mRequestBodyLen;

Shouldn't be called mRequestBodyLenRemaning or so?

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +77,5 @@
>      // after a transaction returned NS_BASE_STREAM_WOULD_BLOCK from its
>      // ReadSegments/WriteSegments methods.
>      //
> +    virtual nsresult ResumeSend(nsAHttpTransaction *caller) = 0;
> +    virtual nsresult ResumeRecv(nsAHttpTransaction *caller) = 0;

I can't say I'm a big fan of this argument.  It forces the programmer to think of what the implementation may all do with the caller or, more likely, search for the reference to pass when there is none at the moment..

I understand the reason for this is only to put the calling transaction's stream to mReadyForWrite queue.  Would be better to let the stream implement this interface, let the transaction relay thought it to the session.  The stream would be then pushing it self to the queue in its ResumeSend implementation.

However, true is that adding this arguments makes the code simpler.

This is just a design thought, not a review request.  I also wasn't thinking of potential cycle leaks.

If we agree on leaving the arg, it at least must be documented in the comment.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4108,5 @@
>  
> +    if (gHttpHandler->IsSpdyEnabled() && !mCachePump && NS_FAILED(mStatus) &&
> +        (mLoadFlags & LOAD_REPLACE) && mOriginalURI && mAllowSpdy) {
> +        // For sanity's sake we may want to cancel an alternate protocol
> +        // redirection involving the original host name

If you really want to remove hosts on a failure I think you should remove the host regardless IsSpdyEnabled() that could be dropped between adding the host to AP hash and getting here.

Also, better would be to have a flag that you redirected to AP (https actually) and use it here, but we already have a lot of flags...  So, up to you.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +157,5 @@
> +    // If for some reason the components to check on NPN aren't available,
> +    // this function will just return true to continue on and disable SPDY
> +
> +    NS_ABORT_IF_FALSE(mSocketTransport, "EnsureNPNComplete "
> +                      "socket transport precondition");

Not enough check for opt builds.

@@ +196,5 @@
> +    LOG(("nsHttpConnection::EnsureNPNComplete %p negotiated to '%s'",
> +         this, negotiatedNPN.get()));
> +    
> +    if (negotiatedNPN.Equals(NS_LITERAL_CSTRING("spdy/2"))) {
> +

Unnecessary blank line.

@@ +198,5 @@
> +    
> +    if (negotiatedNPN.Equals(NS_LITERAL_CSTRING("spdy/2"))) {
> +
> +        mUsingSpdy = true;
> +        mIsReused = true;    /* all spdy streams are reused */

Good to know why (good comments wanted here).  You know, this flag is kinda sensitive.

@@ +329,5 @@
> +    // spdy.
> +    //
> +
> +    if (!gHttpHandler->IsSpdyEnabled() || mUsingSpdy)
> +        return;

I think it is OK to collect this information even when SPDY is disabled to be ready for use immediately when we enable SPDY.  But it is disputable.

@@ +416,5 @@
> +    mKeepAliveMask = false;
> +    mKeepAlive = false;
> +    mIdleTimeout = 0;
> +    if (mUsingSpdy)
> +        mSpdySession->DontReuse();

Rather if (mSpdySession) ?

@@ +918,5 @@
>      PRUint32 n;
>      bool again = true;
>  
>      do {
> +        mSocketOutCondition = NS_OK;

I don't see an actual reason to do this.  Can you please very well describe why this is better then to rather initialize in the constructor?  I'm for removing this change otherwise.

@@ +950,2 @@
>          else {
> +            if (gHttpHandler->IsSpdyEnabled() && !mReportedSpdy) {

Hmm... for consistency you may want to report the connection regardless IsSpdyEnabled().  There should be a flag on the connection whether it was started with spdy enabled or not.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +143,5 @@
>      LOG(("nsHttpConnectionMgr::Init\n"));
>  
>      {
>          ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +        mSpdyPreferredHash.Init();

Why isn't this also initialized in the constructor?

@@ +238,5 @@
>  {
> +    // Leave the timer in place if there are connections that potentially
> +    // need management
> +    if (mNumIdleConns || (mNumActiveConns && gHttpHandler->IsSpdyEnabled()))
> +        return;

What happens when there are active SPDY connections and user drops the pref off?  You may then IMO cancel the timer and leave the active spdy connections open indefinitely or for an undefined amount of time.

@@ +415,5 @@
> +    nsConnectionEntry *ent = mCT.Get(ci->HashKey());
> +    
> +    // If there is no sign of coalescing (or it is disabled) then just
> +    // return the primary hash lookup
> +    if (!gHttpHandler->IsSpdyEnabled() || !gHttpHandler->CoalesceSpdy() ||

Not a good idea to add !gHttpHandler->CoalesceSpdy() here.

What if the user switches this off after we already have some coalesced ents?  You will not find the correct ent here..

@@ +466,5 @@
>      return NS_OK;
>  }
>  
> +void
> +nsHttpConnectionMgr::ReportSpdyConnection(nsHttpConnection *conn,

Add comment for this method, when and where from it is called, what all it does, what all flags it changes, etc.

@@ +482,5 @@
> +    ent->mTestedSpdy = true;
> +
> +    if (!usingSpdy) {
> +        if (ent->mUsingSpdy)
> +            conn->DontReuse();

Explain this please in a comment.

@@ +503,5 @@
> +        mSpdyPreferredHash.Get(ent->mCoalescingKey);
> +
> +    LOG(("ReportSpdyConnection %s %s ent=%p ispreferred=%d\n",
> +         ent->mConnInfo->Host(), ent->mCoalescingKey.get(),
> +         ent, preferred));

preferred != nsnull ?

@@ +513,5 @@
> +    }
> +    else if (preferred != ent) {
> +        // A different hostname is the preferred spdy host for this
> +        // IP address.
> +        ent->mUsingSpdy = true;

This is duplicated in this method...

@@ +514,5 @@
> +    else if (preferred != ent) {
> +        // A different hostname is the preferred spdy host for this
> +        // IP address.
> +        ent->mUsingSpdy = true;
> +        conn->DontReuse();

So, this will cause the following:
- the current transaction(s) dispatched to this ent will be processed using this connection
- all following transactions to the ent's host will be processed by the preferred ent's available connection (coalesced)
- this may happen only in case of a tight race condition when a preferred entry has been detected after the transaction has already been dispatched to this ent

Right?  Please add a comment that explains what call to DontReuse() does/why are you doing it.

@@ +527,5 @@
> +    // The Alternate Protocol hash is protected under the monitor because
> +    // it is read from both the main and the network thread.
> +    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +
> +    return mAlternateProtocolHash.Contains(hostPortKey);

You definitely must check the pref here.  If user switches the pref off while already data for AP are recorded, AP will still be used regardless the option.

@@ +535,5 @@
> +nsHttpConnectionMgr::ReportSpdyAlternateProtocol(nsHttpConnection *conn)
> +{
> +    // Check network.http.spdy.use-alternate-protocol pref
> +    if (!gHttpHandler->UseAlternateProtocol())
> +        return;

You may want to collect AP info even when it is disabled and have data ready to use when user enables it.

@@ +647,5 @@
> +        return nsnull;
> +
> +    sslSocketControl = do_QueryInterface(securityInfo, &rv);
> +    if (NS_FAILED(rv))
> +        return nsnull;

These two early returns might be armed with NS_WARNINGS.

@@ +657,5 @@
> +
> +    if (NS_FAILED(rv) || !isJoined) {
> +        LOG(("nsHttpConnectionMgr::GetSpdyPreferredConnection "
> +             "Host %s cannot be confirmed to be joined "
> +             "with %s connections",

Add log of rv and isJoined values.

@@ +664,5 @@
> +    }
> +
> +    // IP pooling confirmed
> +    LOG(("nsHttpConnectionMgr::GetSpdyPreferredConnection "
> +         "Host %s has cert valid for %s connections",

Maybe worth to mention in the log that host X is coalescing with the host Y.

@@ +673,5 @@
> +void
> +nsHttpConnectionMgr::SetSpdyPreferred(nsConnectionEntry *ent)
> +{
> +    if (!gHttpHandler->CoalesceSpdy())
> +        return;

You call this method from nsHttpConnectionMgr::ReportSpdyConnection.

Even coalescing is disabled and you don't add this entry to the hash table, you set ent->mSpdyPreferred = true in that method.

You should set the flag only when you add the entry to the hash table.

Also I think the check for CoalesceSpdy() is redundant here.  Checking mCoalescingKey.IsEmpty() is enough and actually better here.

@@ +685,5 @@
> +void
> +nsHttpConnectionMgr::RemoveSpdyPreferred(nsACString &aHashKey)
> +{
> +    if (!gHttpHandler->CoalesceSpdy())
> +        return;

Don't check this here.  If user drops the pref while having the entry already in the preferred hash table, you will hold on an invalid pointer!

@@ +785,3 @@
>      // If time to next expire found is shorter than time to next wake-up, we need to
>      // change the time for next wake-up.
> +    if (liveConnections) {

You don't need this flag, timeToNextExpire is your flag here already.

@@ +919,5 @@
> +            // Check to see if a pending transaction was dispatched with the
> +            // coalesce logic
> +            if (count != ent->mPendingQ.Length()) {
> +                count = ent->mPendingQ.Length();
> +                i = 0;

Where is the coalesce logic you refer here to?

@@ +1066,5 @@
> +        // before considering idle http ones.
> +        if (gHttpHandler->IsSpdyEnabled()) {
> +            conn = GetSpdyPreferredConn(ent);
> +            if (conn)
> +                addConnToActiveList = false;

Why don't you just return here with this connection?

Are you planning some changes in the code bellow that would require processing even we already have a connection to dispatch to?

@@ +1108,5 @@
> +            !ent->mConnInfo->UsingHttpProxy())
> +        {
> +            // If this is a possible Spdy connection we need to limit the number
> +            // of connections outstanding to 1 while we wait for the spdy/https
> +            // ReportSpdyConnection()

It took a little time for me to understand this comment.  When I rephrase it says:

"If this host is trying to negotiate a SPDY session right now, don't create any new connections."

@@ +1207,5 @@
> +        LOG(("Spdy Dispatch Transaction via Activate(). Transaction host = %s,"
> +             "Connection host = %s\n",
> +             aTrans->ConnectionInfo()->Host(),
> +             conn->ConnectionInfo()->Host()));
> +        rv = conn->Activate(aTrans, caps, priority);

When you are doing this UsingSpdy() branching, wouldn't be nicer to directly call AddTransaction here and drop the mUsingSpdy branching in Activate then?

From the code I can see mUsingSpdy couldn't ever be set while not having mTransaction set (which is at this moment a spdy session).


From the design point of view, it would be interesting to have AddTransaction method also used for pipelining..

@@ +1389,5 @@
> +        return;
> +
> +    for (PRInt32 index = ent->mPendingQ.Length() - 1;
> +         index >= 0 && conn->CanDirectlyActivate();
> +         --index) {

for (PRInt32 index = ent->mPendingQ.Length();
             index > 0 && conn->CanDirectlyActivate();) 
{
             --index;

@@ +1400,5 @@
> +        ent->mPendingQ.RemoveElementAt(index);
> +
> +        nsresult rv2 = DispatchTransaction(ent, trans, trans->Caps(), conn);
> +        NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv2), "Dispatch SPDY Transaction");
> +        NS_RELEASE(trans);

I think you have to put the transaction back to the pending queue or close on failure here.  Otherwise it will hang I think.  (If put back then the spdy session should be closed, if not already.)

@@ +1430,5 @@
> +    nsConnectionEntry *preferred = GetSpdyPreferred(ent);
> +
> +    // this entry is spdy-enabled if it is involved in a redirect
> +    if (preferred)
> +        ent->mUsingSpdy = true;

Please just add a comment that this will make all *new* connection on this entry go spdy.

@@ +1590,4 @@
>  
> +    if (!ent) {
> +        // this should never happen
> +        NS_ASSERTION(ent, "no connection entry");

This has to be NS_ERROR("..").

@@ +1601,5 @@
>          // This is never the final reference on conn as the event context
>          // is also holding one that is released at the end of this function.
> +
> +        if (ent->mUsingSpdy)
> +            conn->DontReuse();

Needs a comment.

As I understand, when a SPDY connection is released, the session is gone (closed) and therefor according the spec the connection can never be used again for any kind of data, right?

@@ +2127,5 @@
> +                 mEnt->mCoalescingKey.get()));
> +
> +            gHttpHandler->ConnMgr()->ProcessSpdyPendingQ(mEnt);
> +        }
> +    }

Would be nicer separated to a new method.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +199,5 @@
> +        // mSpdyPreferred hash.
> +        //
> +        nsCString mCoalescingKey;
> +
> +        // To have the UsingSpdy flag means some host with the same hash information

what the "hash information" exactly is?

@@ +204,5 @@
> +        // has done NPN=spdy/2 at some point. It does not mean every connection
> +        // is currently using spdy.
> +        bool mUsingSpdy;
> +
> +        bool mTestedSpdy;

Add comments please.

As I understand, this is set to true when we first write on any connection of this entry, right?

Please add comments what all combinations of flag values occur under what conditions.  Like "mTestedSpdy == false, mUsingSpdy == true" means that this entry never tried to create a connection but had been coalesced with an existing preferred one, etc.

@@ +322,5 @@
> +
> +    // Manage the preferred spdy connection entry for this address
> +    nsConnectionEntry *GetSpdyPreferred(nsConnectionEntry *aOriginalEntry);
> +    void               SetSpdyPreferred(nsConnectionEntry *ent);
> +    void               RemoveSpdyPreferred(nsACString &aDottedDecimal);

Please suffix these three methods with Ent.  There is also GetSpdyPreferredConn that confuses when the code is read the first time.

@@ +330,5 @@
> +                                             nsHttpConnection *conn,
> +                                             nsHttpTransaction *trans);
> +
> +    void               ProcessSpdyPendingQ(nsConnectionEntry *ent);
> +    void               ProcessSpdyPendingQ();

No overloading please when the functionality differs... s/ProcessSpdyPendingQ()/ProcessAllSpdyPendingQ()/

@@ +414,5 @@
>      //
>      nsClassHashtable<nsCStringHashKey, nsConnectionEntry> mCT;
> +
> +    // this table is protected by the monitor
> +    nsCStringHashSet mAlternateProtocolHash;

Hmm.. shouldn't this be rather called mSPDY2AlternateProtocolHash?  Because according the code, this gets filled only for "443:npn-spdy/2" header value and you are using this had to redirect to https expecting to get it though spdy.
Comment 4 Patrick McManus [:mcmanus] 2012-01-17 14:02:37 PST
Honza, thanks so much for reading all this code and your thoughtful comments.

I'm going to respond to 1 file at a time. (well, 2 files I guess a cpp and h).

First up: SpdyStream


> ::: netwerk/protocol/http/SpdyStream.cpp


> "ReadSegments %p: Sending request data complete, mUpstreamState=%x"
> 
> BTW, could we check for mTransaction->Available() rather?  As pipelining
> code does.  The check for !*countRead is logical, but means to do one more
> loop to call mTransaction->ReadSegments again and get info about EOF of the
> request stream, if I'm right, though.
> 

mTransaction->Available() is defined as 
    // called to find out how much request data is available for writing.

what is immediately available is not the same as !*countRead which really is EOF (and we generate the fin from this so it must be eof)

> 
> @@ +442,5 @@
> > +
> > +  if (mTransaction->RequestHead()->Method() != nsHttp::Post &&
> > +      mTransaction->RequestHead()->Method() != nsHttp::Put) {
> > +    mSentFinOnData = 1;
> > +    mTxInlineFrame[4] = SpdySession::kFlag_Data_FIN;
> 
> Where is this set when there is no body when requesting with one of the
> above listed methods?

you're asking about a content-length 0 Post/Put ?

It has a data frame with 0 bytes and a fin bit set on it based on calling the nsHttpTransaction::ReadSegements. It has a request-body, just a 0 length one. I've stepped through this in the debugger.

> > +  (reinterpret_cast<PRUint32 *>(mTxInlineFrame.get()))[0] = PR_htonl(mStreamID);
> 
> Maybe wrap after the =.

it fits in 80 char; I'd rather not.

> 
> 
> @@ +809,5 @@
> > +        ChangeState(GENERATING_REQUEST_BODY);
> > +    break;
> > +
> > +  case SENDING_SYN_STREAM:
> > +    rv = NS_BASE_STREAM_WOULD_BLOCK;
> 
> Hmm.. should you ever get here?
> 

if code calls  while (NS_SUCCEEDED(spdystream->onReadSegement()));


> 
> @@ +153,5 @@
> > +
> > +  // Flag is set when there is more request data to send and the
> > +  // stream needs to be rescheduled for writing. Sometimes this
> > +  // is done as a matter of fairness, not really due to blocking
> > +  PRUint32                     mBlockedOnWrite       : 1;
> 
> Also according the comment, maybe would be worth to rename this flag to
> something like mHasDataToWrite or something.

The name of the flag is ok imo - the stream is blocked pending a write operation. The comment has been updated to clarify things.

> 
> @@ +181,5 @@
> > +  // mTxStreamFrameSize and mTxStreamFrameSent track the progress of
> > +  // transmitting a request body data frame. The data frame itself
> > +  // is never copied into the spdy layer.
> > +  PRUint32                     mTxStreamFrameSize;
> > +  PRUint32                     mTxStreamFrameSent;
> 
> I still fight these a bit...  The names, also according the spec, are not
> the best chosen IMO.  TxInlineFrame* should just be TxFrame* and TxStream*
> should be TxDataPayload*.


the names describe how the members are used in the code. inline buffers are those generated by the spdy code, and stream buffers are those that are passed into the spdy code from the http it encapsulates.
Comment 5 Patrick McManus [:mcmanus] 2012-01-17 14:04:17 PST
Created attachment 589297 [details] [diff] [review]
spdystream 0

various spdystream bits
Comment 6 Patrick McManus [:mcmanus] 2012-01-19 10:35:02 PST
SpdySession cpp/h

while testing my updates for the review comments I began to wonder about the partialstream (i.e. the stream that had a partially written frame sent out) and I found a couple problems with it. Basically, while the session code did prevent other data frames from being written while a partial data frame was outstanding it didn't prevent control messages such as pings or rsts from being written - which would corrupt the data stream. I wasn't able to make that happen under normal situations, but if I modified spdysession::onreadsegment to intentionally return partial writes and then engaged in large uploads I could make it happen.

I've included the fix as a separate patch that is dependent on the patch that addresses the review comments so as not to complicate the review comment patch. It removes about the same # of lines than it adds - so it must be a good thing :).. Basically it gets rid of the whole partialstream/stream::blockedonwrite concept and makes writes at the stream level be all-or-nothing up to frame size with the session having to buffer immediately if the network takes a short write of a partial frame. The simplification is for the better as the buffering cannot be large (frames are only 4K) and it will generally not start new writes if buffering them would mean extending the buffer allocation. Definitely a simplification.


onto the spdysession review comments:

> @@ +176,5 @@
> > +    LOG4(("%s", linebuf));
> > +  }
> > +}
> > +
> > +typedef nsresult  (*Control_FX) (SpdySession *self);
> 
> I'm really against calling a method by indexing array using something we
> have read from the network even though it has been sanitized.  This may open
> a risk for security bugs, IMO.

I'm 100% fine with it as long as its bounds checked. Adding the FIRST and LAST macros helps identify it as important.

> > +void
> > +SpdySession::SetWriteCallbacks(nsAHttpTransaction *aTrans)
> 
> This method's name isn't quit telling me what it does. 

I know you don't care for it - but I really do and will just claim author privilege on the name. Having a WriteCallback set is pretty clear for me - when the associated socket is writable the write handler rooted at nsHttpConnection::OnSocketWritable will be called.


> nsHttpConnection::ResumeSend that doesn't use this arg.

I don't care for the mechanism either. But there remains the problem of how a transaction that has experienced a flow control event in the past (i.e. it can't read the data for the request stream) can tell the multiplexed connection that it now has data to send. It can tell the connection that there is some data to send through resumesend() but the poor connection cannot figure out where the data needs to be sourced from which honestly seems like a limitation of the resumesend() api which is why I extended it. But it does touch a lot of code with basically nops.

So, I restructured this - getting rid of the args to resumesend and resumerecv and setwritecallbacks(). I added a nsAHttpConnection::TransactionHasDataToWrite(nsAhttpTransaction *) method to fill the gap - but gave it a default implementation for non multiplexed nsahttpconnections.. so the code impact is very minor - being used in just one place. its not perfect, but its better.

> 
> @@ +395,5 @@
> > +    return;
> > +  
> > +  objSize = newSize;
> > +  nsAutoArrayPtr<char> tmp(new char[objSize]);
> > +  memcpy (tmp, buf, preserve);
> 

> My first encounter of call of this method was realloc from 16000 to 16012. 
> Quit wasting I'd say.  You should enlarge the newSize up to some meaningful
> value.  I think there is some heuristic in nsTArray to grow the buffer up. 
> You may check it and use the grow logic here.  I think you should grow by at
> least the current size of the buffer till some reasonable threshold of
> course to prevent too many reallocations and heap fragmentation.

I think you misremember how aggressive nstarray is - it doubles only sizes less than 4KB, after that it rounds up to the next 4KB boundary. I've made some adjustments to ensurebuffer so that it adds at least 2KB of extra space and rounds that result up to 4KB.

The more important issue here is that these buffers aren't meant to really keep growing exactly for the reasons you cite.

because you cited 16,000 I'm pretty sure I know what you saw and we can make it better. The general output routine tries to go right to the network, if that fails it will buffer in the output-q up to the free space in that buffer. that buffer starts at 16,000. After that it returns WOULD_BLOCK - it does not grow that buffer. But after filling, and before it could be successfully flushed there was need to create some kind of meta output (maybe a goaway?) of 12 bytes and that triggered the reallocation.

I've made that less likely to happen by leaving a little room in the output-q in the general output case so that some small amount of immediate data (which has to be buffered) will fit eithout extending the buffer.

In the general case these buffers should only be growing when headers don't fit into them, probably because of huge cookies.

> 
> Also, I would like you to encapsulate your buffer logic to a class/struct. 
> As I understand you have 4 buffers that share most of the logic (or could be
> made to share).  It is very hard to track and check all places you are doing
> all buffer operations at.  Raises potential of a well hidden security issue.
> Take this as a review request.

barring an actual problem I think this is of relatively low value and kind of a pain. Can you just file an enhancement bug instead?


> 
> @@ +702,5 @@
> > +  nsresult abortCode = NS_OK;
> > +
> > +  if (!aStream->RecvdFin() && aStream->StreamID()) {
> > +    LOG3(("Stream had not processed recv FIN, sending RST"));
> > +    GenerateRstStream(RST_CANCEL, aStream->StreamID());
> 
> Are you sure this is the proper code to send?  Is it even necessary to send
> a RST in this case?

sending the rst lets the other end know that we won't be processing any more received packets for this stream id - given that we haven't seen a fin from the server must be planning to send at least 1 more with that fin - which we would rst. This lets him know earlier we're not going to accept it.

> 
> @@ +725,5 @@
> > +    mFrameDataStream = nsnull;
> > +  }
> > +
> > +  // check the streams blocked on write, this is linear but the list
> > +  // should be pretty short.
> 
> Not sure this comment is correct.

it certainly is in the common case - streams don't block on write for more than a blip unless they are sending large POST/PUT bodies.. and generally when that is happening there is no other concurrency going on. Error situations are a little uglier, but in reality we are still talking about a few dozen compares out at the tail end during error processing. I feel confident that the overhead of keeping an index on that q would be more work in general.

> 
> @@ +923,5 @@
> > +          "0x%X failed", self, streamID));
> > +    return NS_ERROR_ILLEGAL_VALUE;
> > +  }
> > +    
> > +  self->ChangeDownstreamState(PROCESSING_CONTROL_RST_STREAM);
> 
> have written you have to wait for reading from the socket first before you
> actually reset the stream in WriteSegments, right? 

no, because NS_OK is returned to nsHttpConnection::OnSocketReadable, which will loop and call session::WriteSegments immediately

> 
> @@ +956,5 @@
> > +        self, numEntries));
> > +
> > +  for (PRUint32 index = 0; index < numEntries; ++index) {
> > +    // To clarify the v2 spec:
> > +    // Each entry is a 24 bits of a little endian id
> 
> Are you sure about little endian?  The comment says the spec might be wrong,
> can you confirm this?  Was this communicated to Google?

https://groups.google.com/d/msg/spdy-dev/nm_bC-sbwCo/lSGlfi41jysJ

> 
> @@ +1078,5 @@
> > +  self->mCleanShutdown = true;
> > +  
> > +  LOG3(("SpdySession::HandleGoAway %p GOAWAY Last-Good-ID 0x%X.",
> > +        self, self->mGoAwayID));
> > +  self->ResumeRecv(self);
> 
> Resuming because want to catch the socket close after go away?  Please add a
> comment why you resume here.

receiving a go away does not mean that the server won't send more data for existing streams - it just means it won't accept new streams. The whole idea is to give time for exisintg streams to shutdown orderly without adding new ones.

> @@ +1130,5 @@
> > +  PRUint64 progress;
> > +};
> > +
> > +static PLDHashOperator
> > +StreamTransportStatus(nsAHttpTransaction *key,
> 
> StreamTransportStatusEnumerator please

that's removed in the ontransportstatus update patch.

> 
> @@ +1249,5 @@
> > +
> > +    // call readsegments again if there are other streams ready
> > +    // to run in this session
> > +    if (WriteQueueSize())
> > +      rv = NS_OK;
> 
> Here countRead may be 0 and this way you tell the connection that all has
> been sent out (rv == NS_OK, n == 0) 

the stream has marked itself "RequestBlockedOnRead" - meaning the data source for the request-body is blocked on i/o. it can't be done sending data out.

let's revisit any comments about transport status in the transport status patch bug.

> @@ +1252,5 @@
> > +    if (WriteQueueSize())
> > +      rv = NS_OK;
> > +    else
> > +      rv = NS_BASE_STREAM_WOULD_BLOCK;
> > +    SetWriteCallbacks(stream->Transaction());
> 
> You are not setting mPartialFrame here, why?  I would expect to finish
> sending this frame to not break the frame consistency.

the branch is testing RequestBlockedOnRead which doesn't indicate a partially sent frame - it means there is more data to send on the stream - but the actual frames already sent might be complete which allows us to multiplex other streams onto the session [with the patch I mentioned above there is no concept of partially sent frames anymore anyhow]

> 
> @@ +1270,5 @@
> > +    LOG3(("SpdySession::ReadSegments %p stream=%p generated end of frame %d",
> > +          this, stream, *countRead));
> > +    mReadyForWrite.Push(stream);
> > +    SetWriteCallbacks(stream->Transaction());
> > +    return rv;
> 
> I need some explanation of how it is about that a stream generated end of
> frame is indicated by *countRead > 0.
> 

it generated some data (>0) and it was not blockedonwrite (that would have returned), therefore there is no partial frame therefore it generated the end of a frame.[with the patch I mentioned at the start of this comment there no longer is a partialframe anyhow - any data that went our was a complete frame]


> 
> @@ +1282,5 @@
>
> > +  /* we now want to recv data */
> > +  mConnection->ResumeRecv(stream->Transaction());
> 
> Also, as written above, isn't this called quit late when there is a lot of
> concurrent streams scheduled?

this will be done the first time a stream has reached this state - it doesn't mean all the concurrent streams need to reach this state. Not sure why it would need to be done earlier. This is still essentially a write then read protocol.

> @@ +1317,5 @@
> > +
> > +  if (mClosed)
> > +    return NS_ERROR_FAILURE;
> > +
> > +  SetWriteCallbacks(nsnull);
> 
> I don't understand why is this call here.
> 

Sadly, I cannot figure it out either. While it might be un-necessary it is obviously correct and harmless (only if there is data that needs to be sent it calls resumesend()) so can we file a bug about dealing with it separately? If there was a reason I can't remember then there is some risk in touching it and I just want to separate that risk from the review patch.

It might have dealt with the sslthread code where a write would fail with WOULD_BLOCK if a poll for READ had succeeded until that READ was completed. Since this code does the read, it appears to be rescheduling any writes.
 
> @@ +1493,5 @@
> > +    return rv;
> > +  }
> > +  
> > +  NS_ABORT_IF_FALSE(mDownstreamState == BUFFERING_CONTROL_FRAME,
> > +                    "Not in Bufering Control Frame State");
> 
> Because of readability and also since I'm not a big fan of checking just by
> NS_ABORT_* I'd rather see this as:
> 
> if (mDownstreamState == BUFFERING_CONTROL_FRAME) {
>   ... do stuff ...
> }
> else {
>   NS_ABORT_IF_FALSE(false, "Bad state....");
> }
> 

i'm ok with providing both an ABORT and a opt-runtime-handling path for interfaces that might be called incorrectly but not for internal code state thing like this. That's not an error check - its an assertion of program state and doesn't require a runtime fallback.


> 
> @@ +1553,5 @@
> > +
> > +  // need to find the stream and call CleanupStream() on it.
> > +  SpdyStream *stream = mStreamTransactionHash.Get(aTransaction);
> > +  if (!stream) {
> > +    LOG3(("SpdySession::CloseTransaction %p %p %x - not found.",
> 
> Probably worth to NS_ERROR here, this would be a code logic error.

not necessarily a code error, closetransaction often comes as the result of cancel's (nav events) that have to be posted across threads and event queues asynchronously.. it certainly possible it doesn't arrive until after the stream has been cleaned up for some reason (e.g. receipt of a RST frame). This isn't a problem for the integrity of the hash though, because a new key with that value cannot be created until the origial nsahttptransction goes away (not the stream that was servicing it).

> 
> @@ +1561,5 @@
> > +  LOG3(("SpdySession::CloseTranscation probably a cancel. "
> > +        "this=%p, trans=%p, result=%x, streamID=0x%X stream=%p",
> > +        this, aTransaction, aResult, stream->StreamID(), stream));
> > +  CleanupStream(stream, aResult);
> > +  ResumeRecv(this);
> 
> Why?  Because you want to read the rest of the data of the stream and
> discard it?

that is one reason. But there are probably other frames for other streams on this connection with incoming data (only 1 stream is being closed here, not the session)- our state now allows us to consume and dispatch them normally.

> 
> @@ +1599,5 @@
> > +  memcpy(mOutputQueueBuffer.get() + mOutputQueueUsed, buf, count);
> > +  mOutputQueueUsed += count;
> > +  *countRead = count;
> > +
> > +  FlushOutputQueue();
> 
> Can you please document with a comment for this method why you have chosen
> this sending and buffering strategy?  
> 

I added some comments and took your advice of starting by trying to flush the old-queue to see if we could avoid copying the new data.

as further information, part of this strategy was driven by the SSL stack's habit of returning WOULD_BLOCK almost all the time before the SSL thread was removed. This allowed the writes to be batched up into 1 larger write later on, reducing the number of times the whole event loop had to spin to get it out the door, and unless you are posting something the output queue default size pretty much holds all your syn_stream data.  I originally did it without using the outputqueue for request data but the number of spins was incredible. Things are better now with the ssl changes, but the phenomenon still exists much more than for non-ssl writes so its a layer I think is worth having - it also lets us coalesce things together when there is a legit WOULD_BLOCK event which is a nice thing.

 
> I'm a bit scarred of mapping by an instance pointer.  I have seen bugs where
> the pointer was recycled and link in the hash table had been that way broken
> and able to link objects that were no longer in relation.

I hear what you're saying - The nice thing about this particular usage is that the nsClassHashTable handles the deletion of the object when it is removed from the hash. So that helps some and some existing interfaces (e.g. CloseTransaction()) really make looking up by pointer a necessity.


> As I understand, you don't want to call CleanupStream while you are in
> SpdySession::OnWriteSegment that could cause unpredictable reentrancy or
> whatever.  You cannot relay on the rv of mFrameDataStream->WriteSegments()
> since the NS_BASE_STREAM_CLOSED error is eaten by
> nsPipeOutputStream::WriteSegments (or potentially any other implementation
> of WriteSegments that appears on the stack), right?

yes, that's right.
Comment 7 Patrick McManus [:mcmanus] 2012-01-19 10:38:56 PST
Created attachment 589905 [details] [diff] [review]
spdy session review comment updates v0
Comment 8 Patrick McManus [:mcmanus] 2012-01-19 10:40:45 PST
Created attachment 589906 [details] [diff] [review]
frame writes should be atomic v0
Comment 9 Patrick McManus [:mcmanus] 2012-01-19 10:45:17 PST
(
> 
> I wanted to set some flag (my first time I do a postmortem review) here to
> 

I hope you mean s/postmortem/postfacto/  - we're not dead yet :)

I'm just having fun - seriously, thanks for wading through all this.
Comment 10 Honza Bambas (:mayhemer) 2012-01-19 11:43:43 PST
(In reply to Patrick McManus [:mcmanus] from comment #9)
> (
> > 
> > I wanted to set some flag (my first time I do a postmortem review) here to
> > 
> 
> I hope you mean s/postmortem/postfacto/  - we're not dead yet :)

My english not perfect :)  Thanks.

> 
> I'm just having fun - seriously, thanks for wading through all this.

Yeah, it was a lot of time spent on this.  Hopefully all just useful time.
Comment 11 Patrick McManus [:mcmanus] 2012-01-19 14:48:31 PST
this is the last of the review comment responses convering nsHttp* 

> 
> ::: netwerk/protocol/http/nsAHttpConnection.h
> @@ +77,5 @@
> >      // after a transaction returned NS_BASE_STREAM_WOULD_BLOCK from its
> >      // ReadSegments/WriteSegments methods.
> >      //
> > +    virtual nsresult ResumeSend(nsAHttpTransaction *caller) = 0;
> > +    virtual nsresult ResumeRecv(nsAHttpTransaction *caller) = 0;
> 
> I can't say I'm a big fan of this argument. 

this was reverted (and replaced with something smaller impact) in the SpdySession patch. Let me know what you think of that.

> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +4108,5 @@
> >  
> > +    if (gHttpHandler->IsSpdyEnabled() && !mCachePump && NS_FAILED(mStatus) &&
> > +        (mLoadFlags & LOAD_REPLACE) && mOriginalURI && mAllowSpdy) {
> > +        // For sanity's sake we may want to cancel an alternate protocol
> > +        // redirection involving the original host name
> 
> If you really want to remove hosts on a failure I think you should remove
> the host regardless IsSpdyEnabled() that could be dropped between adding the
> host to AP hash and getting here.
> 

sure. A lot of the isSpdyEnabled() checks were just to enhance the reasoning around "no code exsiting paths are changed when spdy is checked in until review.

>
> ::: netwerk/protocol/http/nsHttpConnection.cpp
> @@ +157,5 @@
> > +    // If for some reason the components to check on NPN aren't available,
> > +    // this function will just return true to continue on and disable SPDY
> > +
> > +    NS_ABORT_IF_FALSE(mSocketTransport, "EnsureNPNComplete "
> > +                      "socket transport precondition");
> 
> Not enough check for opt builds.

I know ABORT is a nop on an opt build - but I don't know what you're comment on. It is just asserting program state.

If you think mSocketTransport could be null somehow, then both opt and debug need a path to deal with it other than abort. but I don't think there is one.

> 
> I think it is OK to collect this information even when SPDY is disabled to
> be ready for use immediately when we enable SPDY.  But it is disputable.

changing config state is generally a very rare thing to do - so I'll save the tiny bit of ram in this case rather than assume we're in a race condition.

> 
> @@ +918,5 @@
> >      PRUint32 n;
> >      bool again = true;
> >  
> >      do {
> > +        mSocketOutCondition = NS_OK;
> 
> I don't see an actual reason to do this.  Can you please very well describe
> why this is better then to rather initialize in the constructor?  I'm for
> removing this change otherwise.

without the change onsocketwritable assumes that it will call trans->readsegments and readsegments will eventually call connection:::onreadsegment. That latter function sets mSocketOutCondition... after readsegments() has returned onSocketWritable uses the value of mSocketOutCondition if rv=OK. that assumption is incomplete.

in the general case its very hard to guarantee that every path through spdysession::readsegments will call connection::onraeadsegment - especially because the latter function ERRORs when sent a 0 sized buffer. It's much safer to simply initialize the variable each time (you can't just do it in the ctor because that means some iterations of this code would just inherit the value from the last complete trip through rather than necessarily the last call to readsegments)

I'm inclined to think the trunk HTTP code was actually buggy on this front before the spdy code was merged in. The first lines of nsHttpTransaction::ReadSegments say:

    if (mTransactionDone) {
        *countRead = 0;
        return mStatus;
    }

that's certaily possible to return OK, read = 0 without ever calling onreadsegment which means nsHttpConnection::OnSocketWritable is going to do the if (NS_FAILED(mSocketOutCondition)) check with mSocketOutCondition basically being undefined.

> 
> @@ +238,5 @@
> >  {
> > +    // Leave the timer in place if there are connections that potentially
> > +    // need management
> > +    if (mNumIdleConns || (mNumActiveConns && gHttpHandler->IsSpdyEnabled()))
> > +        return;
> 
> What happens when there are active SPDY connections and user drops the pref
> off?  You may then IMO cancel the timer and leave the active spdy
> connections open indefinitely or for an undefined amount of time.

you're right - but its not real impt imo. Primarily because 
1] pref changes are rare to start with
2] the timer will get started again by pretty much any normal http activity
3] normal sessions will get timed out by the server and we'll handle that as part of the normal state machine

to fix it probably requires keeping a counter of active spdy sessions. if you agree its low priority can you just file a bug?

> 
> @@ +482,5 @@
> > +    ent->mTestedSpdy = true;
> > +
> > +    if (!usingSpdy) {
> > +        if (ent->mUsingSpdy)
> > +            conn->DontReuse();
> 
> Explain this please in a comment.

I believe that was code leftover from a time when I couldn't get NPN to reliably negotiate. It isn't needed now. Thanks.


> @@ +919,5 @@
> > +            // Check to see if a pending transaction was dispatched with the
> > +            // coalesce logic
> > +            if (count != ent->mPendingQ.Length()) {
> > +                count = ent->mPendingQ.Length();
> > +                i = 0;
> 
> Where is the coalesce logic you refer here to?

Like the last one, I think you have identified another meaningless old relic. For a while during development the pending entries actually were moved off of one ent onto another one if a preferred entry was newly located - I believe that was the condition this check and fix was looking for. Now, more sensibly, they stay on the same ent but are dispatched onto the conn of the preferred entry. 

I'm pretty sure, but not 100% positive about that so I'm going to make the review-patch remove this logic but put in an assert that count == ent->pendingq.length() just to be sure. Life is full of silly asserts :)

> 
> @@ +1066,5 @@
> > +        // before considering idle http ones.
> > +        if (gHttpHandler->IsSpdyEnabled()) {
> > +            conn = GetSpdyPreferredConn(ent);
> > +            if (conn)
> > +                addConnToActiveList = false;
> 
> Why don't you just return here with this connection?
> 

it needs a ref added still which is done at the common return

> Are you planning some changes in the code bellow that would require
> processing even we already have a connection to dispatch to?

that whole path is significantly changed in the pipelining patches, so I'd rather keep the churn down to a dull roar.

> 
> It took a little time for me to understand this comment.  When I rephrase it
> says:
> 
much better. thanks.

> 
> @@ +1207,5 @@
> > +        LOG(("Spdy Dispatch Transaction via Activate(). Transaction host = %s,"
> > +             "Connection host = %s\n",
> > +             aTrans->ConnectionInfo()->Host(),
> > +             conn->ConnectionInfo()->Host()));
> > +        rv = conn->Activate(aTrans, caps, priority);
> 
> When you are doing this UsingSpdy() branching, wouldn't be nicer to directly
> call AddTransaction here and drop the mUsingSpdy branching in Activate then?
> 

I did that so there would be a consistent function to add a transaction to the connection, whether or not it was the first one.

but yes, it would be nice if the pipeline/spdy/need-npn/plain-http could all line up a little better. maybe a tracking bug marked enhancement would be useful there.

> @@ +1389,5 @@
> > +        return;
> > +
> > +    for (PRInt32 index = ent->mPendingQ.Length() - 1;
> > +         index >= 0 && conn->CanDirectlyActivate();
> > +         --index) {
> 
> for (PRInt32 index = ent->mPendingQ.Length();
>              index > 0 && conn->CanDirectlyActivate();) 
> {
>              --index;
> 

while stipulating they are both ugly, I actually prefer mine. The for() statement indicates more clearly which indicies will be referenced.


> @@ +1400,5 @@
> > +        ent->mPendingQ.RemoveElementAt(index);
> > +
> > +        nsresult rv2 = DispatchTransaction(ent, trans, trans->Caps(), conn);
> > +        NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv2), "Dispatch SPDY Transaction");
> > +        NS_RELEASE(trans);
> 
> I think you have to put the transaction back to the pending queue or close
> on failure here. 

so that failure path can't happen in the same sense that malloc can't fail. (i.e. it will die trying, probably on an allocation to queue itself) - thus the abort/assertion.
Comment 12 Patrick McManus [:mcmanus] 2012-01-19 14:52:32 PST
Created attachment 590005 [details] [diff] [review]
review comment updates for nshttp* pieces v0
Comment 13 Honza Bambas (:mayhemer) 2012-01-19 16:12:18 PST
Looooong comments...

(In reply to Patrick McManus [:mcmanus] from comment #6)
> SpdySession cpp/h
> 
> while testing ... Definitely a simplification.

Good catch.  I overlooked this.

> I'm 100% fine with it as long as its bounds checked. Adding the FIRST and
> LAST macros helps identify it as important.

Yes, please sanitize right before making the call.

> 
> > > +void
> > > +SpdySession::SetWriteCallbacks(nsAHttpTransaction *aTrans)
> > 
> > This method's name isn't quit telling me what it does. 
> 
> I know you don't care for it - but I really do and will just claim author
> privilege on the name. Having a WriteCallback set is pretty clear for me -
> when the associated socket is writable the write handler rooted at
> nsHttpConnection::OnSocketWritable will be called.

Not sure what you mean I shouldn't care about.

This is a review feedback, feedback of someone seeing the code the first time.  This name hit me several times I've been reading the patch.  The function name IMO should be ConditionalResumeSend, that is what the method actually does.  But yes, I don't care how the method is eventually gonna be named.

> one place. its not perfect, but its better.

Thanks, I'll check the patch.

> barring an actual problem I think this is of relatively low value and kind
> of a pain. Can you just file an enhancement bug instead?

That's OK, I can work on it.  The goal is to not repeat code, make it more readable (I misinterpreted the members few times) and make it more potential error prone.

> > Not sure this comment is correct.
> 
> it certainly is in the common case 

I might misplaced my comment here... sorry.

> https://groups.google.com/d/msg/spdy-dev/nm_bC-sbwCo/lSGlfi41jysJ

Aha.

> > I don't understand why is this call here.
> it calls resumesend()) so can we file a bug about dealing with it
> separately? 

Yep.

> i'm ok with providing both an ABORT and a opt-runtime-handling path for
> interfaces that might be called incorrectly but not for internal code state
> thing like this. That's not an error check - its an assertion of program
> state and doesn't require a runtime fallback.

Just to be sure not to run a code for a state that we are actually not in, the if () {} branch would be good.

> worth having - it also lets us coalesce things together when there is a
> legit WOULD_BLOCK event which is a nice thing.

Agree.

> I hear what you're saying - The nice thing about this particular usage is
> that the nsClassHashTable handles the deletion of the object when it is
> removed from the hash. So that helps some and some existing interfaces (e.g.
> CloseTransaction()) really make looking up by pointer a necessity.

I understand it is necessary to have this hash table.  It's not here just to find the stream when you have only the transaction.

However, I don't see the place where key is addrefed and released, also as per [1].  When tracing adding to the hash table in a debugger, I don't see transaction's ref be added.  So the key is not secured against becoming invalid when present in the hash table.

(In reply to Patrick McManus [:mcmanus] from comment #11)
> If you think mSocketTransport could be null somehow, then both opt and debug
> need a path to deal with it other than abort. but I don't think there is one.

Worth to add a check, just to avoid crashes.

> changing config state is generally a very rare thing to do - so I'll save
> the tiny bit of ram in this case rather than assume we're in a race
> condition.

Rare doesn't mean we don't have to handle it.  I agree to not collect the information, you are right about memory.  But the check has to be in both methods then.

> > > +        mSocketOutCondition = NS_OK;

I see.  Do you have any object to init the condition (maybe both) in the constructor?  My strong argument here is that when we change something, this may clear the bad socket condition.  And that is quit undesirable.

> > What happens when there are active SPDY connections and user drops the pref
> to fix it probably requires keeping a counter of active spdy sessions. if
> you agree its low priority can you just file a bug?

Yep.  But shouldn't be forgotten.

Actually I was thinking of having mSpdyConnections array in the connection manager.  Also with respect to my suggestion about creation of ssl and proxy'ed connections before getting to connection manager.

> but yes, it would be nice if the pipeline/spdy/need-npn/plain-http could all
> line up a little better. maybe a tracking bug marked enhancement would be
> useful there.

Agree, that might simplify few things.  This might be one of the architectural tweaks I wanted to suggest.

> > I think you have to put the transaction back to the pending queue or close
> > on failure here. 
> 
> so that failure path can't happen in the same sense that malloc can't fail.
> (i.e. it will die trying, probably on an allocation to queue itself) - thus
> the abort/assertion.

But things may change, worth to close or put the transaction back or do anything that will prevent hangs.  Don't want people report mysterious bugs like "one image didn't load", "my page loads indefinitely" etc that are hard to track down...


I'll try to get to the patches ASAP, best before the next merge, my r q is right now a bit long.  Also, you probably understand that spdy.enable = true is conditioned by having tests for it.


[1] http://hg.mozilla.org/mozilla-central/annotate/e5e66f40c35b/xpcom/glue/nsHashKeys.h#l232
Comment 14 Patrick McManus [:mcmanus] 2012-01-19 16:40:27 PST
(In reply to Honza Bambas (:mayhemer) from comment #13)
> > 
> > I know you don't care for it - but I really do and will just claim author

> Not sure what you mean I shouldn't care about.

Lost in translation :)

"don't care for it" roughly means a slightly softer version of "don't like it".. I wasn't saying you shouldn't care about it.

cheers!
Comment 15 Honza Bambas (:mayhemer) 2012-01-19 16:49:06 PST
(In reply to Patrick McManus [:mcmanus] from comment #14)
> (In reply to Honza Bambas (:mayhemer) from comment #13)
> > > 
> > > I know you don't care for it - but I really do and will just claim author
> 
> > Not sure what you mean I shouldn't care about.
> 
> Lost in translation :)
> 
> "don't care for it" roughly means a slightly softer version of "don't like
> it".. I wasn't saying you shouldn't care about it.
> 
> cheers!

ah, care FOR, not care ABOUT!  Now I understand, thanks :)
Comment 16 Patrick McManus [:mcmanus] 2012-01-24 12:50:41 PST
> However, I don't see the place where key is addrefed and released, also as
> per [1].  When tracing adding to the hash table in a debugger, I don't see
> transaction's ref be added.  So the key is not secured against becoming
> invalid when present in the hash table.
>

The SpdyStream (the value ptr in the element) holds a reference to the key/transaction and the spdy stream can only be destroyed by being removed from the hash table. That seems like a pretty reasonable relationship. I've made that more clear by making the spdystream dtor private/friend-to the nsAutoPtr used by the hash table implementation. I've also added a comment about how mTransaction is a necessary reference as long as the stream exists - but this relationship guarantees the key points to that stream as long as the stream is alive and the stream can only be destroyed by removing it from the hash table. so I think we're ok.
Comment 17 Patrick McManus [:mcmanus] 2012-01-24 14:15:04 PST
> 
> > > > +        mSocketOutCondition = NS_OK;
> 
> I see.  Do you have any object to init the condition (maybe both) in the
> constructor?  My strong argument here is that when we change something, this
> may clear the bad socket condition.  And that is quit undesirable.


I think it has to be cleared on every iteration of OnSocketWritable(), because there is no way of knowing whether or not the call to readsegments actually called OnReadSegment() or not.. so it can't tell if it is stale information.

it's basically being used like "errno" for ReadSegments(). You need to check errno after the system call returns - you can't assume its going to be in place later on.
Comment 18 Patrick McManus [:mcmanus] 2012-01-24 14:30:54 PST
honza, I'm going to update the 4 r? patches to reflect the comments you provided in c13.
Comment 19 Honza Bambas (:mayhemer) 2012-01-24 14:49:01 PST
(In reply to Patrick McManus [:mcmanus] from comment #17)
> > 
> > > > > +        mSocketOutCondition = NS_OK;
> > 
> > I see.  Do you have any object to init the condition (maybe both) in the
> > constructor?  My strong argument here is that when we change something, this
> > may clear the bad socket condition.  And that is quit undesirable.
> 
> 
> I think it has to be cleared on every iteration of OnSocketWritable(),
> because there is no way of knowing whether or not the call to readsegments
> actually called OnReadSegment() or not.. so it can't tell if it is stale
> information.
> 
> it's basically being used like "errno" for ReadSegments(). You need to check
> errno after the system call returns - you can't assume its going to be in
> place later on.

Let me put it a different way: the socket condition is determined by call to PR_Read, PR_Write, PR_Available.  Until that, with the current code on m-c, the socket condition is undetermined (and the variable holding it not initialized - which is not good, of course, and I was always wondering why and when it broke something). 

W/ the patch you may drop the condition to NS_OK from any previously detected error state - this my argument to do it a different way

If OnReadSegment() was NOT called during call to OnSocketWritable (i.e. one of PR_Read, PR_Write, PR_Available didn't get called) then the condition simply didn't change from the last time.

I think we now have to assume the _*initial*_ condition of the socket is NS_OK, and so that until we call one of the functions that determines the actual state.

About stale information: the condition of the socket goes only from OK to ERROR, never opposite, where OK means both OK or WOULD_BLOCK.  From ERROR, it never goes back, right?

Is it that you want to drop potential WOULD_BLOCK state to OK ?  Why would you do that?

Main question: will my proposal break spdy?
Comment 20 Honza Bambas (:mayhemer) 2012-01-24 14:50:06 PST
(In reply to Patrick McManus [:mcmanus] from comment #18)
> honza, I'm going to update the 4 r? patches to reflect the comments you
> provided in c13.

OK, I'll wait, I didn't start with the review of them anyway.  Thanks for a note.
Comment 21 Patrick McManus [:mcmanus] 2012-01-24 14:54:38 PST
Created attachment 591282 [details] [diff] [review]
spdy stream 1
Comment 22 Patrick McManus [:mcmanus] 2012-01-24 14:55:32 PST
Created attachment 591283 [details] [diff] [review]
spdy session review comment updates v1
Comment 23 Patrick McManus [:mcmanus] 2012-01-24 14:56:54 PST
Created attachment 591284 [details] [diff] [review]
frame writes should be atomic v1

the updates here are just bitrot - but since you haven't looked at it might as well bring it in sync with my hg queue
Comment 24 Patrick McManus [:mcmanus] 2012-01-24 14:57:42 PST
Created attachment 591285 [details] [diff] [review]
review comment updats for nshttp* pieces v1
Comment 25 Patrick McManus [:mcmanus] 2012-01-25 09:39:11 PST
> About stale information: the condition of the socket goes only from OK to
> ERROR, never opposite, where OK means both OK or WOULD_BLOCK.  From ERROR,
> it never goes back, right?
> 
> Is it that you want to drop potential WOULD_BLOCK state to OK ?  Why would
> you do that?

consider a case with streams A B and C. A and C want readsegments called.

stream A is called, it tries to write to network - gets a would block. msocketoutcondition = wouldblock.

streams A and B are canceled on the receive path via a RST_STREAM.

stream C is called - it returns NS_OK without trying to write to the network by calling OnReadSegment (maybe it is trying to form the request headers which the extension API allows to be streamed from the extension. but the extension stream blocked before they all came through).

mSocketOutCondition should not be would_block at this point.

> Main question: will my proposal break spdy?

i think so :)

I'm trying to see a case where mSocketOutCondition is meant to report some kind of general state rather than just being an out of band return value for OnReadSegments() specifically in OnSocketWritable() and I can't see one.
Comment 26 Honza Bambas (:mayhemer) 2012-01-25 11:01:40 PST
Comment on attachment 591282 [details] [diff] [review]
spdy stream 1

Review of attachment 591282 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: netwerk/protocol/http/SpdyStream.cpp
@@ +564,5 @@
>    NS_ABORT_IF_FALSE(mTxStreamFrameSize >= mTxStreamFrameSent,
>                      "negative unsent");
>    PRUint32 avail =  mTxStreamFrameSize - mTxStreamFrameSent;
>  
>    while (avail) {

while (avail && buf) ?

I'm still a bit worried to rely only on state of the stream frame status vs passed buffer argument.  If we make a mistake here, it might be caught in debug build with only low probability.
Comment 27 Honza Bambas (:mayhemer) 2012-01-25 11:02:13 PST
Comment on attachment 591283 [details] [diff] [review]
spdy session review comment updates v1

Review of attachment 591283 [details] [diff] [review]:
-----------------------------------------------------------------

This is probably the patch where the following comment should be addressed or at least commented on (maybe you did and I just missed that):

::: netwerk/protocol/http/SpdySession.cpp
@@ +923,5 @@
> +          "0x%X failed", self, streamID));
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +    
> +  self->ChangeDownstreamState(PROCESSING_CONTROL_RST_STREAM);

I don't much understand why you need a state to handle this kind of packet.

Actually, what you need to do is to cleanup the stream with given streamID and immediately stop sending any more data on the stream.  But with what you have written you have to wait for reading from the socket first before you actually reset the stream in WriteSegments, right?  It may take a while before it gets called again..


Otherwise r=honzab

::: netwerk/protocol/http/SpdySession.cpp
@@ +345,5 @@
>  
> +  // If the output queue is close to filling up and we have sent out a good
> +  // chunk of data from the beginning then realign it.
> +  
> +  if ((mOutputQueueSent >= 8192) &&

8192 should be kXXX, as the kQueueTailRoom is

@@ +418,5 @@
> +  // Leave a little slop on the new allocation - add 2KB to
> +  // what we need and then round the result up to a 4KB (page)
> +  // boundary.
> +
> +  objSize = (newSize + 2048 + 4095) & ~4095;

Nice :)

@@ +588,5 @@
>        }
> +
> +      // check for null characters
> +      if (*cPtr == '\0')
> +        return NS_ERROR_ILLEGAL_VALUE;

Tested?

@@ +852,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
>  SpdySession::HandleSynReply(SpdySession *self)

Like more statics with |self| then member function pointers?

@@ +1704,5 @@
> +  // This routine should not be allowed to fill up the output queue
> +  // all on its own - at least kQueueReserved bytes are always left
> +  // for other routines to use.
> +
> +  if ((mOutputQueueUsed + count) > (mOutputQueueSize - kQueueReserved)) {

Maybe add a comment to the constant that initialize mOutputQueueSize that it can never be smaller then kQueueReserved - that would lead here to an overflow.

@@ +1837,3 @@
>  
> +  mReadyForWrite.Push(stream);
> +  SetWriteCallbacks();

According how TransactionHasDataToWrite is called, this ResumeSend() called from SetWriteCallbacks() may be redundant.

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +82,3 @@
>  
> +    // After a connection has had ResumeSend() called by a transaction,
> +    // and it is ready to write to the network it it may need to know the

it it
Comment 28 Honza Bambas (:mayhemer) 2012-01-25 11:02:27 PST
Comment on attachment 591284 [details] [diff] [review]
frame writes should be atomic v1

Review of attachment 591284 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/SpdySession.cpp
@@ +784,1 @@
>      Close(NS_OK);

Why was |if (NS_FAILED(abortCode)) Close(abortCode);| removed?

@@ +1658,5 @@
> +      PRUint32 required = count - *countRead;
> +      // assuming a commitment() happened, this ensurebuffer is a nop
> +      EnsureBuffer(mOutputQueueBuffer, required, 0, mOutputQueueSize);
> +      memcpy(mOutputQueueBuffer.get(), buf + *countRead, required);
> +      mOutputQueueUsed = required;

This is why the r? is still pending:

A bit dangerous to assume mOutputQueueUsed is 0 here.  It depends on a not really simple and transparent logic of the output queue buffer management and CommitToSegmentSize implementation.  You could loose data.

Either don't call EnsureBuffer or use mOutputQueueUsed properly.

@@ +1698,5 @@
> +    return NS_OK;
> +  
> +  // if we are using part of our buffers already, try again later
> +  if (mOutputQueueUsed)
> +    return NS_BASE_STREAM_WOULD_BLOCK;

The |if ((mOutputQueueUsed + count) <= (mOutputQueueSize - kQueueReserved))| condition is tight to the EnsureBuffer implementation.  If it passes, it says EnsureBuffer will not reallocate.

This is a thing that should be encapsulated in the buffer class (bug 720818).

::: netwerk/protocol/http/SpdySession.h
@@ +160,5 @@
>  
>    // an overload of nsAHttpConnection
>    void TransactionHasDataToWrite(nsAHttpTransaction *);
>  
> +  // a similar version for SpdySession

s/SpdySession/SpdyStream/ ?

::: netwerk/protocol/http/SpdyStream.cpp
@@ +137,1 @@
>        mRequestBlockedOnRead = 1;

mTxInlineFrameUsed > 0 means all data needed to build the part of the frame are read from the transaction, right?  It means we now have a complete frame - it is just two state: == 0 -> frame construction still pending (need to collect the headers first), state > 0 -> frame constructed completely.

This should be added to the comment, it took a while for me to recall this again.

@@ +149,3 @@
>          rv = NS_BASE_STREAM_WOULD_BLOCK;
>        }
>      }

Maybe these two conditions could be under one if (!mTxInlineFrameUsed) { }.  Up to you.

@@ +801,5 @@
>        NS_ABORT_IF_FALSE(mTxInlineFrameUsed,
>                          "OnReadSegment SynFrameComplete 0b");
>        rv = TransmitFrame(nsnull, nsnull);
> +      NS_ABORT_IF_FALSE(NS_FAILED(rv) || !mTxInlineFrameUsed,
> +                        "Transmit Frame should be all or nothing");

Translated:
NS_ABORT_IF_TRUE(NS_SUCCEEDED(rv) && mTxInlineFrameUsed)

Yes, it makes sense now...
Comment 29 Honza Bambas (:mayhemer) 2012-01-25 11:02:34 PST
Comment on attachment 591285 [details] [diff] [review]
review comment updats for nshttp* pieces v1

Review of attachment 591285 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1601,5 @@
>      nsHttpConnectionInfo *ci = nsnull;
>  
>      if (!ent) {
>          // this should never happen
> +        NS_ABORT_IF_FALSE(false, "no connection entry");

I wanted NS_ERROR to have a log in release builds.  But probably not that important.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +206,4 @@
>          bool mUsingSpdy;
>  
> +        // mTestedSpdy is set after NPN negotiation has occurred and we know
> +        // with confidence where a host speaks spdy or not (which is reflected

s/where/whether/ ?
Comment 30 Patrick McManus [:mcmanus] 2012-01-25 13:18:34 PST
(In reply to Honza Bambas (:mayhemer) from comment #26)

> while (avail && buf) ?
> 

I did this instead:

   while (avail) {
-    NS_ABORT_IF_FALSE(buf, "Stream transmit with null buf argument to "
-                      "TransmitFrame()");
+    if (!buf) {
+      // this cannot happen
+      NS_ABORT_IF_FALSE(false, "Stream transmit with null buf argument to "
+                        "TransmitFrame()");
+      LOG(("Stream transmit with null buf argument to TransmitFrame()\n"));
+      return NS_ERROR_UNEXPECTED;
+    }
+
Comment 31 Patrick McManus [:mcmanus] 2012-01-25 13:36:18 PST
(In reply to Honza Bambas (:mayhemer) from comment #27)
> Comment on attachment 591283 [details] [diff] [review]
> spdy session review comment updates v1
> 
> Review of attachment 591283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is probably the patch where the following comment should be addressed
> or at least commented on (maybe you did and I just missed that):
> 
> ::: netwerk/protocol/http/SpdySession.cpp
> @@ +923,5 @@
> > +          "0x%X failed", self, streamID));
> > +    return NS_ERROR_ILLEGAL_VALUE;
> > +  }
> > +    
> > +  self->ChangeDownstreamState(PROCESSING_CONTROL_RST_STREAM);
> 
> I don't much understand why you need a state to handle this kind of packet.
> 
> Actually, what you need to do is to cleanup the stream with given streamID
> and immediately stop sending any more data on the stream.  But with what you
> have written you have to wait for reading from the socket first before you
> actually reset the stream in WriteSegments, right?  It may take a while
> before it gets called again..
> 

my reply that I repeat below [with new annotations in []] was buried in comment 6 - its easy to lose track of stuff in here!

> 
> @@ +923,5 @@
> > +          "0x%X failed", self, streamID));
> > +    return NS_ERROR_ILLEGAL_VALUE;
> > +  }
> > +    
> > +  self->ChangeDownstreamState(PROCESSING_CONTROL_RST_STREAM);
> 
> have written you have to wait for reading from the socket first before you
> actually reset the stream in WriteSegments, right? 

no [there is no delay], because NS_OK is returned to nsHttpConnection::OnSocketReadable, which will loop and call session::WriteSegments immediately [with the PROCESSING_CONTROL_RST_STREAM state].

[it could do the work inline as you suggest, but its nicer imo to colocate that code with the other processing_* functions]
Comment 32 Patrick McManus [:mcmanus] 2012-01-25 13:52:40 PST
> >        }
> > +
> > +      // check for null characters
> > +      if (*cPtr == '\0')
> > +        return NS_ERROR_ILLEGAL_VALUE;
> 
> Tested?

by hand, yes. maybe nick can add a xpcshell test for that and a few other illegal header things (upper case names, e.g.).. I'm not real up on node.js myself yet (though its on the todo list).

> 
> @@ +852,5 @@
> >    return NS_OK;
> >  }
> >  
> >  nsresult
> >  SpdySession::HandleSynReply(SpdySession *self)
> 
> Like more statics with |self| then member function pointers?
> 

it has a static c linkage because of the jump table I used for dispatch... but yeah, the self pointers don't bother me enough to wrap a c++ version in this and take the function call overhead.

> 
> @@ +1837,3 @@
> >  
> > +  mReadyForWrite.Push(stream);
> > +  SetWriteCallbacks();
> 
> According how TransactionHasDataToWrite is called, this ResumeSend() called
> from SetWriteCallbacks() may be redundant.
>

yep - you're right.
Comment 33 Patrick McManus [:mcmanus] 2012-01-25 14:22:30 PST
(In reply to Honza Bambas (:mayhemer) from comment #28)
> Comment on attachment 591284 [details] [diff] [review]
> frame writes should be atomic v1
> 
> Review of attachment 591284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/SpdySession.cpp
> @@ +784,1 @@
> >      Close(NS_OK);
> 
> Why was |if (NS_FAILED(abortCode)) Close(abortCode);| removed?

the only path that changed abortCode from its init'd value of NS_OK was when mPartialFrameSender was not null. And the point of this patch is to get rid of mPartialFrameSender completely :)

> 
> @@ +1658,5 @@
> > +      PRUint32 required = count - *countRead;
> > +      // assuming a commitment() happened, this ensurebuffer is a nop
> > +      EnsureBuffer(mOutputQueueBuffer, required, 0, mOutputQueueSize);
> > +      memcpy(mOutputQueueBuffer.get(), buf + *countRead, required);
> > +      mOutputQueueUsed = required;
> 
> This is why the r? is still pending:
> 
> A bit dangerous to assume mOutputQueueUsed is 0 here.

I disagree - we're inside this conditional  "if (!mOutputQueueUsed && mSegmentReader)"

The call to ensurebuffer isn't out of concern that outputqueuebuffer is non zero, its out of concern that the total size of the buffer is too small. although as the comment notes if the called made a call to commitment earlier on this can't happen. I'll update that comment.

>
>
> Translated:
> NS_ABORT_IF_TRUE(NS_SUCCEEDED(rv) && mTxInlineFrameUsed)
> 
> Yes, it makes sense now...

heh, yes. I tend to stick to NS_ABORT_IF_FALSE because it has clear semantics that match assert(3), which is a declarative paradigm that I like.
Comment 34 Honza Bambas (:mayhemer) 2012-01-25 14:32:20 PST
(In reply to Patrick McManus [:mcmanus] from comment #30)
> (In reply to Honza Bambas (:mayhemer) from comment #26)
> 
> > while (avail && buf) ?
> > 
> 
> I did this instead:
> 
>    while (avail) {
> -    NS_ABORT_IF_FALSE(buf, "Stream transmit with null buf argument to "
> -                      "TransmitFrame()");
> +    if (!buf) {
> +      // this cannot happen
> +      NS_ABORT_IF_FALSE(false, "Stream transmit with null buf argument to "
> +                        "TransmitFrame()");
> +      LOG(("Stream transmit with null buf argument to TransmitFrame()\n"));
> +      return NS_ERROR_UNEXPECTED;
> +    }
> +

That will do, thanks.

(In reply to Patrick McManus [:mcmanus] from comment #31)
> my reply that I repeat below [with new annotations in []] was buried in
> comment 6 - its easy to lose track of stuff in here!
> > have written you have to wait for reading from the socket first before you
> > actually reset the stream in WriteSegments, right? 
> 
> no [there is no delay], because NS_OK is returned to
> nsHttpConnection::OnSocketReadable, which will loop and call
> session::WriteSegments immediately [with the PROCESSING_CONTROL_RST_STREAM
> state].
> 
> [it could do the work inline as you suggest, but its nicer imo to colocate
> that code with the other processing_* functions]

Ah! yes, it seems better to leave the whole comment citation.  I was trying to search for pieces of text of the comment, but failed, and forgot or lost you your reply.

That is OK then.

(In reply to Patrick McManus [:mcmanus] from comment #33)
> > A bit dangerous to assume mOutputQueueUsed is 0 here.
> 
> I disagree - we're inside this conditional  "if (!mOutputQueueUsed &&
> mSegmentReader)"

Ha!  Yes, missed that.

> > Translated:
> > NS_ABORT_IF_TRUE(NS_SUCCEEDED(rv) && mTxInlineFrameUsed)
> > 
> > Yes, it makes sense now...
> 
> heh, yes. I tend to stick to NS_ABORT_IF_FALSE because it has clear
> semantics that match assert(3), which is a declarative paradigm that I like.

Sometimes I really need to translate for my self.  And I'm not the only one :)

Thanks for quick reply.
Comment 35 Honza Bambas (:mayhemer) 2012-01-25 14:32:38 PST
Comment on attachment 591283 [details] [diff] [review]
spdy session review comment updates v1

r=honzab
Comment 36 Honza Bambas (:mayhemer) 2012-01-25 14:32:51 PST
Comment on attachment 591284 [details] [diff] [review]
frame writes should be atomic v1

r=honzab
Comment 37 Patrick McManus [:mcmanus] 2012-01-25 14:36:17 PST
> 
> I wanted NS_ERROR to have a log in release builds.  But probably not that
> important.

I added a explicit log too just now.
Comment 38 Patrick McManus [:mcmanus] 2012-01-25 21:22:23 PST
4 checkins to inbound (see below). When merged to m-c this can be FIXED

changeset:   85410:50912fc18328
tag:         tip
user:        Patrick McManus <mcmanus@ducksong.com>
date:        Thu Jan 26 00:15:26 2012 -0500
summary:     bug 708415 spdy code review of nshttp* r=honzab

changeset:   85409:c0e2f40662cc
user:        Patrick McManus <mcmanus@ducksong.com>
date:        Thu Jan 26 00:15:26 2012 -0500
summary:     bug 708415 spdy make stream frame writes to session atomic r=honzab

changeset:   85408:d94542f096a4
user:        Patrick McManus <mcmanus@ducksong.com>
date:        Thu Jan 26 00:15:26 2012 -0500
summary:     bug 708415 spdysession review comments r=honzab

changeset:   85407:2fad8903e5f0
user:        Patrick McManus <mcmanus@ducksong.com>
date:        Thu Jan 26 00:15:26 2012 -0500
summary:     bug 708415 - spdystream review comments r=honzab

Note You need to log in before you can comment on or make changes to this bug.