Closed Bug 1355539 Opened 7 years ago Closed 7 years ago

Transaction may get queued and stuck despite "Found a speculative or a free-to-use half open connection" during its dispatch

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mayhemer, Assigned: kershaw)

References

Details

(Whiteboard: [necko-active])

Attachments

(4 files, 3 obsolete files)

Attached file parent process log
>	xul.dll!mozilla::net::nsHttpConnectionMgr::MakeNewConnection(0x0be1b340, 0x142583e0) Line 1216	C++
 	xul.dll!mozilla::net::nsHttpConnectionMgr::TryDispatchTransaction(0x0be1b340, false, 0x142583e0) Line 1485	C++
 	xul.dll!mozilla::net::nsHttpConnectionMgr::ProcessNewTransaction(0x13d2b000) Line 1736	C++
 	xul.dll!mozilla::net::nsHttpConnectionMgr::OnMsgNewTransaction(-20, 0x13d2b014) Line 2124	C++
 	xul.dll!mozilla::net::ConnEvent::Run() Line 214	C++


MakeNewConnection does:
  "Found a speculative or a free-to-use half open connection"
  pendingTransInfo->mHalfOpen =
                do_GetWeakReference(static_cast<nsISupportsWeakReference*>(ent->mHalfOpens[i]));
  return NS_OK;


TryDispatchTransaction returns NS_ERROR_NOT_AVAILABLE

ProcessNewTransaction handles that NS_ERROR_NOT_AVAILABLE by putting the transaction into a queue.


This can make a page load completely stuck.  I can reproduce this locally 100% with one of my testing profiles while opening a new tab.  New tab is set to default to show the top sites hence it tries to load a preview of a page I've been to before.  The load I can observe being stuck this way is a load for a preview.

No idea if this is a result of Bug 1341572.  tentatively blocking.

Snippet from the log:


2017-04-11 16:22:09.098000 UTC - [Socket Thread]: D/nsHttp nsHttpTransaction adding blocking transaction 150A3000 from request context 08E9AD30
2017-04-11 16:22:09.098000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::MakeNewConnection 08770D70 ent=09D92840 trans=150A3000
2017-04-11 16:22:09.098000 UTC - [Main Thread]: D/cache2 CacheEntry::RememberCallback [this=0BEBACC0, cb=07EB90C0, state=EMPTY]
2017-04-11 16:22:09.098000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::MakeNewConnection [ci = .SA....getfedora.org:443^userContextId=5]
Found a speculative or a free-to-use half open connection
2017-04-11 16:22:09.098000 UTC - [Main Thread]: D/cache2 CacheEntry::Load [this=0BEBACC0, trunc=0]
2017-04-11 16:22:09.098000 UTC - [Socket Thread]: V/nsHttp    dispatched step 4 (async new conn) trans=150A3000
2017-04-11 16:22:09.098000 UTC - [Main Thread]: D/cache2   already loaded
2017-04-11 16:22:09.098000 UTC - [Socket Thread]: V/nsHttp   adding transaction to pending queue [trans=150A3000 pending-count=4]
2017-04-11 16:22:09.098000 UTC - [Main Thread]: D/cache2 CacheEntry::InvokeCallbacks BEGIN [this=0BEBACC0]
2017-04-11 16:22:09.098000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::nsConnectionEntry::InsertTransaction trans=150A3000, windowId=4294967297


Since then, there is nothing else happening with the transaction until it's canceled via cancellation of the docshell loading the preview (from the child process, by timeout).

Optimized build based on m-c@2a3ecdb7d1ea, no local patches.
Flags: needinfo?(mcmanus)
Can you give me a longer log, I need to figure out what is happening with halfOpen. halfOpen should timeout, actually socket should timeout or backup socket should be ready.
Flags: needinfo?(honzab.moz)
(In reply to Dragana Damjanovic [:dragana] from comment #1)
> Can you give me a longer log, I need to figure out what is happening with
> halfOpen. halfOpen should timeout, actually socket should timeout or backup
> socket should be ready.

I'm afraid there is nothing more to log, it's complete and there should also be visible that the load group is canceled in about 1 and half minutes.

But I can run again till the browser shutdown and upload the complete set of logs.
Flags: needinfo?(honzab.moz)
sorry, I have not seen the log attached.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Attached file logs.tar.xz
Here is a complete set again.  You can filter by CI key ".SA....getfedora.org:443^userContextId=5" in the parent process log to find operation on the preview load.

Example stuck transactions: 07EB1400, 0802CC00 and probably more.
nsHalfOpenSocket 0E2926B0 is the socket transaction 07EB1400 is waiting for.

The socket is ready:
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHalfOpenSocket::OnOutputStreamReady [this=0E2926B0 ent=getfedora.org primary]
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::ConditionallyStopTimeoutTick armed=1 active=7
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp Creating nsHttpConnection @080BA5D0
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHalfOpenSocket::OnOutputStreamReady Created new nshttpconnection 080BA5D0
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHttpConnection::Init this=080BA5D0
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHalfOpenSocket::OnOutputStreamReady no transaction match returning conn 080BA5D0 to pool

The socket had a nulltransaction associated with it (it was a speculative connect) so it is return to the pool and there is where something goes wrong.

We call OnMsgReclaimConnection, this should go through the pending queue and find a transaction.

2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::OnMsgReclaimConnection [conn=080BA5EC]
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHttpConnection::SetupSSL 080BA5D0 caps=0x411 .SA....getfedora.org:443^userContextId=5
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHttpConnection::InitSSLParams [this=080BA5D0] connectingToProxy=0
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHttpConnection::GetSecurityInfo trans=00000000 tlsfilter=00000000 socket=07FB0410
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHttpConnection::SetupSSL Allow SPDY NPN selection
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: I/nsHttp Http2Session::ALPNCallback sslsocketcontrol=083B2364
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: I/nsHttp Http2Session::ALPNCallback version=304
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp nsHttpConnection::SetupNPNList 080BA5D0 0
2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp InitSSLParams Setting up SPDY Negotiation OK
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp   adding connection to idle list
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp nsHttpConnection::BeginIdleMonitoring [this=080BA5D0]
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp Entering Idle Monitoring Mode [this=080BA5D0]
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::OnMsgProcessPendingQ [ci=.SA....getfedora.org:443^userContextId=5]


This is function that should search for a transaction:

2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::ProcessPendingQForEntry [ci=.SA....getfedora.org:443^userContextId=5 ent=07A4EF00 active=1 idle=1 urgent-start queue=0queued=46]

We have 46 transaction in the pending queue!



2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp FindCoalescableConnection .SA....getfedora.org:443^userContextId=5
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp FindCoalescableConnection(.SA....getfedora.org:443^userContextId=5) no matching conn
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp GetSpdyActiveConn() request for ent 07A4EF00 .SA....getfedora.org:443^userContextId=5 did not find an active connection
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::AvailableNewConnectionCount total connection count = 5, limit 6


From code the focus window does not have any transaction!!! So we go to Non-focus windows.



2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp nsConnectionEntry::AppendPendingQForNonFocusedWindows [ci=.SA....getfedora.org:443^userContextId=5], pendingQ count=1 for non focused window



And we end up with only 1 transaction!!!! and that transaction is blocked on 9 other resources! And we end up our search.  I think the search through transaction is brocken therefore no all of transaction get dispatch.
Regression of bug 1312782.


2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::ProcessPendingQForEntry [trans=083CA400, halfOpen=00000000, activeConn=00000000]
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::TryDispatchTransaction without conn [trans=083CA400 halfOpen=00000000 conn=00000000 ci=07A89800 ci=.SA....getfedora.org:443^userContextId=5 caps=31 tunnelprovider=00000000 onlyreused=0 active=1 idle=1]
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp FindCoalescableConnection .SA....getfedora.org:443^userContextId=5
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp FindCoalescableConnection(.SA....getfedora.org:443^userContextId=5) no matching conn
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp GetSpdyActiveConn() request for ent 07A4EF00 .SA....getfedora.org:443^userContextId=5 did not find an active connection
2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp    blocked by request context: [rc=07361610 trans=083CA400 blockers=9]
Flags: needinfo?(mcmanus)
Blocks: 1312782
No longer blocks: 1341572
We are not switching to Aurora next week?
This needs to be fix before going to next channel!!!!

Kershaw, do you have time to work on this?
Flags: needinfo?(kechang)
(In reply to Dragana Damjanovic [:dragana] from comment #6)
> We are not switching to Aurora next week?
> This needs to be fix before going to next channel!!!!
> 
> Kershaw, do you have time to work on this?

Yes, I can work on this.

Honza, could you provide the steps of how to reproduce this to me? Thanks.
Flags: needinfo?(kechang)
Flags: needinfo?(honzab.moz)
(In reply to Dragana Damjanovic [:dragana] from comment #5)
> nsHalfOpenSocket 0E2926B0 is the socket transaction 07EB1400 is waiting for.
> 
> The socket is ready:
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHalfOpenSocket::OnOutputStreamReady [this=0E2926B0 ent=getfedora.org
> primary]
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnectionMgr::ConditionallyStopTimeoutTick armed=1 active=7
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp Creating
> nsHttpConnection @080BA5D0
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHalfOpenSocket::OnOutputStreamReady Created new nshttpconnection 080BA5D0
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnection::Init this=080BA5D0
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHalfOpenSocket::OnOutputStreamReady no transaction match returning conn
> 080BA5D0 to pool
> 
> The socket had a nulltransaction associated with it (it was a speculative
> connect) so it is return to the pool and there is where something goes wrong.
> 
> We call OnMsgReclaimConnection, this should go through the pending queue and
> find a transaction.
> 
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnectionMgr::OnMsgReclaimConnection [conn=080BA5EC]
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnection::SetupSSL 080BA5D0 caps=0x411
> .SA....getfedora.org:443^userContextId=5
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnection::InitSSLParams [this=080BA5D0] connectingToProxy=0
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnection::GetSecurityInfo trans=00000000 tlsfilter=00000000
> socket=07FB0410
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnection::SetupSSL Allow SPDY NPN selection
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: I/nsHttp
> Http2Session::ALPNCallback sslsocketcontrol=083B2364
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: I/nsHttp
> Http2Session::ALPNCallback version=304
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnection::SetupNPNList 080BA5D0 0
> 2017-04-12 09:50:55.870000 UTC - [Socket Thread]: V/nsHttp InitSSLParams
> Setting up SPDY Negotiation OK
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp   adding
> connection to idle list
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnection::BeginIdleMonitoring [this=080BA5D0]
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp Entering Idle
> Monitoring Mode [this=080BA5D0]
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnectionMgr::OnMsgProcessPendingQ
> [ci=.SA....getfedora.org:443^userContextId=5]
> 
> 
> This is function that should search for a transaction:
> 
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnectionMgr::ProcessPendingQForEntry
> [ci=.SA....getfedora.org:443^userContextId=5 ent=07A4EF00 active=1 idle=1
> urgent-start queue=0queued=46]
> 
> We have 46 transaction in the pending queue!
> 
> 
> 
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp
> FindCoalescableConnection .SA....getfedora.org:443^userContextId=5
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp
> FindCoalescableConnection(.SA....getfedora.org:443^userContextId=5) no
> matching conn
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp
> GetSpdyActiveConn() request for ent 07A4EF00
> .SA....getfedora.org:443^userContextId=5 did not find an active connection
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp
> nsHttpConnectionMgr::AvailableNewConnectionCount total connection count = 5,
> limit 6
> 
> 
> From code the focus window does not have any transaction!!! So we go to
> Non-focus windows.
> 
> 
> 
> 2017-04-12 09:50:55.874000 UTC - [Socket Thread]: V/nsHttp
> nsConnectionEntry::AppendPendingQForNonFocusedWindows
> [ci=.SA....getfedora.org:443^userContextId=5], pendingQ count=1 for non
> focused window
> 

I think the culprit is at |nsConnectionEntry::AppendPendingQForNonFocusedWindows|. It gets transactions from pendingQ according to the current available connection count, which is only 1 in this case, and the manner of selecting transactions is simply based on the transaction's priority. So, if the selected transaction is not 07EB1400, it will be blocked to dispatch.

I think we should modify the behavior of |AppendPendingQForNonFocusedWindows|. Maybe always get the blocking transaction first?

What do you think, Honza?
First, Dragana - great thanks for diagnosing this!

So, what I missed during the review was that we have to handle blocking and load-unblocked transactions with priority, right?  That definitely must be in the selection algorithm, yes - that is the fix here.

I'm also thinking, also because this goes to Aurora soon and we rather want to be on the save side, to temporarily allow more than only 1 connection utilization for background tabs till we find a smarter logic here (needs a broader discussion).  This means to also revert the requirements in bug 1312782 comment 32 as part of the patch.

Thanks!

STR:
- m-c @ 2a3ecdb7d1ea, an optimized build
- link to the zipped profile sent privately to Kershaw
- start the browser
- open a new tab (Ctrl-T), this starts a preview load for getfedora.org, it will never finish

A test for this would be good to have, but it's definitely NOT a requirement for the fix, since it may not be easy to write such a test quickly.
Flags: needinfo?(honzab.moz)
Assignee: dd.mozilla → kechang
last I heard 55 will not be going to aurora next week because aurora will be removed next week. so 55 will be around as nightly for another 6 weeks. (There's always a chance my info is outdated :))

so next week 57->release, 56->beta, 55 stays on nightly for another 6 weeks.

This means we are going to need to take newfound care with when we land things on nightly as some landings will go to beta very very quickly.
(In reply to Honza Bambas (:mayhemer) from comment #9)
> First, Dragana - great thanks for diagnosing this!
> 
> So, what I missed during the review was that we have to handle blocking and
> load-unblocked transactions with priority, right?  That definitely must be
> in the selection algorithm, yes - that is the fix here.
> 

Yes. That means we have to dispatch transactions with NS_HTTP_LOAD_AS_BLOCKING or NS_HTTP_LOAD_UNBLOCKED first, so other transactions could get a chance to be dispatched.

In current code, we append transactions into a temporary queue sorted by priority for dispatching. To dispatch blocking transactions first, the temporary queue has to be split into two parts: first part includes blocking transactions and the second part contains the rest transactions. It would look like this: [blocking transactions...|other transactions...].
If we only want to change the selection algorithm to do this, the algorithm itself would become very complicated and slow, because the current structure of pending transactions is a simple hash table (<WindowId : transactions-by-priority[]>).

I think maybe we want to modify the structure as:

<WindowId : TransactionTable>

TransactionTable
{
  // Transaction queue for transactions that could be blocked because the blocker count in request context is not 0.
  blockedTransactions [];

  // Transaction queue for transactions with NS_HTTP_LOAD_AS_BLOCKING or NS_HTTP_LOAD_UNBLOCKED flag.
  // These transactions should not be blocked by others and should be dispatched as soon as possible.
  nonBlockedTransactions [];
}

@Honza, I'd like to hear your feedback first, because this looks like a big change. If this sounds like a plan to you, I even want to also put urgent-start queue into TransactionTable. So, the code could become more clear and also reduce some duplicate code.
Flags: needinfo?(honzab.moz)
Summary:
 - Modify |InsertTransactionSorted| to put blocking transactions in the front of pendingQ.
Flags: needinfo?(honzab.moz)
Attachment #8859947 - Flags: review?(honzab.moz)
Comment on attachment 8859947 [details] [diff] [review]
Insert blocking transactions in the front of pendingQ

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

sorry for such a late answer, still getting up from the taipei meeting.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +49,5 @@
> +HasBlockingOrUnblockedFlag(nsHttpTransaction *trans)
> +{
> +    uint32_t caps = trans->Caps();
> +    return (caps & NS_HTTP_LOAD_AS_BLOCKING) || (caps & NS_HTTP_LOAD_UNBLOCKED);
> +}

I'd rather see a function like:

ComparePriority(nsHttpTransaction *t1, *t2)
{
  bool t1blocking = t1->caps & (NS_HTTP_LOAD_AS_BLOCKING | NS_HTTP_LOAD_UNBLOCKED);
  bool t2blocking = t2->caps & (NS_HTTP_LOAD_AS_BLOCKING | NS_HTTP_LOAD_UNBLOCKED);
  
  if (t1blocking > t2blocking) return 1;
  if (t1blocking < t2blocking) return -1;
  
  return t1->Priority() - t2->Priority();
}

and use that in InsertTransactionSorted instead of just plain trans->Priority() >= t->Priority().

the above code is an outline of the idea, please make sure the signs are correct ;)

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +226,5 @@
>          // This table provides a mapping from top level outer content window id
> +        // to a queue of pending transaction information. The pending queue is
> +        // comprised of two groups of transactins: transactions with
> +        // NS_HTTP_LOAD_AS_BLOCKING or NS_HTTP_LOAD_UNBLOCKED flags are
> +        // in the first group, and the rest of transactions are in the second group.

seems outdated?
Attachment #8859947 - Flags: review?(honzab.moz) → feedback+
Summary:
 - Use another function to compare transaction's order
 - Give the rest available connections to background tab when processing pendingQ for all entries.

Thanks.
Attachment #8859947 - Attachment is obsolete: true
Attachment #8862849 - Flags: review?(honzab.moz)
Attached patch Part2: Test case (obsolete) — Splinter Review
Summary:
 - A test case to test the order of pending transactions
Attachment #8862850 - Flags: review?(honzab.moz)
Attachment #8862849 - Flags: review?(honzab.moz) → review+
Comment on attachment 8862850 [details] [diff] [review]
Part2: Test case

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

disclaimer: haven't tested locally.

::: netwerk/test/unit/test_bug1355539_http1.js
@@ +14,5 @@
> +// 3. Server starts to process the 6 dummy http requests, so the client can start to
> +//    process the pending queue. Server will queue those http requests and put them in
> +//    |responseQueue|.
> +// 4. When the server receive all 6 requests, check if the order in |responseQueue| is
> +//    equal to |transactionQueue| by comparing the value of X-ID.

thanks!

@@ +82,5 @@
> +
> +function setup_HttpRequests() {
> +  log("setup_HttpRequests");
> +  // Create channels in reverse order
> +  for (var i = transactionQueue.length - 1; i > -1; i--) {

nit:

for (var i = transactionQueue.length - 1; i > 0;) {
  --i;
  do stuff;
}
Attachment #8862850 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #16)
> Comment on attachment 8862850 [details] [diff] [review]
> Part2: Test case
> 
> Review of attachment 8862850 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> disclaimer: haven't tested locally.
> 
> ::: netwerk/test/unit/test_bug1355539_http1.js
> @@ +14,5 @@
> > +// 3. Server starts to process the 6 dummy http requests, so the client can start to
> > +//    process the pending queue. Server will queue those http requests and put them in
> > +//    |responseQueue|.
> > +// 4. When the server receive all 6 requests, check if the order in |responseQueue| is
> > +//    equal to |transactionQueue| by comparing the value of X-ID.
> 
> thanks!
> 
> @@ +82,5 @@
> > +
> > +function setup_HttpRequests() {
> > +  log("setup_HttpRequests");
> > +  // Create channels in reverse order
> > +  for (var i = transactionQueue.length - 1; i > -1; i--) {
> 

Thanks for the review!

> nit:
> 
> for (var i = transactionQueue.length - 1; i > 0;) {
>   --i;
>   do stuff;
> }

I think this should be:

for (var i = transactionQueue.length - 1; i > -1;) {
  do stuff;
  --i;
}
Carry reviewer's name.
Attachment #8862849 - Attachment is obsolete: true
Attachment #8862850 - Attachment is obsolete: true
Attachment #8863613 - Flags: review+
Carry reviewer's name.
Attachment #8863614 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce25dc4382d4
Insert blocking transactions in the front of pendingQ, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd5c1cfd9fa
Test case, r=mayhemer
Keywords: checkin-needed
(In reply to Kershaw Chang [:kershaw] from comment #17)
> I think this should be:
> 
> for (var i = transactionQueue.length - 1; i > -1;) {
>   do stuff;
>   --i;
> }

No.  As you write it it's equal to for (var i = transactionQueue.length - 1; i > -1; --i)

I'm sorry, I meant to write it as:

for (var i = length; i > 0;) {
  --i;
  do stuff;
}

But whatever, you've already land that anyway.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: