603512 - Adjust Connection Pipeline Status for Unexpected Large Transaction Content-Length

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

If a connection receives a header with a large content-length mark the connection non-suitable for additional pipelined items until it completes. If there are already idempotent requests that have been sent down the connection behind this large response, consider repeating them on a different connection or moving them to non pipelined queues. These probably require 2 different thresholds.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: reschedule-blocked-pipeline-xacts v1

the type and state patch tries hard not to form pipelines behind resources that could become head of line blockers. But of course that requires the ability to predict the future, and won't be perfect. 

This patch reacts to a transaction that has a large response body (defined by either a large content-length header or actually reading a large number of chunked bytes) by cancelling any transactions that have been pipelined down the same connection and rescheduling them elsewhere. It also changes the type of the connection to "solo", which prevents new transactions from being pipelined onto this one and provides class-specific negative feedback to the pipeline manager so that near-future requests to the same host of the same type (e.g. general) will not be pipelined but other types (e.g. img or js/css) can still do that.

Content-Length is ideal, because it allows us to identify the problem so early. But even actually reading the document for a fairly long time gives it a fairly high probability of not ending soon. (i.e. long document sizes are spread over a larger range than small ones. duh.)

The pref network.http.pipelining.maxsize controls the threshold. I set the default at 150KB, which is roughly the bandwidth delay product of a 1mbit 100ms rtt connection and 1 rtt is mostly what you are giving up by canceling it on one connection and sending it on another. (modulo maybe needing a handshake).

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: reschedule blocked pipeline xacts v2

update due to 606719 induced bitrot

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: reschedule blocked pipeline xacts v3

update bitrot, confrom better to style guide, updates based on experience (i.e. bugs and tweaks), etc..

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: reschedule blocked pipeline xacts v4

update patch to fix a bug involving pipeline cancellation data structures

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: reschedule blocked pipeline xacts v5

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: reschedule blocked pipeline xacts v6

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: reschedule blocked by large 7

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: patch taken from larch (by Patrick McManus)

Created this attachment just to have the nice and actual spliter UI.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 547445 [details] [diff] [review]
patch taken from larch (by Patrick McManus)

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

This looks straightforward and simple.  I am giving r- for just two major details:

- PipelineData() and the new mPipelineData member:
Please don't do relations like these where you really don't have to.  This may create a cycle reference and a leak and is absolutely unnecessary, even there would be a need to reach the pipeline object, it can be done better.  This concrete code can be well build w/o this extra reference:

Only caller of the abstract nsAHttpConnection::CancelPipeline() method /that may be using the PipelineData() getter/ is private method nsHttpTransaction::CancelPipeline.  nsHttpTransaction::mConnection can only be one of nsConnectionHandler->nsHttpConnection chain or nsHttpPipeline directly.  When there is a pipeline object directly, we reach the correct code with mConnection->CancelPipeline() and get what we expect (cancel the rest of scheduled transactions).  But when there would be nsConnectionHandler class, the method should have do nothing, i.e. should have just been unimplemented.  I don't see a realistic situation when a transaction trying to cancel its potential pipeline would be directly bound to nsHttpConnection object (through nsConnectionHandle) and that connection would have nsHttpPipeline as a transaction over it.

Also I don't see you would ever drop this new reference from nsHttpConnection early enough, e.g. when the transaction on a connection (here nsHttpPipeline) gets canceled, or so.


- You probably broke pipeline kill code in nsHttpPipeline::CloseTransaction(), if not please provide a detailed explanation for the change.

::: modules/libpref/src/init/all.js
@@ +765,5 @@
>  // connection instead
>  pref("network.http.pipelining.max-optimistic-requests" , 4);
>  
>  pref("network.http.pipelining.aggressive", false);
> +pref("network.http.pipelining.maxsize" , 200000);

This doesn't seem to conform the bug comment, but you know better what the number should be.

Needs to be added to the mobile prefs as well.

BTW: could we somehow optimize maxsize by measured RTT of the connection?

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +38,5 @@
>  #ifndef nsAHttpConnection_h__
>  #define nsAHttpConnection_h__
>  
>  #include "nsISupports.h"
> +#include "nsAHttpTransaction.h"

Wouldn't be better to rather move nsAHttpTransaction::Classifier to nsAHttpConnection?  It is not used by nsAHttpTransaction abstract class anyway and just cross link headers because of it doesn't seems to be a good idea.

But in bug 672958 we will move this to an interface, anyway.

@@ +131,5 @@
>      // reference to it to the caller.
>      virtual nsHttpConnection *TakeHttpConnection() = 0;
> +
> +    // Cancel and reschedule transactions deeper than the current response
> +    virtual PRUint32 CancelPipeline() = 0;

Comment also that the result is the number of canceled transactions.

::: netwerk/protocol/http/nsHttpConnection.h
@@ +226,5 @@
>      PRPackedBool                    mLastTransactionExpectedNoContent;
>      PRPackedBool                    mIdleMonitoring;
>  
>      nsAHttpTransaction::Classifier  mClassification;
> +    nsRefPtr<nsHttpPipeline>        mPipelineData;

As commented about, please get rid of this new referring member.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2001,5 @@
> +nsHttpConnectionMgr::nsConnectionHandle::CancelPipeline()
> +{
> +    if (mConn && mConn->PipelineData())
> +        return mConn->PipelineData()->CancelPipeline();
> +    return 0;

This method shell be unimplemented (always just return 0)

@@ +2009,5 @@
> +nsHttpConnectionMgr::nsConnectionHandle::Classification()
> +{
> +    if (mConn)
> +        return mConn->Classification();
> +    return nsAHttpTransaction::CLASS_SOLO;

Could it happen we wrongly base a code path on the default result (SOLO) when there is no connection in the handle?  You may want to add NS_WARNING or LOG that we are querying a class on a handle w/o a connection.

@@ +2143,5 @@
>          case BadInsufficientFraming:
>              mPipeliningClassPenalty[classification] += 7000;
>              break;
> +        case BadUnexpectedLarge:
> +            mPipeliningClassPenalty[classification] += 250;

So, one large image will disable pipelining of images for more then 4 minutes?

I'm thinking of having the penalty much smaller for this case or even none.  Also, shouldn't size of the limit be different for different classes?  Images might be larger and we still might want to pipeline them.  But you probably know more then me to decide on this.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +180,5 @@
>          BadInsufficientFraming = kPipelineInfoTypeBad | 0x0008,
> +        
> +        // Used when a very large response is recevied in a potential pipelining
> +        // context. Large responses cause head of line blocking.
> +        BadUnexpectedLarge = kPipelineInfoTypeBad | 0x000B,

Just a suggestion, not a review request: you may want to renumber the events to keep good track of what the max is here or (I wanted to suggest it in the T+S patch already, but didn't) there is no need to have an increasing overall number, have it just for each group (as I actually originally suggested).  That would make adding new events simpler (by not mistaking the largest number), and also avoid conflicts when we might want to persist the event numbers in the future - we would always be backwards compatible.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +997,5 @@
>  
> +    if (PREF_CHANGED(HTTP_PREF("pipelining.maxsize"))) {
> +        rv = prefs->GetIntPref(HTTP_PREF("pipelining.maxsize"), &val);
> +        if (NS_SUCCEEDED(rv)) {
> +            mMaxPipelineObjectSize = PRInt64 (NS_CLAMP(val, 1000, 100000000));

No space between PRInt64 and (

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +277,5 @@
>          // the others will be shortly as well.
>          killPipeline = PR_TRUE;
>      }
>  
> +    DontReuse();

Add a comment why not to reuse this connection after a transaction has been canceled on it.

Also remember that CloseTransaction gets called when user presses the stop button, I believe.  Not sure we want to throw the connection away in that case, but maybe we do.

@@ +282,5 @@
>      trans->Close(reason);
>      NS_RELEASE(trans);
>  
> +    if (killPipeline)
> +        CancelPipeline();

I don't understand this change.  Didn't you want to leave this unchanged?  The pipeline is actually not closed like this, at least mStatus and mClosed members are not properly set and (probably?) Response(0) is left in the queue.

Otherwise this needs a very good explanation.

@@ +421,5 @@
> +nsHttpPipeline::Classification()
> +{
> +    if (mConnection)
> +        return mConnection->Classification();
> +    return nsAHttpTransaction::CLASS_SOLO;

Here as well as with the handler.  Just an idea to check on something like that.

@@ +706,5 @@
> +    reqLen = mRequestQ.Length();
> +    respLen = mResponseQ.Length();
> +    total = reqLen + respLen;
> +    if (respLen)
> +        total--;                                  /* dont count ourselves */

Rather: /* Don't count the first response, if present. */ ?

@@ +708,5 @@
> +    total = reqLen + respLen;
> +    if (respLen)
> +        total--;                                  /* dont count ourselves */
> +
> +    if (total > 0) {

Rather:

if (!total) 
    return 0;

Generally early returns are preferred, if not in price of readability.

@@ +710,5 @@
> +        total--;                                  /* dont count ourselves */
> +
> +    if (total > 0) {
> +        // any pending requests can ignore this error and be restarted
> +        for (i=0; i < reqLen; i++) {

Even you just copy the old code, please add spaces around |=|. ++i.

@@ +717,5 @@
> +            NS_RELEASE(trans);
> +        }
> +        mRequestQ.Clear();
> +        
> +        // any remaining pending responses can be restarted

// except the first one, that we might either want to finish on this pipeline or cancel individually later.

..or something like that..

@@ +751,4 @@
>      nsRefPtr<nsHttpConnectionInfo> ci;
>      GetConnectionInfo(getter_AddRefs(ci));
> +    PRUint32 numRescheduled = CancelPipeline();
> +    if (ci && numRescheduled)

Hmm.. what if there was just a single Response before call to CancelPipeline?  CancelPipeline will then return 0, right?  It is then different from the previous condition, but this change might be correct.  Add a comment please, if so.

@@ +758,2 @@
>  
>      if (trans) {

I'm just curious, if canceling the Response(0) after all other responses might influence the transaction priority ordering.  But it is just a detail we may, if necessary, deal later with.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1224,5 @@
>          this, count, *contentRead, mContentRead, mContentLength));
>  
> +    if ((mClassification != CLASS_SOLO) &&
> +        mChunkedDecoder &&
> +        (mContentLength > mMaxPipelineObjectSize))

Shouldn't you check mContentRead and not mContentLength here?

@@ +1326,5 @@
>  
> +void
> +nsHttpTransaction::CancelPipeline(PRUint32 reason)
> +{
> +    // reason is casted through a uint to avoid compiler header deps

Could predeclaration of |enum nsHttpConnectionMgr::PipelineFeedbackInfo| in nsHttpTransaction.h help?  (I never tried my self).

@@ +1332,5 @@
> +        mConnInfo, (enum nsHttpConnectionMgr::PipelineFeedbackInfo) reason,
> +        nsnull, mClassification);
> +
> +    mConnection->CancelPipeline();
> +    mClassification = CLASS_SOLO;

This is here just because we want to prevent this transaction get into a pipeline again during restart or so, right?  If confirmed, please add a comment why we change the class.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 547445 [details] [diff] [review]
patch taken from larch (by Patrick McManus)

By the way: we may want to try to find a little bit better name for nsAHttpConnection::CancelPipeline method.  It invokes feeling we cancel all requests and do not reschedule.  Maybe RestartPipeline, TearOffPipeline or ReschedulePipeline?  (I would prefer the last one).

Comment 11

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #9)

> Needs to be added to the mobile prefs as well.

as I understand it, mobile will inherit the default prefs so there is no reason to add to mobile prefs unless we want a different value there.

> BTW: could we somehow optimize maxsize by measured RTT of the connection?

unfortunately maxsize is really a property of bandwidth and not rtt .. but even still linking it to rtt probably better than a single constant. Probably worth refining at later date.

> 
> @@ +2143,5 @@
> >          case BadInsufficientFraming:
> >              mPipeliningClassPenalty[classification] += 7000;
> >              break;
> > +        case BadUnexpectedLarge:
> > +            mPipeliningClassPenalty[classification] += 250;
> 
> So, one large image will disable pipelining of images for more then 4
> minutes?
> 
> I'm thinking of having the penalty much smaller for this case or even none. 
> Also, shouldn't size of the limit be different for different classes? 
> Images might be larger and we still might want to pipeline them.  But you
> probably know more then me to decide on this.
> 

we do want to stop pipelining in the face of large objects for two reasons:

1] the reward is proportionally less because transfer time is made up of latency plus transfer time, right? As transfer time grows (due to byte count) the latency becomes less important.
2] head of line blocking is the number one problem with pipelines as far as I am concerned and large objects are what cause that. So I really like that if we see large images we stop pipelining them but continue to pipeline other kinds of objects (js, html, etc..)

But based on combing through my logs I do want to tune this to be a little less conservative. I have raised the max object size and cut in half the penalty associated with this, but the basic algorithm is the same.


> ::: netwerk/protocol/http/nsHttpConnectionMgr.h
> @@ +180,5 @@

> Just a suggestion, not a review request: you may want to renumber the events
> to keep good track of what the max is here or (I wanted to suggest it in the

given that it can have a mask comprised of multiple types, I like that the id field is 'globally' unique.

> 

> I don't understand this change.  Didn't you want to leave this unchanged? 
> The pipeline is actually not closed like this, at least mStatus and mClosed
> members are not properly set and (probably?) Response(0) is left in the
> queue.
> 
> Otherwise this needs a very good explanation.

the way the code was written before a killPipeline==true meant that the connection was torn down right away. That meant even if the CloseTransaction() did not refer to Response(0) we could not finish reading Response(0) - and a partial response(0) cannot be easily restarted. So the connection->Close() was removed and instead the connection is marked DontReuse() and all the pipelined transactions are immediately rescheduled elsewhere - and then we allow the connection manager to naturally call close() on it when the connection is reclaimed.

The issues with mStatus, mClosed, and any lingering Response(0) are cleaned up when that close happens.

> @@ +758,2 @@
> >  
> >      if (trans) {
> 
> I'm just curious, if canceling the Response(0) after all other responses
> might influence the transaction priority ordering.  But it is just a detail
> we may, if necessary, deal later with.

its an interesting thought.

> 
> Shouldn't you check mContentRead and not mContentLength here?
> 

yes! thank you.

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: v8

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 585450 [details] [diff] [review]
v8

I mistakenly left an assert in connectionhandle::cancelpipeline() where it should just be a nop (there is no pipeline, but its ok to try and cancel it).

will fix it up in the morning.

Comment 14

[Patrick McManus [:mcmanus]]

Attachment: v9

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 585739 [details] [diff] [review]
v9

try server says v9 has a leak.

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 585739 [details] [diff] [review]
v9

last comment was incorrect. Try server says 603514 (1 digit off!) has a leak. sorry for the churn.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 585739 [details] [diff] [review]
v9

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

Looks good, except usage of NS_ERROR_CORRUPTED_CONTENT, it needs some explanation.

r=honzab with that.

::: netwerk/protocol/http/SpdySession.cpp
@@ +1766,5 @@
> +nsAHttpTransaction::Classifier
> +SpdySession::Classification()
> +{
> +  if (!mConnection)
> +    return nsAHttpTransaction::CLASS_GENERAL;

Blank line after this line please.

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +113,2 @@
>      virtual bool IsReused() = 0;
> +    virtual void   DontReuse() = 0;

One space only.

::: netwerk/protocol/http/nsHttpConnection.h
@@ +41,5 @@
>  
>  #include "nsHttp.h"
>  #include "nsHttpConnectionInfo.h"
>  #include "nsAHttpTransaction.h"
> +#include "nsHttpPipeline.h"

Why?

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +55,5 @@
>  #include "nsIObserver.h"
>  #include "nsITimer.h"
>  #include "nsIX509Cert3.h"
>  
> +#include "nsHttpPipeline.h"

Why?

::: netwerk/protocol/http/nsHttpHandler.h
@@ +227,5 @@
>      bool GetPipelineAggressive()     { return mPipelineAggressive; }
> +    void GetMaxPipelineObjectSize(PRInt64 &outVal)
> +    {
> +        outVal = mMaxPipelineObjectSize;
> +    }

I'd rather see this as * and not &.

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +281,5 @@
>      NS_RELEASE(trans);
>  
> +    if (killPipeline)
> +        // reschedule anything from this pipeline onto a different connection
> +        CancelPipeline(reason);

Put braces around.  Two lines command demands it.

@@ +828,5 @@
> +    }
> +    mRequestQ.Clear();
> +
> +    // any pending responses can be restarted except for the first one,
> +    // that we might want to finish on this pipeline or cancel individually

Maybe add a note that we don't pipeline POSTs (or better, any non-idempotent methods).  Or even better, add a note this is dependent on fact it is ensured by higher levels we don't do that.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1140,5 @@
>              mContentLength = mResponseHead->ContentLength();
>  
> +            if ((mClassification != CLASS_SOLO) &&
> +                (mContentLength > mMaxPipelineObjectSize))
> +                CancelPipeline(nsHttpConnectionMgr::BadUnexpectedLarge);

Braces around the command please.

@@ +1249,5 @@
> +    // for this response reschedule the pipeline
> +    if ((mClassification != CLASS_SOLO) &&
> +        mChunkedDecoder &&
> +        (mContentRead > mMaxPipelineObjectSize))
> +        CancelPipeline(nsHttpConnectionMgr::BadUnexpectedLarge);

Braces here as well.

I'm curious if this doesn't actually gets better handled by the stall read in pending pipelined transactions.  It could happen the content is just about to be read completely (be just a bit above the threshold).

Also maybe more effective is to use directly the value of the chunk size we know very early.  If a chunk is large, you have to wait for the whole chunk, that may be split among many frames, a long time while you already know the size of it and can decide.

@@ +1359,5 @@
> +        mConnInfo,
> +        static_cast<nsHttpConnectionMgr::PipelineFeedbackInfoType>(reason),
> +        nsnull, mClassification);
> +
> +    mConnection->CancelPipeline(NS_ERROR_CORRUPTED_CONTENT);

Really NS_ERROR_CORRUPTED_CONTENT ?


Please give an explanation why using this error.  I think NS_ERROR_ABORT is the right here.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +148,5 @@
>      nsresult ProcessData(char *, PRUint32, PRUint32 *);
>      void     DeleteSelfOnConsumerThread();
>  
>      Classifier Classify();
> +    void       CancelPipeline(PRUint32 reason);

The name here is confusing with nsAHttpConnection::CancelPipeline

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 585739 [details] [diff] [review]
v9

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

r=honzab with few nits.

::: netwerk/protocol/http/SpdySession.cpp
@@ +1766,5 @@
> +nsAHttpTransaction::Classifier
> +SpdySession::Classification()
> +{
> +  if (!mConnection)
> +    return nsAHttpTransaction::CLASS_GENERAL;

Blank line after this line please.

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +113,2 @@
>      virtual bool IsReused() = 0;
> +    virtual void   DontReuse() = 0;

One space only.

::: netwerk/protocol/http/nsHttpConnection.h
@@ +41,5 @@
>  
>  #include "nsHttp.h"
>  #include "nsHttpConnectionInfo.h"
>  #include "nsAHttpTransaction.h"
> +#include "nsHttpPipeline.h"

Why?

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +55,5 @@
>  #include "nsIObserver.h"
>  #include "nsITimer.h"
>  #include "nsIX509Cert3.h"
>  
> +#include "nsHttpPipeline.h"

Why?

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +828,5 @@
> +    }
> +    mRequestQ.Clear();
> +
> +    // any pending responses can be restarted except for the first one,
> +    // that we might want to finish on this pipeline or cancel individually

Maybe add a note that we don't pipeline POSTs (or better, any non-idempotent methods).  Or even better, add a note this is dependent on fact it is ensured by higher levels we don't do that.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1249,5 @@
> +    // for this response reschedule the pipeline
> +    if ((mClassification != CLASS_SOLO) &&
> +        mChunkedDecoder &&
> +        (mContentRead > mMaxPipelineObjectSize))
> +        CancelPipeline(nsHttpConnectionMgr::BadUnexpectedLarge);

I'm curious if this doesn't actually gets better handled by the stall read in pending pipelined transactions.  It could happen the content is just about to be read completely (be just a bit above the threshold).

Also maybe more effective to use directly the value of the chunk size we know very early.  If a chunk is large, you have to wait for the whole chunk, that may be split among many frames, a long time while you already know the size of the whole chunk.

@@ +1359,5 @@
> +        mConnInfo,
> +        static_cast<nsHttpConnectionMgr::PipelineFeedbackInfoType>(reason),
> +        nsnull, mClassification);
> +
> +    mConnection->CancelPipeline(NS_ERROR_CORRUPTED_CONTENT);

Really NS_ERROR_CORRUPTED_CONTENT ?

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +148,5 @@
>      nsresult ProcessData(char *, PRUint32, PRUint32 *);
>      void     DeleteSelfOnConsumerThread();
>  
>      Classifier Classify();
> +    void       CancelPipeline(PRUint32 reason);

The name here is confusing with nsAHttpConnection::CancelPipeline

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #18)
> I'm curious if this doesn't actually gets better handled by the stall read
> in pending pipelined transactions.  It could happen the content is just
> about to be read completely (be just a bit above the threshold).

It won't...

Comment 20

[Patrick McManus [:mcmanus]]

> 
> Also maybe more effective to use directly the value of the chunk size we
> know very early.  If a chunk is large, you have to wait for the whole chunk,
> that may be split among many frames, a long time while you already know the
> size of the whole chunk.
> 

this was a good idea and pretty easy to do.. so I a version of it to the patch


> @@ +1359,5 @@
> > +        mConnInfo,
> > +        static_cast<nsHttpConnectionMgr::PipelineFeedbackInfoType>(reason),
> > +        nsnull, mClassification);
> > +
> > +    mConnection->CancelPipeline(NS_ERROR_CORRUPTED_CONTENT);
> 
> Really NS_ERROR_CORRUPTED_CONTENT ?
> 

I changed it to error_abort

Comment 21

[Patrick McManus [:mcmanus]]

Attachment: v10

updated patch fyi

Comment 22

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89ff973f27e6

Comment 23

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound because something in the push appeared to cause a leak in mochitest-browser-chrome:
https://hg.mozilla.org/integration/mozilla-inbound/rev/577da08878cb

Comment 24

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87ea59a6d162

Comment 25

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/87ea59a6d162