1312774 - Allow channels marked as "urgentStart" to bypass connection limits

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

In general this means to implement the "A" slot as described in the CDP tracking bug's document [1].

[1] https://docs.google.com/document/d/1FWpLUOhR4cuXn-hB3xy6aXkDfp_aZxklG0TWTEnUU4U/edit

Comment 1

[Honza Bambas (:mayhemer)]

Amy, as I heard you want to work on this, I'm assigning the bug to you now.

The main change here will probably happen in nsHttpConnectionMgr::TryDispatchTransaction or around.

Feel free to ask whatever questions come up, the more the better.  I have to take a look around this code myself to be a good adviser, it's rather complicated ;)

Other good source of information and directions here is Patrick.  He was doing larger changes here lately and he may also point at some of the HTTP/2 (h2) specifics that may not be obvious.

Also note that there is a lot of code that is specific to HTTP/1.1 pipelining in the areas you will probably modify here.  We never fully enabled that code and Patrick was recently talking about removing it.

Hence, if the pipelining code would block implementing this bug, it's probably OK to either break it or even remove some of its portions.

Comment 2

[Amy Chung [:Amy]]

Hi Honza,
I have some views and questions on this bug as below:
1. According context-type (audio/mp4 or image/jpeg) to set ClassOfService in HttpBaseChannel.cpp (Using nsHttpChannel::SetClassFlags).
2. For the situation "4 - If we aren't up against our connection limit, then open a new one", if the class flag is urgentStart, ignore the number of connections limit and directly add a new connection for urgentStart on nsHttpConnectionMgr::TryDispatchTransaction.

Do I have to implement step1? if I need to implement it, and what does the situation of downloading must to set ClassOfService to urgentStart?
furthermore, does the step2 meet the requirements of this bug?

Please give me your suggestions, thanks!

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Amy Chung [:Amy] from comment #2)
> Hi Honza,
> I have some views and questions on this bug as below:
> 1. According context-type (audio/mp4 or image/jpeg) to set ClassOfService in
> HttpBaseChannel.cpp (Using nsHttpChannel::SetClassFlags).

I think this bug is more about handling the urgentStart flag, not about setting it.  I think setting the flag for particular cases (a video/audio tag, object embedding, downloads, anything that is originated by a user interaction - like image zoom) should be fixed in separate respective bugs yet to be filed (please, feel free to file them).  Probably finding the spots to set the flag will be more complicated than the core Necko changes in this bug.

In general, I think the more proper way is to try to find out where from the load is coming.  E.g, to demonstrate it's not always that simple: when there is a video tag, we may preload few seconds of the video before the user clicks the play button.  That preload request though should not be set the urgentStart flag to not interfere with other portions of the page currently loading that are probably more important to reach meaningful paint.  I don't think the channel should try to find out if it should be 'urgent' based on e.g. content type, but should be told from above during it's creation (or, before it's AsyncOpened).

Other cases may be more complicated.  E.g. image zoom may not be that simple, since there can be a lot of JS code involved and the knowledge that a certain load is actually originated by a very recent user interaction may be pretty complicated.

I think each of the cases for which you want to set the urgentStart flag should have its own bugs.  Some of them might turn to be more complicated than beneficial and we may just rather WONTFIX them or postpone.



> 2. For the situation "4 - If we aren't up against our connection limit, then
> open a new one", if the class flag is urgentStart, ignore the number of
> connections limit and directly add a new connection for urgentStart on
> nsHttpConnectionMgr::TryDispatchTransaction.

Yes.  That should be it, but you also have to make sure that the "urgentStart" transaction will be picked from the pending transaction queue as the first when either an existing connection becomes available or the new connection is set up.  

Also, we may want to have some other, broader limit for the "A" slot.  It simply should be behind a preference.

> 
> Do I have to implement step1? 

As above, and IIUC, that should be split to separate bugs.

> if I need to implement it, and what does the
> situation of downloading must to set ClassOfService to urgentStart?

I usually put a break point to HttpChannelChild::AsyncOpen to find out where from it's being triggered.  Maybe do the same for a download and you will quickly find out where you have to set the class flag.  If you are called from javascript, you may use DumpJSStack() function evaluation from the paused debugger.  It will show you the current JS stack.

> furthermore, does the step2 meet the requirements of this bug?

I could imagine a simple js (unit) test to confirm:
- have a http response handler that handles the response asynchronously (and actually never sends a response)
- when you make a limit+1 http requests (open that number of http channels) your http response handler should be called only for |limit| number of times
- the limit+1 request will be queued
- now, when you open another channel and set it the urgentStart flag, you must hit your handler this time

and that should do as an automated test and confirmation of whatever necko changes you do here are correct or on the right path.

> 
> Please give me your suggestions, thanks!

Comment 4

[Amy Chung [:Amy]]

Attachment: Test case

Hi Honza,
I create a unit test for this bug, and the function and feature work as below:
[current function]
1. Created 7 http requests.
2. Confirmed 7 requests have processed to finish (Cr=NS_OK and print msg from nsHttpTransaction::~nsHttpTransaction() to check the transaction already prcoessed.)

[feather work]
I will add a new http request which set to urgentStart after finishing to implement the code.

Please help me to confirm my patch of unit test and feature work which fit the expectation of this bug.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8814855 [details] [diff] [review]
Test case

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

::: netwerk/test/unit/test_httpResponseHandler.js
@@ +1,5 @@
> +/* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: i think we don't license tests

@@ +10,5 @@
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +
> +var server = new HttpServer();
> +var baseURL = "http://localhost:" + server.identity.primaryPort + "/";
> +server.start(-1);

worth a check, bug I think that "server.identity.primaryPort" is not defined before you do "server.start(-1)".  the start() method of the server binds it to a random port and only then primaryPort is known.  but I may be mistaken.  anyway, double check this.

@@ +31,5 @@
> +function setup_tests() {
> +  var prefs= Cc["@mozilla.org/preferences-service;1"]
> +                            .getService(Ci.nsIPrefBranch);
> +  let maxConnections = prefs.getIntPref("network.http.max-persistent-connections-per-server");
> +  for (var i = 0; i < maxConnections + 1; i++) {

maybe, to save some resources, change the preference to something less then 6.  but that's entirely optional.

@@ +62,5 @@
> +    // Wait until the timeout should have passed, then respond.
> +    response.processAsync();
> +    response.setStatusLine(metadata.httpVersion, 200, "OK");
> +    response.write("Hello world");
> +    response.finish();

ok, here you have to add either a timeout, or implement a "manual trigger" that will do write() + finish() on the response.  but it's already stated in the comment just above.

generally this looks good, you are on the right path!

probably also add Cache-control: no-cache response header to make sure that all the requests made to the same URL will all go to the server.

Comment 6

[Amy Chung [:Amy]]

Hi Patrick,
In nsHttpConnectionMgr::ProcessPendingQForEntry(), this function process Spdy transaction first.
Do we have to set the highest priority to the urgentStart transaction for would be able to process the urgentStart transaction before spdy transaction?
And can the urgentStart transaction always be processed before spdy transaction by nsHttpConnectionMgr?

Comment 7

[Patrick McManus [:mcmanus]]

ProcessSpdyPendingQ() just says dispatch any transactions onto h2 that can be done without making a new connection. That's a resaonable disposition for an urgent start transaction - you might want to make a version that only considers urgent start ones so that you don't fill up the parallel transaction limit counter on h2 (which is typically 100 - but is set by the server) with non urgent ones.

If you have an urgent one that cannot be dispatched this way, you'll need to open a new connection for it.

Comment 8

[Amy Chung [:Amy]]

Attachment: bug-1312774.patch

Hi Honza,
I divided this patch to 4 part.
1. implement as the last dissuasion.
2. test cases
    i. h1 test cases
    ii.h2 test cases
3. i.   Set a pref "network.max-urgentstart-queue" for the maximum of urgentStart queue (default set to 10, I want to ask a question, What is the  longest waitting time of the urgentStart transcation?)
   ii.  If urgentStart queue len >= network.max-urgentstart-queue
   iii. doesn't let other transactions to create connection. 
4. Added priority when setting NS_HTTP_URGENT_START.

Finished: 
1, 2i, 3, 4.

Not Finish:
2ii.

Please help me to confirm my patch, thanks!

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8818872 [details] [diff] [review]
bug-1312774.patch

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

I think it's on a good way, see the comments.

::: modules/libpref/init/all.js
@@ +1437,5 @@
>  // a new connection will only be attempted if the number of active persistent
>  // connections to the server is less then max-persistent-connections-per-server.
>  pref("network.http.max-persistent-connections-per-server", 6);
>  
> +pref("network.max-urgentstart-queue", 10);

you'll need a comment for this (can be in the very last version of the patch)

::: netwerk/protocol/http/nsHttp.h
@@ +87,5 @@
>  // Transactions with this flag should react to errors without side effects
>  // First user is to prevent clearing of alt-svc cache on failed probe
>  #define NS_HTTP_ERROR_SOFTLY         (1<<10)
>  
> +// Transactions with this flag should process first.

nit: should be processed

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +887,5 @@
> +      pendingQ = &(ent->mUrgentStartQ);
> +    } else if (ent->mPendingQ.Length() > 0) {
> +      pendingQ = &(ent->mPendingQ);
> +    } else {
> +      dispatchedSuccessfully = true;

I think you want to go with false here?

maybe just |return false| here?

@@ +1089,5 @@
> +
> +    if (ent->mUrgentStartQ.Length() >= mMaxUrgentStartQ) {
> +       LOG((" urengt start Q len == max urgent start queue len "));
> +       return true;
> +    }

OK, here I'm not sure how this should work.  My understanding of the preference is that we allow only 10 connections to be created by urgent start transactions.  If there is 11th urgent start transaction added, it's just added to the queue and doesn't allow opening a new connection.

To speak by code, I think this should look more like:

if (caps & NS_HTTP_URGENT_START) {
   if (ent->mUrgentStartQ.Length() >= mMaxUrgentStartQ) {
       LOG((" urengt start Q len == max urgent start queue len "));
       return true;
   }
   return false;
}

or am I mistaken?

@@ +2265,5 @@
>      nsConnectionEntry *ent = LookupConnectionEntry(trans->ConnectionInfo(),
>                                                     nullptr, trans);
>  
>      if (ent) {
> +        int32_t caps = trans->Caps();

uint32_t (see declaration at https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/netwerk/protocol/http/nsAHttpTransaction.h#63)

@@ +2266,5 @@
>                                                     nullptr, trans);
>  
>      if (ent) {
> +        int32_t caps = trans->Caps();
> +        nsTArray<RefPtr<nsHttpTransaction> > pendingQ;

nit: in a new code there is no need to do '> >' (with space).  now every toolkit we build on is OK with just '>>' on nested template args.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +293,5 @@
>          explicit nsConnectionEntry(nsHttpConnectionInfo *ci);
>          ~nsConnectionEntry();
>  
>          RefPtr<nsHttpConnectionInfo> mConnInfo;
> +        nsTArray<RefPtr<nsHttpTransaction> > mUrgentStartQ; // pending the urgent start transaction queue

nit: // the urgent start transaction queue

OK, so you decided to go with a separate array (queue) for the urgent-start marked transactions.

Then you will have to go over all references to mPendingQ in nsHttpConnectionMgr.cpp and check if you also need to add handling of your new mUrgentStartQ next to existing handling of mPendingQ.

There are number of such places.  I hope it won't turn to be too over-complicated.

::: netwerk/test/unit/test_bug1312774.js
@@ +85,5 @@
> +    do_check_eq(status, Cr.NS_OK);
> +    leastRequests--; 
> +    if (cos.classFlags & Ci.nsIClassOfService.UrgentStart) {
> +      do_check_eq(leastRequests, 1);
> +    }

can you please describe a bit how this test exactly works? it's not very clear from the code.

note that httpd.js doesn't support keep-alive connections :/  (bug 469228).

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Amy Chung [:Amy] from comment #8)
> Created attachment 8818872 [details] [diff] [review]
> bug-1312774.patch
> 
> Hi Honza,
> I divided this patch to 4 part.
> 1. implement as the last dissuasion.
> 2. test cases
>     i. h1 test cases
>     ii.h2 test cases
> 3. i.   Set a pref "network.max-urgentstart-queue" for the maximum of
> urgentStart queue (default set to 10, I want to ask a question, What is the 
> longest waitting time of the urgentStart transcation?)

Not sure what you mean.  But I think indefinite.  If we are on the limit of 10 connections being opened because of urgent-start transactions, then we simply have to wait until a connection is available.  We simply can't do more to get an immediate response for everything.

Does it answer your question?

>    ii.  If urgentStart queue len >= network.max-urgentstart-queue
>    iii. doesn't let other transactions to create connection. 
> 4. Added priority when setting NS_HTTP_URGENT_START.
> 
> Finished: 
> 1, 2i, 3, 4.
> 
> Not Finish:
> 2ii.
> 
> Please help me to confirm my patch, thanks!

Comment 11

[Amy Chung [:Amy]]

Attachment: bug-1312774-v2.patch

Hi Honza,
I have modified the patch from your suggestions as below:
[modules/libpref/init/all.js]
1.  Added comment
[netwerk/protocol/http/nsHttp.h]
1. nit
[netwerk/protocol/http/nsHttpConnectionMgr.cpp]
1. Modified “dispatchedSuccessfully = true;” to “return false” in nsHttpConnectionMgr::ProcessPendingQForEntry().
2. Moved “if (ent->mUrgentStartQ.Length() >= mMaxUrgentStartQ)” when transaction set to urgentStart.
3. int32_t caps = trans->Caps(); => uint43_t caps.
4. Found all mPendingQ and added corresponding code on mUrgentStartQ.
[netwerk/protocol/http/nsHttpConnectionMgr.h]
1. nit
[netwerk/test/unit/test_bug1312774.js]
1. modified test case do_check_eq(leastRequests, 1) to do_check_true(remainingRequests >= 1) and added comment

And I have a question on http2 test cases:
I figured out if the http2URL set to “http://localhost” that will make “preferred(in nsHttpConnectionMgr::GetSpdyPreferredEnt)” still is nullptr after call nsHttpConnectionMgr::LookupPreferredHash(aOriginalEntry).
But if http2URL set to other url like: foo.example.com, the mUsingSpdy will be false which makes nsHttpConnectionMgr triggers the http1 process.
And I tried to test test_http2.js, the testing result same to mine.
Do I have to set something (pref or environment variables)?

Thanks a lot!

Comment 12

[Amy Chung [:Amy]]

Attachment: bug-1312774-v4.patch

Hi Honza,
I updated a new patch and modified some code as below:
1. Added a nsParamName naming MAX_URGENT_START_Q on nsHttpConnectionMgr.h
2. Added network.max-urgentstart-queue on nsHttpHandler::PrefsChanged.
3. Included all modifications from bug-1312774-v2.patch.

Please give me your suggestions, thanks!

Comment 13

[Amy Chung [:Amy]]

Attachment: part2: test case

Hi Honza,
This patch is the test cases, and still existed the problem about http2 request can't process (same result when running test_http2.js).

Please help me and give me your suggestions.
Thanks!

Comment 14

[Honza Bambas (:mayhemer)]

Amy, I'll answer after the new year.  With the h2 question from comment 11 (which I'm not sure I fully understand) may help Patrick.

Comment 15

[Amy Chung [:Amy]]

Attachment: bug-1312774-v5.patch

I have finished the h2 test cases.
The modifications as below:
1. implement
   i.  The urgentStart transaction can create new connection when connection number >= 6. 
   ii.  Created a urgentStartQ.
   iii. Set a pref "network.max-urgentstart-queue" for the maximum of urgentStart queue (default set to 10, I want to ask a question, What is the  longest waitting time of the urgentStart transcation?)
   iv.  If urgentStart queue len >= network.max-urgentstart-queue
   v.   Doesn't let other transactions to create connection. 
   vi   Added priority when setting NS_HTTP_URGENT_START.
2. test cases
    i. h1 test cases
    ii.h2 test cases

Comment 16

[Amy Chung [:Amy]]

Attachment: bug-1312774-v5.patch

Comment 17

[Amy Chung [:Amy]]

Comment on attachment 8822619 [details] [diff] [review]
bug-1312774-v5.patch

Hi Honza,
The modifications of bug-1312774-v5.patch as below:
1. implement
   i.  The urgentStart transaction can create new connection when connection number >= 6. 
   ii.  Created a urgentStartQ.
   iii. Set a pref "network.max-urgentstart-queue" for the maximum of urgentStart queue (default set to 10, I want to ask a question, What is the  longest waitting time of the urgentStart transcation?)
   iv.  If urgentStart queue len >= network.max-urgentstart-queue
   v.   Doesn't let other transactions to create connection. 
   vi   Added priority when setting NS_HTTP_URGENT_START.
2. test cases
    i. h1 test cases
    ii.h2 test cases(all http2 transactions can create connection directly.)

Would you please review my patch? 
Thanks!

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Amy Chung [:Amy] from comment #11)
> And I have a question on http2 test cases:
> I figured out if the http2URL set to “http://localhost” that will make
> “preferred(in nsHttpConnectionMgr::GetSpdyPreferredEnt)” still is nullptr
> after call nsHttpConnectionMgr::LookupPreferredHash(aOriginalEntry).
> But if http2URL set to other url like: foo.example.com, the mUsingSpdy will
> be false which makes nsHttpConnectionMgr triggers the http1 process.
> And I tried to test test_http2.js, the testing result same to mine.
> Do I have to set something (pref or environment variables)?

Amy sorry for such a later answer here (I say this too often these days :))

This question is more for Patrick or Nick.  They know the http2 stuff better than me.  I really can't answer w/o studying the code.  Locally the test h2 fails for me (windows debug build) which might be because node.js is missing on my system.

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8822619 [details] [diff] [review]
bug-1312774-v5.patch

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

Overall looks good!  Thanks.  Now it's probably time to make the HTTP/1 (yes, *1*) test work (see the review notes, f- only for the bad test).  It will tell us if the code is correct or not the easiest way.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6012,5 @@
>              mCaps |= NS_HTTP_LOAD_UNBLOCKED;
>          }
> +        if (mClassOfService & nsIClassOfService::UrgentStart) {
> +            mCaps |= NS_HTTP_URGENT_START;
> +            SetPriority(nsISupportsPriority::PRIORITY_HIGHEST);

Why exactly are you setting the priority here?  urgent start already expresses a kind of (and only desired) priority here, so why moving the channel's prio as well?  maybe it's correct or points at something I missed.  anyway, it needs a "why" comment here or in bugzilla.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2061,5 @@
> +    if (ent->mUrgentStartQ.Length() > 0) {
> +        pendingQ = &(ent->mUrgentStartQ);
> +    } else if (ent->mPendingQ.Length() > 0) {
> +        pendingQ = &(ent->mPendingQ);
> +    } else {

When you process the whole urgent start queue (it's empty after the loop is done), and there is also something in the regular pending queue, you need to process the regular pending queue too (do the loop twice).

@@ +2296,2 @@
>          }
> +

nit: maybe assert here that the transaction is not contained in the opposite queue. (i.e. when NS_HTTP_URGENT_START is set in the trans' caps the trans is not present in mPendingQ, and vise versa).

@@ +2326,1 @@
>              if (transIndex >= 0) {

nit: I think you should first look the mPendingQ up.  we won't have many urgent start transactions.  also, why can't you use the caps flag to decide which queue to look at?

@@ +2423,5 @@
>      if (!ent) {
>          return;
>      }
>  
> +    for (int32_t i = ent->mUrgentStartQ.Length() - 1; i >= 0; --i) {

when you are here, please change this to (for both queues):

for (uint32_t i = ent->mUrgentStartQ.Length(); i > 0;) {
 --i;
 do stuff..
}

@@ +3447,5 @@
>  
>      // if this is still in the pending list, remove it and dispatch it
> +    //index = mEnt->mPendingQ.IndexOf(mTransaction);
> +    urgentStartIndex = mEnt->mUrgentStartQ.IndexOf(mTransaction);
> +    pendingIndex = mEnt->mPendingQ.IndexOf(mTransaction);

again, why not use the caps flag to decide on the queue to lookup?

::: netwerk/test/unit/test_bug1312774_http1.js
@@ +1,2 @@
> +"use strict";
> +

please add few lines describing steps the test performs, it makes it easier for anyone reading it or maintaining it to understand it more quickly.

@@ +50,5 @@
> +                 do_timeout(0, urgentStartHttpRequest);
> +               } else {
> +                 do_timeout(0, commonHttpRequest);
> +               }
> +             });

this time I applied the patch and ran this test looking at it more closely.

the test is actually wrong.  what you do is:

- open a channel
- let it finish
- open another one
- let it finish
- etc (4 times)

but what you have to do is:

- leave max-conn pref at its default (6)
- when a request arrives to the handler, process it asynchronously (as you do) but not finish() it yet
- put it to some queue (let's call it "test response queue" further)
and then the test itself:
- open, say, max-conn*2 channels, in parallel, where, say, the last 2 will be set UrgentStart
- wait until the test response queue has max-conn+2 elements
- then the test response queue should contain exactly max-conn normal requests and two urgent requests ; if not, something is wrong
- then close all channels and finish all async responses pending in the test response queue (just to make the httpd.js happy)

@@ +93,5 @@
> +    response.processAsync();
> +    response.setStatusLine(metadata.httpVersion, 200, "OK");
> +    response.setHeader("Cache-control", "no-cache");
> +    response.write("Hello world");
> +    response.finish();

as mentioned above, the response must be put to a queue (simply an array) and finish() on it must not be called before examination of the queue after all channels have been asyncOpen'ed.

Comment 20

[Amy Chung [:Amy]]

Attachment: Part1: implementation

Hi Honza,
I have modified the patch from your suggestions as below:
1. Finished to process mUrgentStartQ then prceossed the mPendingQ in ProcessSpdyPendingQ().
2. Set assert after calling "index = pendingQ->IndexOf(trans);".
3. Used caps to decide which queue have to remove trans on nsHttpConnectionMgr::OnMsgCancelTransaction() and nsHalfOpenSocket::OnOutputStreamReady().
4. Moved i into the for loop.

Please help me to review my patch.

But I haven't finished test cases yet, I arranged the test cases steps as below:
1. remove response.finish();
2. add a test queue and put response to the queue.
   var testResponseQueue = new Array();
   testResponseQueue.push(response);
3. create max-conn*2 channels, and set two channel to UrgentStart
4. create a function to process all responses which in queue.
   function processQueue() {
     let processResponse = queue.pop();
     processResponse.finish();
   }
5. set add_test(processQueue); in run_test();
Would you help me to confirm the steps of the test cases?

Thanks!

Comment 21

[Honza Bambas (:mayhemer)]

Amy, I don't see these changes in the latest patch (diffing [1] with [2]).  Have you submitted the most recent/correct patch?  Only major change I can see is split out of the test.

Then, are the patches from 2016-12-19 still valid?  If not, can you please obsolete them?

Thanks.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8827419&action=edit
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8822619&action=edit

Comment 22

[Amy Chung [:Amy]]

Attachment: Part1: implementation

Hi Honza,
Sorry for uploading a wrong file, and I have modified the patch from your suggestions as below:
1. Finished to process mUrgentStartQ then prceossed the mPendingQ in ProcessSpdyPendingQ().
2. Set assert after calling "index = pendingQ->IndexOf(trans);".
3. Used caps to decide which queue have to remove trans on nsHttpConnectionMgr::OnMsgCancelTransaction() and nsHalfOpenSocket::OnOutputStreamReady().
4. Moved i into the for loop.

Would you help me to review my patch?

Thanks!

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8830755 [details] [diff] [review]
Part1: implementation

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

Amy, sorry for such a late reply.  My backlog was pretty long.

f+ only one block that is probably not correct, but I think this is getting close to land.  Thanks!

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +641,1 @@
>          return preferred;

nit: when you are here, please surround the body with { }

@@ +884,5 @@
>  
>      nsHttpTransaction *trans;
>      nsresult rv;
>      bool dispatchedSuccessfully = false;
> +    nsTArray<RefPtr<nsHttpTransaction> > *pendingQ = nullptr;

no need for > >, >> is OK now.  seen on other places too.

@@ +1076,5 @@
> +        if (ent->mUrgentStartQ.Length() >= mMaxUrgentStartQ) {
> +            LOG((" urengt start Q len == max urgent start queue len "));
> +            return true;
> +         }
> +         return false;

please fix the indention.

@@ +1957,3 @@
>      if (rv == NS_ERROR_NOT_AVAILABLE) {
> +        if (caps & NS_HTTP_URGENT_START) {
> +            LOG(("  adding transaction to pending queue "

maybe:

"  adding transaction to the urgent-start pending queue "...?

@@ +2063,5 @@
> +        // Process mUrgentStartQ first.
> +        if (index == 0 && ent->mUrgentStartQ.Length() > 0)
> +            pendingQ = &(ent->mUrgentStartQ);
> +        else if (index > 0 && ent->mPendingQ.Length() > 0)
> +            pendingQ = &(ent->mPendingQ);

if only the mPendingQ has elements (most of the cases), then on the first loop (when index == 0) result of this code will be pendingQ = nullptr;  right?

This, if I'm not mistaken, should crash instantly when only ran regularly as a browser.

I'd rather see the inner loop (the |for (index = 0;...) {}| ) separated to a new (internal+private) method and call it twice, first for the urgent start q and second for the pending q.

@@ +2099,5 @@
> +
> +        // Put the leftovers back in the pending queue and get rid of the
> +        // transactions we dispatched
> +        leftovers.SwapElements(*pendingQ);
> +        leftovers.Clear();

when there are leftovers (dispatch failed or something) you will do the outer loop (while) again.  that is not correct.

Comment 24

[Amy Chung [:Amy]]

Attachment: implementation + test cases

Hi Honza,
I have modified your suggestions as below:
1. surrounded "return preferred;"  with { }.
2. modified > > to >>.
3. modified the indention on line 1076.
4. added a function "DispatchSpdyPendingQ".
5. called DispatchSpdyPendingQ(ent->mUrgentStartQ) first and then called DispatchSpdyPendingQ(ent->mPendingQ) on ProcessSpdyPendingQ.

And the attachment "test_result" is the testing output when I removed the "response.finish", you would see the TIMEOUT result and the blocked situation.
Do I use the wrong code flow on my test cases?

Thanks!

Comment 25

[Amy Chung [:Amy]]

Attachment: the testing output of test_bug1312774_http1.js

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 8835982 [details] [diff] [review]
implementation + test cases

Applying the patch fails:

applying bug-1312774-v8.patch
unable to find 'netwerk/test/unit/test_bug1312774_http1.js' for patching
2 out of 2 hunks FAILED -- saving rejects to file netwerk/test/unit/test_bug1312774_http1.js.rej
patching file netwerk/test/unit/xpcshell.ini
Hunk #1 FAILED at 285
1 out of 1 hunks FAILED -- saving rejects to file netwerk/test/unit/xpcshell.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory


You probably added the interdiff to the patch and not the whole patch for the tests.

Comment 27

[Amy Chung [:Amy]]

Attachment: implementation + test cases

Hi Honza,
Sorry for this mistake, and I uploaded new patch.
Would you help me to review my patch again?

Thanks!

Comment 28

[Amy Chung [:Amy]]

Attachment: implementation + test cases

Hi Honza,
I have finished to modify the test cases as below:
1. added id to confirm the http requests.
2. set cache-control to no-store on http request.
3. removed the response.finish().
4. pushed the response to queue.
5. when queue len = totalConnections, called processQueue().
6. pop the response and call finish().
7. confirmed the first processed request and second processed request are set to urgent start.

Would you help me to review my patch?
Thanks!

Comment 29

[Amy Chung [:Amy]]

Attachment: implementation + test cases

Sorry, I removed some extra spaces.

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 8836710 [details] [diff] [review]
implementation + test cases

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

This is closing to a final state.

The test still needs some work + few details in the code.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +890,5 @@
> +
> +    if (ent->mUrgentStartQ.Length() > 0) {
> +        pendingQ = &(ent->mUrgentStartQ);
> +    } else if (ent->mPendingQ.Length() > 0) {
> +        pendingQ = &(ent->mPendingQ);

shouldn't we process both queues?  same as in the http2 case?  sorry, if I previously misled you somehow at this place.

@@ +924,5 @@
>              else
>                  LOG(("  removing pending transaction based on "
>                       "TryDispatchTransaction returning hard error %x\n", rv));
>  
> +            int32_t caps = trans->Caps();

caps seems unused?

@@ +1078,5 @@
> +        if (ent->mUrgentStartQ.Length() >= mMaxUrgentStartQ) {
> +            LOG((" urengt start Q len == max urgent start queue len "));
> +            return true;
> +        } else {
> +            return false;

no |else| after return; please.

@@ +2049,5 @@
> +                                          nsConnectionEntry *ent)
> +{
> +    nsHttpConnection *conn = GetSpdyPreferredConn(ent);
> +    if (!conn || !conn->CanDirectlyActivate())
> +        return;

I think GetSpdyPreferredConn is a relatively expensive to call.  could we obtain and check the connection in ProcessSpdyPendingQ and then pass it to DispatchSpdyPendingQ as an argument?

@@ +2050,5 @@
> +{
> +    nsHttpConnection *conn = GetSpdyPreferredConn(ent);
> +    if (!conn || !conn->CanDirectlyActivate())
> +        return;
> +     if (pendingQ.Length() < 0) {

please check the indention

@@ +2061,5 @@
> +     // Dispatch all the transactions we can
> +     for (index = 0;
> +          index < pendingQ.Length() && conn->CanDirectlyActivate();
> +          ++index) {
> +          trans = pendingQ[index];

check indention

@@ +2084,5 @@
> +     // Slurp up the rest of the pending queue into our leftovers bucket (we
> +     // might have some left if conn->CanDirectlyActivate returned false)
> +     for (; index < pendingQ.Length(); ++index) {
> +          trans = pendingQ[index];
> +          leftovers.AppendElement(trans);

indention

@@ +2297,5 @@
> +        int32_t index = pendingQ->IndexOf(trans);
> +        MOZ_ASSERT(index >= 0);
> +        pendingQ->RemoveElementAt(index);
> +        InsertTransactionSorted(*pendingQ, trans);
> +

remove blank like

@@ +2326,5 @@
>              LookupConnectionEntry(trans->ConnectionInfo(), nullptr, trans);
>  
>          if (ent) {
> +            uint32_t caps = trans->Caps();
> +            int32_t transIndex = -1;

no need to init?

@@ +2437,5 @@
> +        LOG(("nsHttpConnectionMgr::OnMsgCancelTransactions %s %p %p\n",
> +             ci->HashKey().get(), ent, trans));
> +        trans->Close(reason);
> +        ent->mUrgentStartQ.RemoveElementAt(i);
> +        --i;

I prefer:

for (uint32_t i = array.Length(); i > 0;) {
  --i;
  T* t = array[i];
}

@@ +3453,5 @@
>      mHasConnected = true;
>  
>      // if this is still in the pending list, remove it and dispatch it
> +    uint32_t caps = mTransaction->Caps();
> +    int32_t index = -1;

no need to init

@@ +4014,5 @@
>              gHttpHandler->ConnMgr()->mNumHalfOpenConns--;
>          }
>      }
>  
> +    nsConnectionEntry *ent = gHttpHandler->ConnMgr()->mCT.Get(mConnInfo->HashKey());

what is this change?

::: netwerk/test/unit/test_bug1312774_http1.js
@@ +1,1 @@
> +Cu.import("resource://testing-common/httpd.js");

please add a comment to the top of the test what it is doing (steps) and how it tests the feature

@@ +24,5 @@
> +  var listner = new HttpResponseListener(id);
> +  chan.setRequestHeader("X-ID", id, false);
> +  chan.setRequestHeader("Cache-control", "no-store", false);
> +  chan.asyncOpen2(listner);
> +  dump("COMMON id=" + id + "\n");

I've added the dumps only to demonstrate its possible use.  but for production tests these dumps have to be removed or disabled.  you have two options:
1. remove all the dumps(..);
2. have |var debug = false; function log(message) { if (debug) dump(message); }| and replace all |dumps| with calls to |log| instead

@@ +44,5 @@
> +  for (var i = 0; i < maxConnections ; i++) {
> +    commonHttpRequest(i);
> +    do_test_pending();
> +  }
> +

remove this blank line

@@ +48,5 @@
> +
> +}
> +
> +function setup_urgentStartRequests() {
> +  for (var i = 0; i < 2; i++) {

please change the "2" to some global |const urgentRequsets = 2;| or something and then used it all around instead of "2" directly.

@@ +56,5 @@
> +}
> +
> +function HttpResponseListener(id)
> +{
> +  this.id = id

if you remove the dumps, remove the id as well.

@@ +74,5 @@
> +  onStopRequest: function (request, ctx, status) {
> +    testOrder++;
> +    switch (testOrder) {
> +      case 1:
> +        do_check_eq(this.id, 1000);

This is a bad check/test.  You cannot rely on order of responses coming back to you.  It's just a coincidence this works!  And to be honest, doesn't test much anything.  please remove this whole switch.

This whole test js file, when finally fixed, will indicate failure of the tested feature only by timeout.  Simply, if not all the expected requests (6 + 2) won't make it to the server, it will never finish the responses and will just hang.  That is alright, I don't see other way how to do this.

@@ +90,5 @@
> +function setup_http_server()
> +{
> +  var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
> +  maxConnections = prefs.getIntPref("network.http.max-persistent-connections-per-server");
> +  totalConnections = maxConnections + 2;

JavaScript Warning: "ReferenceError: assignment to undeclared variable totalConnections"

it's unused, please remove it.

@@ +98,5 @@
> +  {
> +    response.processAsync();
> +    responseQueue.push(response);
> +    // Wait all responses push to queue, then call finish() to
> +    // confirm the processing order.

Better might be:

// Wait for all expected requests to come but don't process then. Collect them in a queue for later processing.  We don't want to respond to the client until all the expected requests are made to the server.

@@ +99,5 @@
> +    response.processAsync();
> +    responseQueue.push(response);
> +    // Wait all responses push to queue, then call finish() to
> +    // confirm the processing order.
> +    if (respnseQueue.length == maxConnections +2) {

typo (respnseQueue) proving this test is not much of a test

+ 2 (space)

@@ +112,5 @@
> +
> +function processResponse() {
> +  var resposne = responseQueue.pop();
> +  if (response)
> +    resposne.finish();

another typo: resposne

please always do if () { } - brace the body.

anyway, this function is called just once, so it will finish only the first response that came to the server.

what you have to do here is:

  while (responseQueue.length) {
    var response = responseQueue.pop();
    response.finish();
  }

::: netwerk/test/unit/xpcshell.ini
@@ +290,5 @@
>  fail-if = os == "android"
>  # http2 unit tests require us to have node available to run the spdy and http2 server
>  [test_http2.js]
> +[test_bug1312774_http1.js]
> +[test_bug1312774_http2.js]

test_bug1312774_http2.js is missing in the patch.  I'd really like to check that test prior landing.

Comment 31

[Amy Chung [:Amy]]

Attachment: implementation + test cases

Hi Honza,
Thanks for your suggestions, I modified my patch as below:
[Implementation]
1. Added a DispatchPendingQ().
2. Called DispatchPendingQ(ent->mUrgentStartQ, ...) first, then called DispatchPendingQ(ent->mPendingQ) on ProcessPendingQForEntry().
3. Removed all init on int32_t index.
4. Moved "--i" to first line in the for loop.
5. Removed extra code and blank line.
6. Confirmed and fix the indention.
[http1 test case]
1. Added a comment for the testing steps.
2. Removed all dump and created log function.
3. Called log() on OnStopRequest().
4. Created urgentRequest for the for loop in setup_urgentStartRequests().
5. Removed the confirm code on OnStopRequest().
6. Removed totalConnections.
7. Modified the typo.
8. Removed extra blank line.
9. Confirmed and fixed the indention.

Would you help me to review my patch?
Thanks!

Comment 32

[Honza Bambas (:mayhemer)]

Comment on attachment 8837589 [details] [diff] [review]
implementation + test cases

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

This is almost r+!  There are few comments that I'd want you to fix and then just land the patch, if is was ready...

I tried to run a build with this patch applied (only this patch applied, on the same parent changeset it's based on) and browse a bit, got very soon an assertion failure here:

>	xul.dll!mozilla::net::nsHttpConnectionMgr::OnMsgReschedTransaction(32767, 0x0ba15c14) Line 2298	C++
 	xul.dll!RefPtr<mozilla::net::nsHttpConnectionMgr>::Proxy<void,int,mozilla::net::ARefBase *>::operator()<int &,RefPtr<mozilla::net::ARefBase> &>(32767, {...}) Line 334	C++
 	xul.dll!mozilla::net::ConnEvent::Run() Line 213	C++

exactly at:
        int32_t index = pendingQ->IndexOf(trans);
>       MOZ_ASSERT(index >= 0);
        pendingQ->RemoveElementAt(index);

the transaction is apparently not found in the queue (common queue, the urgency flag is not set on the transaction).  but according the original code, it seems like it doesn't have to be found (there originally was |if (index >= 0) ...|.  in comment 19, my suggestion was to check that when you find the transaction in its queue, it's not also contained in the other queue.  sorry if my english was too cryptic.

Please next time run a build with your patch before submitting for review and also please push to try.

also, the patch no longer applies to the latest mozilla-central sources (needs rebase).

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -868,2 @@
>  {
> -    MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);

I'd rather leave the assertion also inside nsHttpConnectionMgr::DispatchPendingQ, so that when somebody adds a call to this method is not obligated to do the threading check.

@@ +2055,5 @@
> +    }
> +
> +    nsTArray<RefPtr<nsHttpTransaction>> leftovers;
> +    uint32_t index = 0;
> +    nsHttpTransaction *trans = nullptr;

very small nit: it was OK to have two declarations, each for one block (would keep the patch simpler).  up to you to revert or leave as you have it now.

@@ +2101,5 @@
> +nsHttpConnectionMgr::ProcessSpdyPendingQ(nsConnectionEntry *ent)
> +{
> +    nsHttpConnection *conn = GetSpdyPreferredConn(ent);
> +    if (!conn || !conn->CanDirectlyActivate())
> +        return;

if (..) {
  return;
}

@@ +2431,5 @@
>          return;
>      }
>  
> +    for (int32_t i = ent->mUrgentStartQ.Length() - 1; i >= 0;) {
> +        --i;

this is really incorrect, the correct form is:

for (int32_t i = ent->mUrgentStartQ.Length(); i > 0;) {
  --i;
  access the array here
}

@@ +2440,5 @@
> +        ent->mUrgentStartQ.RemoveElementAt(i);
> +    }
> +
> +    for (int32_t i = ent->mPendingQ.Length() - 1; i >= 0;) {
> +        --i;

here as well

::: netwerk/test/unit/test_bug1312774_http1.js
@@ +1,2 @@
> +// test bug 1312774.
> +// 1. Create six(set from network.http.max-persistent-connections-per-server)

s/set/read/ ?

@@ +3,5 @@
> +//    common Http requests and two urgent-start Http requests.
> +// 2. Set cache-control to no-store on every http requests.
> +// 3. Push every Http responses to queue.
> +// 4. Let every Http response calling finish() when the length of queue equals to 8.
> +// 5. Wait all Http Requests finish and will make this test doens't time out.

ok, but this should more both describe the steps and also explain what you are testing and how.  so, I'd suggest something like:

- create 6 (=network.http.max-persistent-connections-per-server) common + 2 urgent requests to a single host and path, in parallel
- let all the requests unanswered by the server handler (process them async and don't finish)
- the first 6 pending common requests will fill the limit for per-server parallelism
- but the two urgent requests must reach the server despite those 6 common pending requests
- the server handler doesn't let the test finish until all 8 expected requests arrive
- note: if the urgent request handling is broken (the urgent-marked requests get blocked by queuing) this test will time out

@@ +14,5 @@
> +server.start(-1);
> +var baseURL = "http://localhost:" + server.identity.primaryPort + "/";
> +var maxConnections = 0;
> +var urgentRequests = 0;
> +var debug = true;

switch to false before landing

@@ +18,5 @@
> +var debug = true;
> +
> +function log(msg) {
> +  if (!debug)
> +    return;

if () {
}

@@ +83,5 @@
> +  onDataAvailable: function (request, ctx, stream, off, cnt) {
> +  },
> +
> +  onStopRequest: function (request, ctx, status) {
> +    log("STOP id=" + this.id);

I think this log is the least interesting.  more interesting is to log you have asyncopen'ed a channel (+its id) and then that you received it on the server (again +its id).

::: netwerk/test/unit/xpcshell.ini
@@ +289,5 @@
>  # Bug 675039: test fails consistently on Android
>  fail-if = os == "android"
>  # http2 unit tests require us to have node available to run the spdy and http2 server
>  [test_http2.js]
> +[test_bug1312774_http1.js]

yep, let's have h2 testing in a different bug.  don't get blocked here.

Comment 33

[Amy Chung [:Amy]]

Attachment: implementation + test cases

Hi Honza,
I have modified the patch from your suggestions as below:
[implementation]
1. Removed MOZ_ASSERT(index >= 0); and reverted to original code.
2. Added "MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);" to nsHttpConnectionMgr::DispatchPendingQ.
3. Moved nsHttpTransaction *trans into for loop.
4. Modified the condition of for loop to "for (int32_t i = ent->mUrgentStartQ.Length(); i > 0;)".

[test case]
1. Modified the description of test. 
2. Set debug to false.
3. Added log when asyncopen'ed a channel and received it on the server.

And I have push to try server but only test the xpcshell:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a4a5bad73fc0d98585351cb0fc504b0f990baa5

Would you help me to review my patch?
Thanks!

Comment 34

[Honza Bambas (:mayhemer)]

(In reply to Amy Chung [:Amy] from comment #33)
> And I have push to try server but only test the xpcshell:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0a4a5bad73fc0d98585351cb0fc504b0f990baa5

Please push with try: -b do -p all -u all -t none to make sure we don't break anything.  This patch needs a good testing, it's not that simple as it seems.

Thanks!

Comment 35

[Amy Chung [:Amy]]

Hi Honza,
Sorry for didn't do the full test, I have pushed my patch to try server and the testing result as below:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4c178691bea00c486944856e836e6dd8ea71b10
Would you help me to review my patch?

Thanks!

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to Amy Chung [:Amy] from comment #35)
> Hi Honza,
> Sorry for didn't do the full test, I have pushed my patch to try server and
> the testing result as below:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d4c178691bea00c486944856e836e6dd8ea71b10
> Would you help me to review my patch?
> 
> Thanks!

So, we have one failing test:
Android (all) tc-X(4): netwerk/test/unit/test_http2.js 

That definitely needs to be taken care of.

Comment 37

[Amy Chung [:Amy]]

Attachment: implementation + test cases

Hi Honza,
Beside the modifications are same as https://bugzilla.mozilla.org/show_bug.cgi?id=1312774#c33.
I found the http1 test blocked on Android platform, I think maybe the Cache-control set to no-store doesn't work on Android.
Because I tried to remove the code about queuing http responses, the test can pass on Android.
And the error log is same as the situation that occurred when I didn't set Cache-control to no-store.
For these reason, I skipped the http1 test on Android.

try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b584fa05ee2b7244daa4391ada23516df186c34

Would you help me to review my patch?

Comment 38

[Honza Bambas (:mayhemer)]

(In reply to Amy Chung [:Amy] from comment #37)
> Created attachment 8840804 [details] [diff] [review]
> implementation + test cases
> 
> Hi Honza,
> Beside the modifications are same as
> https://bugzilla.mozilla.org/show_bug.cgi?id=1312774#c33.
> I found the http1 test blocked on Android platform, I think maybe the
> Cache-control set to no-store doesn't work on Android.

I would not bet on it.  Did you try pushing the test with the logs/dumps in the test on ? (debug = true)  You should be able to find it in the try run log then.  I checked your try pushes and I don't see your test logging enabled.  The reason can be anything.

> Because I tried to remove the code about queuing http responses, the test
> can pass on Android.

Sorry, I don't understand this sentence.  But if you make the test pass by modification of the code, the test might catch a mistake in it.

> And the error log is same as the situation that occurred when I didn't set
> Cache-control to no-store.
> For these reason, I skipped the http1 test on Android.

The log seems to be the same, but please first push with debug = true and check what that tells you.

> 
> try server result:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0b584fa05ee2b7244daa4391ada23516df186c34
> 
> Would you help me to review my patch?

Comment 39

[Honza Bambas (:mayhemer)]

Comment on attachment 8840804 [details] [diff] [review]
implementation + test cases

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

only f+ because of the android test failure.  if the cause is explained and is caused by the test infra specific to android, I'll change this to r+ and we can land.

there are few small nits to fix anyway.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +921,5 @@
> +         if (dispatchedSuccessfully && !considerAll) {
> +             break;
> +         }
> +
> +         ++i;

I think the indention here was correct in the previous patch version.

::: netwerk/test/unit/test_bug1312774_http1.js
@@ +112,5 @@
> +  {
> +    var id = metadata.getHeader("X-ID");
> +    response.processAsync();
> +    responseQueue.push(response);
> +    log("Server recived the response id=" + id);

nit: move the |var id = ..| line close to this log.

::: netwerk/test/unit/xpcshell.ini
@@ -289,5 @@
>  [test_safeoutputstream.js]
>  [test_simple.js]
>  [test_sockettransportsvc_available.js]
>  [test_socks.js]
> -# Bug 675039: test fails consistently on Android

why are you removing this line?

@@ +382,5 @@
>  [test_trackingProtection_annotateChannels.js]
>  [test_race_cache_network.js]
> +[test_bug1312774_http1.js]
> +# The test will block when the http server waits for the response queue is
> +# full then do response.finish() on Android.

please check the grammar of this comment.  Also, I'm not sure I understand the reason to disable the test just by reading this comment.  

as said in comment 38, I'd rather find the cause why the test doesn't run on Android first.

Comment 40

[Honza Bambas (:mayhemer)]

Amy, I will also ask you to rebase this patch on top of patch from bug 1340655.  That patch is way larger and also more important to land ASAP (it's between else a crash fix).  Sorry for this inconvenience, but merging is unfortunately part of this work...  You may try using hg rebase, see [1] for more info.

ni? to make sure you don't miss this.


[1] https://www.mercurial-scm.org/wiki/RebaseExtension

Comment 41

[Amy Chung [:Amy]]

I understood, I will rebase the patch of bug 1340655 after bug 1340655 landing.
Thanks!

Comment 42

[Amy Chung [:Amy]]

Attachment: implementation + test cases

Hi Honza,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=153e95dcfed1114967729e8d32812e7f96fd4d84&selectedJob=81496483
I have rebase the patch of bug 1340655 and the test_bug1312774_http1.js passed on android.
Would you help me to review my patch?

Thanks!

Comment 43

[Amy Chung [:Amy]]

Comment on attachment 8843844 [details] [diff] [review]
implementation + test cases

Hi Honza,
I forgot to add the length of UrgentStartQ to log on nsHttpConnectionMgr::ProcessPendingQForEntry().
I will add the length of UrgentStartQ before this patch be landed.

Thanks!

Comment 44

[Honza Bambas (:mayhemer)]

Comment on attachment 8843844 [details] [diff] [review]
implementation + test cases

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

r=honzab with one last comment addressed.  btw, needs rebase again, this grows old quickly :(

::: modules/libpref/init/all.js
@@ +1472,5 @@
>  // connections to the server is less then max-persistent-connections-per-server.
>  pref("network.http.max-persistent-connections-per-server", 6);
>  
> +// If the length of mUrgentStartQ >= network.max-urgentstart-queue,
> +// the nsHttpConnectionMgr can't create connection to common transaction.

this comment is pretty much insufficient, but see below to what it led me.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +965,5 @@
>      LOG(("nsHttpConnectionMgr::AtActiveConnectionLimit [ci=%s caps=%x]\n",
>          ci->HashKey().get(), caps));
>  
> +    if (caps & NS_HTTP_URGENT_START) {
> +        if (ent->mUrgentStartQ.Length() >= mMaxUrgentStartQ) {

I actually found this to be an inaccurate condition when I wanted to suggest a better comment for the preference (proves importance of good comments ;)).  Interesting I haven't caught this earlier... 

I think what we need here is to limit the number of connections we create beyond the limit of 6 to send urgent-start requests on.  The urgent request queue length doesn't exactly reflect that.  I think that we should rather check against the number of open + half-open connections for the host, which is the number now calculated at https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/netwerk/protocol/http/nsHttpConnectionMgr.cpp#980 as totalCount var.

Just move it here so that we can use it in this condition and replace |ent->mUrgentStartQ.Length()| with |totalCount|.  


(I'll send a patch roughly showing the idea)

Your new preference max-urgentstart-queue will need an update too.  The number in the preference should be the number of connections per host we allow to go beyond the max-persistent limit (6).  I'd start with 4, which will give an overall limit of 10 connections per host when there is a large number of urgent start requests.

Name of the preference should be something like "max-urgent-start-excessive-connections-per-host".

And the comment for the preference should be:

// Number of connections that we can open beyond the standard parallelism limit defined by max-persistent-connections-per-server/-proxy to handle urgent-start marked requests

::: netwerk/test/unit/test_bug1312774_http1.js
@@ +138,5 @@
> +}
> +
> +function run_test() {
> +  setup_http_server();
> +  //Run http1 test cases

space after //

and I'm not sure we need this comment at all when the test is split for h1 and h2 separately.

Comment 45

[Amy Chung [:Amy]]

Attachment: implementation + test cases

[Modification]
I have modified as below:
1. Rebase patch of Bug 1326339.
2. Modified the pref id "network.http.max-urgentstart-queue" to "network.http.max-urgent-start-excessive-connections-per-host".
3. Modified the condition of limiting for dispatching urgent-start transaction to connection on AtActiveConnectionLimit().
4. Removed comment on test_bug1312774_http1.js.

[Try server]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74ccb087b5e29e2cef6354fa798b3164bd18c974

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efff49456095
Allow channels marked as urgentStart to bypass connection limits. r=mayhemer

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/efff49456095