1283319 - Make connections container aware, and use separate connections for each container.

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

Email to containers@ asking about connection reuse below.  This bug is to determine if we reuse the channel and try to figure out how to fix it if we do.

Hey,

A colleague of ours was wondering if our implementation of containers
ensures to not reuse connections across containers:
https://twitter.com/sleevi_/status/743872814181027841. Because if we
do reuse connections, we lose. Daniel suspected we might not ensure
that, since the feature is implemented at a higher-level.

Kind regards,


-- 
https://annevankesteren.nl/

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I think this is something necko folks would know the most about. I do know that while we do make an effort to separate private and normal contexts in PSM (for example, TLS intolerance), there isn't any similar mechanism in place for different containers (this is minor in comparison to Sleevi's concern about cookie reuse, though).

Comment 2

[Tanvi Vyas[:tanvi]]

Honza, do you know if connections are separated by OriginAttributes?  Can you point us to the place in the code where connections are created?

Thanks!

Comment 3

[Anne (:annevk)]

David, the concern is not cookie reuse, but being able to detect the user is the same through e.g., TLS session IDs, connection reuse, etc. (Which is the same as reusing cookies, when it comes down to it.)

Comment 4

[Anne (:annevk)]

Basically, you saying this is different from private vs normal makes me worried we're not doing the right thing here, as that separation is also what we use for credentialed vs non-credentialed fetches. (And also makes me wonder how we're planning on implementing private in terms of this, as I overheard somewhere...)

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #2)
> Honza, do you know if connections are separated by OriginAttributes?  Can
> you point us to the place in the code where connections are created?
> 
> Thanks!

They are not separated by OA.  

You may be more interested in place where we distinguish connections, which is here:

https://dxr.mozilla.org/mozilla-central/rev/b69a5bbb5e40bd426e35222baa600b481e50d265/netwerk/protocol/http/nsHttpConnectionInfo.cpp#115

You simply have to append origin attrs suffix to the mHashKey string.

Comment 6

[Ethan Tseng [:ethan]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> They are not separated by OA.  
> You may be more interested in place where we distinguish connections, which
> is here:
> https://dxr.mozilla.org/mozilla-central/rev/
> b69a5bbb5e40bd426e35222baa600b481e50d265/netwerk/protocol/http/
> nsHttpConnectionInfo.cpp#115
> You simply have to append origin attrs suffix to the mHashKey string.

Hi Honza,
Thanks for the information, which directly provides the answer and solution for us!

I was trying to prove this issue (cookies are not really segregated by containers due to connection reuse) today, but couldn't make it happen and I didn't know connection reuse happens or not.

I talked to Valentin. It seems we don't have test cases for connection reuse.
Without test cases, do you have idea on how to prove this issue?
That could help us to verify the fix once we have a patch.

Comment 7

[Dave Huseby [:huseby]]

It seems to me that writing a test that serves a page and sub-resources with no-cache headers is in order.

> Cache-Control: no-cache, no-store, must-revalidate
> Pragma: no-cache
> Expires: 0

If the served content isn't cached, and the HTML makes lots of sub-resource requests, that we could then open the page in two containers and trigger connection reuse, if I understand the necko code correctly.

adding the origin attributes to the cache key calculation is the right solution here.  it is what has worked in all other subsystems.

Comment 8

[Dave Huseby [:huseby]]

I was just looking at nsHttpConnectionInfo to see how difficult it will be to get the origin attributes information into the hash key.  The nsHttpConnectionInfo doesn't have any reference to an object through which it can get the origin attributes.  That means we'll need to feed some state/reference into the nsHttpConnectionInfo class, OR, we can add an abstract reference to an "added information" source that then does a late binding on the origin attributes.

The simple and straight forward solution is to pass origin attributes into the nsHttpConnectionInfo class upon construction.  The downside to this solution is that there are about 12 places where we create nsHttpConnectionInfo instances per ( https://dxr.mozilla.org/mozilla-central/search?q=%2Btype-ref%3Amozilla%3A%3Anet%3A%3AnsHttpConnectionInfo ).  I haven't looked at all of those creation locations to see if the origin attributes state is accessible from those call sites.  If not, then things get even more complicated.

So let's just assume we figure out how to get origin attributes into the nsHttpConnectionInfo, then it appears that we will successfully bucket Http connections base on OA's.

Comment 9

[Tanvi Vyas[:tanvi]]

Is the channel that initiated the connection available in the nsHttpConnectionInfo?  If it is, we can just get the originAttributes from the channel's loadinfo.

Ethan, you may need a server setup in order to determine that the same connection is being used.  Either way, we know now this is a bug and requires an implementation change.

Comment 10

[Ethan Tseng [:ethan]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #9)
> Ethan, you may need a server setup in order to determine that the same
> connection is being used.  Either way, we know now this is a bug and
> requires an implementation change.
I have an Apache server running on my own PC in the office.
I will set up it to use HTTPS and HTTP/2 for experiment.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Dave Huseby [:huseby] from comment #7)
> adding the origin attributes to the cache key calculation is the right
> solution here.  it is what has worked in all other subsystems.

I don't understand what you mean here.  HTTP cache is using originsuffix string which contains userctxid.

This bug is about isolation of HTTP _connections_ and has nothing to do with caching.  A manual test doesn't need any messing with caching.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Ethan Tseng [:ethan] from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > They are not separated by OA.  
> > You may be more interested in place where we distinguish connections, which
> > is here:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > b69a5bbb5e40bd426e35222baa600b481e50d265/netwerk/protocol/http/
> > nsHttpConnectionInfo.cpp#115
> > You simply have to append origin attrs suffix to the mHashKey string.
> 
> Hi Honza,
> Thanks for the information, which directly provides the answer and solution
> for us!
> 
> I was trying to prove this issue (cookies are not really segregated by
> containers due to connection reuse) today, but couldn't make it happen and I
> didn't know connection reuse happens or not.
> 
> I talked to Valentin. It seems we don't have test cases for connection reuse.
> Without test cases, do you have idea on how to prove this issue?
> That could help us to verify the fix once we have a patch.

We don't have a test, AFAIK.  All you need to do is to have a server (even just a local) that is capable of keep-alive connections.  Usually, standard installation of Apache or IIS do, and most hosting providers support it these days by default.  

The following should tell you 

Have two windows in two different contexts.  Load a page X - just a plain text "Hello world" - in the first window.  If you do it the first time, you should see a new connection to the server made (checked by e.g. netstat).  When load is done, check the connection is left open (is in the ESTABLISHED state).  Now quickly switch to the second window and load the same page X.  

If we correctly isolated, another connection to the server should be created.  If you reload the page in each window, the existing connection (you can recognize by the outbound port number) should be used respectively.

But what you will probably see is reuse of the first connection to load the page in the second window --> after the second load is performed, there will only be one connection open.

Comment 13

[Honza Bambas (:mayhemer)]

If we fix this in general, I think it should "just work" for http/2 as well.  Check with Patrick McManus on that, since we do some coalescing of connections for http/2.

To pass the originsuffix string down, I think you just need to update nsHttpConnectionInfo to carry it.  Then see the places it's being created at (we use plain |new| to instantiate it).  You will have to deal with AltServiceMapping too, but that's more Patrick's or Nick's area.

Comment 14

[Ethan Tseng [:ethan]]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> We don't have a test, AFAIK.  All you need to do is to have a server (even
> just a local) that is capable of keep-alive connections.  Usually, standard
> installation of Apache or IIS do, and most hosting providers support it
> these days by default.
> The following should tell you 

Hi Honza,

Thanks for your detailed explanation!

Yes, I set up an Apache server to verify it.
I also took Paul's advice to use "Node.js + http module + express" to simulate an HTTP server,
which is much quicker and simpler. (Thanks Paul!)

The conclusion is as expected, connections are NOT isolated between container tabs.
Different context containers are using the same connection.

Jonathan took this bug and I'll assist him to verify the fix once we have a patch.

Comment 15

[Valentin Gosu [:valentin] (he/him)]

It was my understanding that TOR also has a similar behaviour (using different connections per tab & domain).
If that's the case (I might be wrong), maybe we could use/adapt one of the patches for this bug?

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #15)
> It was my understanding that TOR also has a similar behaviour (using
> different connections per tab & domain).
> If that's the case (I might be wrong), maybe we could use/adapt one of the
> patches for this bug?

Can you point to a patch?  Maybe, but I think having simply our own patch as suggested will do.  It's easy to write, harder is to test it in an automated way...

Comment 17

[Jonathan Hao (inactive) [:jhao]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> If we fix this in general, I think it should "just work" for http/2 as well.
> Check with Patrick McManus on that, since we do some coalescing of
> connections for http/2.
> 
> To pass the originsuffix string down, I think you just need to update
> nsHttpConnectionInfo to carry it.  Then see the places it's being created at
> (we use plain |new| to instantiate it).  You will have to deal with
> AltServiceMapping too, but that's more Patrick's or Nick's area.

Do you mean we have to add origin attributes to AltServiceMapping::MakeHashKey()?

Patrick, what do you think?

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Attachment: Add origin attributes to nsHttpConnectionInfo's hash key.

Here's a WIP patch, but there is still one place I don't know how to get origin attributes.

nsHttpConnectionMgr::ReportFailedToProcess(nsIURI *uri)
http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnectionMgr.cpp#1022

Tracing up a bit:

http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#2164
http://searchfox.org/mozilla-central/source/image/ProgressTracker.cpp#563

So this connection info is created when there's an image decoding error. The error can come from these places: http://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla5image15ProgressTracker18SyncNotifyProgressEjRKNS_3gfx12IntRectTypedINS2_12UnknownUnitsEEE&redirect=false

I don't know how I can get origin attributes in these image classes.

Honza, any idea?

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Jonathan Hao [:jhao] from comment #18)
> Created attachment 8770906 [details] [diff] [review]
> Add origin attributes to nsHttpConnectionInfo's hash key.
> 
> Here's a WIP patch, but there is still one place I don't know how to get
> origin attributes.
> 
> nsHttpConnectionMgr::ReportFailedToProcess(nsIURI *uri)
> http://searchfox.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpConnectionMgr.cpp#1022

This is just reporter to HTTP pipelining stats.  It's just an alert that may help decide whether to use or not use pipelining with the specific host.  

Note that pipelining is off by default now and about to be removed from the code base.

To correctly implement it, instead of "report the event for all the permutations of anonymous and private versions of this host" you would need to iterate over the whole mCT hashtable at http://searchfox.org/mozilla-central/rev/e269c68bc594b4a858624d73f0d9eb002f3c0844/netwerk/protocol/http/nsHttpConnectionMgr.cpp#973 and match by host+port+username.  Then notify each matching nsConnectionEntry individually.  Doable, but that is IMO overkill, I would just block this on removing the pipelining code or just pass context=0 or something here.

Up to you to, sync with Patrick on this.

Comment 20

[Patrick McManus [:mcmanus]]

The fact that connections are colaesced across first party is in general a feature we don't want to lose by simply adding OA to the hash key. That's a design that makes the web work better (it improves latency, has better congestion control properties, etc..)

However, I'm fine with segmentation via explicit personality/container or even origin-attribute via opt-in pref ala tor. This is much like we currently do segment PB vs non-PB.

make sense?

(also, the TLS session ticket thing probably needs to be addressed..)

Comment 21

[Jonathan Hao (inactive) [:jhao]]

Tanvi, per Patrick's comment, do you think we should create a pref like privacy.userContext.connection_reuse.disabled for this?

Comment 22

[Patrick McManus [:mcmanus]]

(In reply to Jonathan Hao [:jhao] from comment #21)
> Tanvi, per Patrick's comment, do you think we should create a pref like
> privacy.userContext.connection_reuse.disabled for this?

so that sounds like it would be more global than you want.. don't you want to scope the sharing to the user context (not to the origin attribute necessarily)?

Comment 23

[Jonathan Hao (inactive) [:jhao]]

(In reply to Patrick McManus [:mcmanus] from comment #22)
> (In reply to Jonathan Hao [:jhao] from comment #21)
> > Tanvi, per Patrick's comment, do you think we should create a pref like
> > privacy.userContext.connection_reuse.disabled for this?
> 
> so that sounds like it would be more global than you want.. don't you want
> to scope the sharing to the user context (not to the origin attribute
> necessarily)?

I don't think we want to limit the scope to user context because that's the point of having origin attributes, so that a single mechanism can deal with many separation criteria. For example, origin attributes will have first-party URI soon (pref off). Besides, the other attributes (appId and inIsolatedBrowser) are not that important since b2g is gone.

The name I proposed, "privacy.userContext.connection_reuse.disabled", is probably not general enough. How about something like: "network.http.connection_reuse.privacy_level"? 

Level 0: the original behavior
Level 1: disable reuse across different origin attributes

However, it may be weird to have only two levels.

Comment 24

[Patrick McManus [:mcmanus]]

it sounds like what you're proposing is that if someone is using 15 regular tabs including one home gmail tab, plus one gmail work tab then we would have 16 connection pools?

I think that ought to be 2 pools. (maybe 3)

I'm fine with having a config that gives you 16 pools (tor mode) - but I don't think just using containers should make that happen as I understand containers anyhow.

Comment 25

[Jonathan Hao (inactive) [:jhao]]

(In reply to Patrick McManus [:mcmanus] from comment #24)
> it sounds like what you're proposing is that if someone is using 15 regular
> tabs including one home gmail tab, plus one gmail work tab then we would
> have 16 connection pools?

No, that's not what I was proposing.  Does my WIP patch actually did that?  Or is it because my proposed pref name is confusing? If so, we can come up with a better one.

> I think that ought to be 2 pools. (maybe 3)

Yes, connection should be reused in each container. We're only concerned about connection reuse across containers.  My patch adds origin attributes to connection info's hash key. Currently, the origin attributes should be the same for pages in the same container.

Comment 26

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Removing [userContextId] to help clean up tracking. This is closer to [oa] than containers (no UI) and containers depends on [oa] work anyways.

Comment 27

[Patrick McManus [:mcmanus]]

(In reply to Jonathan Hao [:jhao] from comment #25)
> Currently, the origin attributes should be
> the same for pages in the same container.

That's what I misunderstood, thanks. Is there a corrollary for each tab or first party or something like that?

can you give an example of what CreateSuffix generates for my edification?

It sounds like this can be on by default as long as the pooling behavior of the non-container use case is unchanged.

Comment 28

[Tanvi Vyas[:tanvi]]

Not sure if my needinfo is needed anymore....  Each container should share its own connection pool.  So that if the user has the default container, personal container, and shopping container open, they woudl have 3 connection pools.  If they also had private browsing mode open, they should have 4.  google.com/maps and google.com/search in the default container can use the same connection, but google.com/plus in the shopping container must use a different connection.


(P.S. Sorry for all the whiteboard tag switching yesterday.)

Comment 29

[Jonathan Hao (inactive) [:jhao]]

(In reply to Patrick McManus [:mcmanus] from comment #27)
> That's what I misunderstood, thanks. Is there a corrollary for each tab or
> first party or something like that?

Dave is adding first-party URI into origin attributes, but that will be pref'd off.

> can you give an example of what CreateSuffix generates for my edification?

CreateSuffix would generate something like (without quote): "^userContextId=2&privateBrowsingId=1"
And if an attribute has a default value (e.g. zero for userContextId), it wouldn't appear in the suffix.  So if userContextId is 0 in the previous example, then the suffix would just be (without quote) "^privateBrowsingId=1".

> It sounds like this can be on by default as long as the pooling behavior of
> the non-container use case is unchanged.

If all attributes have default values, the suffix will be an empty string, so the non-container use case will be unchanged for sure.

Comment 30

[Jonathan Hao (inactive) [:jhao]]

Attachment: Add origin attributes to nsHttpConnectionInfo's hash key.

I use default origin attributes in nsHttpConnectionMgr::ReportFailedToProcess() as Honza suggested in https://bugzil.la/1283319#c19.

Patrick, would you like to review this patch?

Comment 31

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8774238 [details] [diff] [review]
Add origin attributes to nsHttpConnectionInfo's hash key.

Honza, could you review this patch?

Comment 32

[Honza Bambas (:mayhemer)]

Comment on attachment 8774238 [details] [diff] [review]
Add origin attributes to nsHttpConnectionInfo's hash key.

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

r+ with comments

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1710,5 @@
>  
>      AltSvcMapping::ProcessHeader(altSvc, scheme, originHost, originPort,
>                                   mUsername, mPrivateBrowsing, callbacks, proxyInfo,
> +                                 mCaps & NS_HTTP_DISALLOW_SPDY,
> +                                 mLoadInfo->GetOriginAttributes());

not sure, but I think there still can be channel w/o load info?

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +163,5 @@
>      }
>  
> +    nsAutoCString originAttributes;
> +    mOriginAttributes.CreateSuffix(originAttributes);
> +    mHashKey.Append(originAttributes);

I'd rather see this appended as the very last string.  it's nicely isolated by the first '^' character, which is always present (if the suffix would be non-empty).

Comment 33

[Jonathan Hao (inactive) [:jhao]]

Attachment: Add origin attributes to nsHttpConnectionInfo's hash key.

I moved the origin attribute suffix to then end of the hash key, and use NS_GetOriginAttributes in nsHttpChannel.cpp.

Hi Honza. Although you already r+'ed but I think it's better to let you double check the changes to nsHttpChannel.

Thanks.

Comment 34

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06442c481416&selectedJob=25174607

Comment 35

[Patrick McManus [:mcmanus]]

Comment on attachment 8777707 [details] [diff] [review]
Add origin attributes to nsHttpConnectionInfo's hash key.

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

this lgtm. would you please add an xpcshell test or two around this.

specifically I'm concerned about the case of 2 tabs in the default container regressing so that it would require two connections if the originAttributes text changed.. maybe just a test for that and its opposite (the case where they really should be separated).

It won't be too hard to do.. you can add stuff that's really just for testing to nsIHttpChannelInternal.idl (comment that its just for testing).. I would add an attribute that returns HTTPBaseChannel's mConnectionInfo.HashKey().. which you ought to be able to grab out of OnStartRequest() without a problem in a test

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +224,5 @@
>      }
> +
> +    nsAutoCString originAttributes;
> +    mOriginAttributes.CreateSuffix(originAttributes);
> +    mHashKey.Append(originAttributes);

can you update the comments earlier in this fx?

Comment 36

[Jonathan Hao (inactive) [:jhao]]

(In reply to Patrick McManus [:mcmanus] PTO until Aug 15 from comment #35)
> Comment on attachment 8777707 [details] [diff] [review]
> Add origin attributes to nsHttpConnectionInfo's hash key.
> 
> Review of attachment 8777707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this lgtm. would you please add an xpcshell test or two around this.
> 
> specifically I'm concerned about the case of 2 tabs in the default container
> regressing so that it would require two connections if the originAttributes
> text changed.. maybe just a test for that and its opposite (the case where
> they really should be separated).

Not sure what you mean by "if the originAttributes changed".  Anyway, I'll add a test for 2 tabs in default/non-default/different containers and check whether their hash keys are the same.

Comment 37

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests for connection info's hash key.

Comment 38

[Honza Bambas (:mayhemer)]

Comment on attachment 8779230 [details] [diff] [review]
Tests for connection info's hash key.

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

::: netwerk/test/unit/test_separate_connections.js
@@ +1,3 @@
> +Cu.import("resource://testing-common/httpd.js");
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

write comment what this test does in general and list each step

@@ +8,5 @@
> +
> +let httpserv = null;
> +
> +function handler(metadata, response) {
> +  response.setHeader("Content-Type", "text/plain", false);

maybe add Cache-control: no-cache as well

@@ +16,5 @@
> +}
> +
> +function makeChan(url, userContextId) {
> +  let chan = NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
> +  chan.loadInfo.originAttributes = { userContextId };

I'm no JS master, but should this be { userContextId: userContextId }?

@@ +40,5 @@
> +          do_check_neq(hashKey, hashKeys[id]);
> +        }
> +      }
> +    }
> +    hashKeys[userContextId] = hashKey;

if userContextId is something undefined, you will never perform your checks.  I'd rather carry the contextId in the Listener class (then you really see that what you want is what you get)

@@ +67,5 @@
> +
> +  // First round.
> +  doTest();
> +  // Compare the hash keys with the first round.
> +  doTest();

seems like you count on requests going out and responses coming in the order you make them.  please don't.  this needs to be synchronized.

Comment 39

[Jonathan Hao (inactive) [:jhao]]

Thank you, Honza.

(In reply to Honza Bambas (:mayhemer) from comment #38)
> Comment on attachment 8779230 [details] [diff] [review]
> Tests for connection info's hash key.
> 
> Review of attachment 8779230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/test/unit/test_separate_connections.js
> @@ +1,3 @@
> > +Cu.import("resource://testing-common/httpd.js");
> > +Cu.import("resource://gre/modules/NetUtil.jsm");
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> 
> write comment what this test does in general and list each step
> 
> @@ +8,5 @@
> > +
> > +let httpserv = null;
> > +
> > +function handler(metadata, response) {
> > +  response.setHeader("Content-Type", "text/plain", false);
> 
> maybe add Cache-control: no-cache as well
> 
> @@ +16,5 @@
> > +}
> > +
> > +function makeChan(url, userContextId) {
> > +  let chan = NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
> > +  chan.loadInfo.originAttributes = { userContextId };
> 
> I'm no JS master, but should this be { userContextId: userContextId }?

{ a } is a syntactic sugar for { a: a }, but I can surely use the latter if it's clearer.

> @@ +40,5 @@
> > +          do_check_neq(hashKey, hashKeys[id]);
> > +        }
> > +      }
> > +    }
> > +    hashKeys[userContextId] = hashKey;
> 
> if userContextId is something undefined, you will never perform your checks.
> I'd rather carry the contextId in the Listener class (then you really see
> that what you want is what you get)
> 
> @@ +67,5 @@
> > +
> > +  // First round.
> > +  doTest();
> > +  // Compare the hash keys with the first round.
> > +  doTest();
> 
> seems like you count on requests going out and responses coming in the order
> you make them.  please don't.  this needs to be synchronized.

I don't need them in the order I make them, as long as every userContextId is opened twice.  But making them synchronized is better and I'll fix that.

Comment 40

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests for connection info's hash key.

Comment 41

[Honza Bambas (:mayhemer)]

Comment on attachment 8780027 [details] [diff] [review]
Tests for connection info's hash key.

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

thanks, looks better, few nits and you are good to go

::: netwerk/test/unit/test_separate_connections.js
@@ +44,5 @@
> +
> +    do_check_eq(request.loadInfo.originAttributes.userContextId, this.userContextId);
> +
> +    let hashKey = request.connectionInfoHashKey;
> +    if (gTestsRun <= 3) {

< 3 ?  I'd rather see a different flag saying "round two" or something set in OnStopRequest when gTestsRun == 3 and you are starting the second round.

@@ +68,5 @@
> +    if (gTestsRun == 3) {
> +      // Do the second round.
> +      doTest();
> +    }
> +    do_test_finished();

you are forgetting to stop the http server.

Comment 42

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests for connection info's hash key.

Comment 43

[Jonathan Hao (inactive) [:jhao]]

Thanks, Honza.

Comment 44

[Honza Bambas (:mayhemer)]

I had to r-.  Please see how servers are stopped in other tests.  You must not do_test_finished() before the server is gone.

Usually it's something like httpserver.stop(do_test_finished);

Comment 45

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests for connection info's hash key.

Thank, Honza, for double checking.

Comment 46

[Honza Bambas (:mayhemer)]

Comment on attachment 8782244 [details] [diff] [review]
Tests for connection info's hash key.

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

thanks.

Comment 47

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1aba9d704e7&selectedJob=26115240

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2abf925226d
Add origin attributes to nsHttpConnectionInfo's hash key. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8be09dc649
Tests for connection info's hash key. r=mayhemer

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2abf925226d
https://hg.mozilla.org/mozilla-central/rev/ac8be09dc649

Comment 50

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
* fx51.0a1, buildid: 20160823030224, changeset: 24763f58772d
** https://archive.mozilla.org/pub/firefox/nightly/2016/08/2016-08-23-03-02-24-mozilla-central/

I used the following test case based on comment#12:

* setup a basic node.js server that accepts incoming TCP connections
* opened http://localhost:8000/ in a non-container tab
* opened http://localhost:8000/ in the Personal, Work, Banking and Shopping containers
* used "sudo lsof -i -n -P | grep TCP" under the terminal (macOS/Ubuntu)
** sysinternals tcpview was used under Windows

Even though every container had http://localhost:8000/ opened, only a single connection was being listed/used:

node 5838 kjozwiak 11u IPv6 0xa303457327ece293 0t0 TCP *:8000 (LISTEN)
node 5838 kjozwiak 14u IPv6 0xa30345732a109d53 0t0 TCP [::1]:8000->[::1]:53480 (ESTABLISHED)
firefox 8598 kjozwiak 53u IPv6 0xa30345732286d813 0t0 TCP [::1]:53480->[::1]:8000 (ESTABLISHED)

Went through verification using the following builds & platforms:

* fx53.0a1, buildid: 20170103030204, changeset: c91249f41e37
** https://archive.mozilla.org/pub/firefox/nightly/2017/01/2017-01-03-03-02-04-mozilla-central/
* fx52.0a2, buildid: 20170103004005, changeset: a6f6cf418098
** https://archive.mozilla.org/pub/firefox/nightly/2017/01/2017-01-03-00-40-05-mozilla-aurora/
* fx51.0b11, buildid: 20170103031119, changeset: 0a17d3922070
** https://archive.mozilla.org/pub/firefox/candidates/51.0b11-candidates/build1/

Platforms being used:

* macOS 10.12.2 x64 - PASSED
* Ubuntu 16.04.1 LTS x64 - PASSED
* Windows 10 x64 - PASSED

Example of several containers having http://localhost:8000/ opened:

node 5838 kjozwiak 11u IPv6 0xa303457327ece293 0t0 TCP *:8000 (LISTEN)
node 5838 kjozwiak 14u IPv6 0xa3034573255ec813 0t0 TCP [::1]:8000->[::1]:51781 (ESTABLISHED)
node 5838 kjozwiak 15u IPv6 0xa3034573255ff293 0t0 TCP [::1]:8000->[::1]:51783 (ESTABLISHED)
node 5838 kjozwiak 16u IPv6 0xa303457327ecd813 0t0 TCP [::1]:8000->[::1]:51787 (ESTABLISHED)
node 5838 kjozwiak 17u IPv6 0xa303457320442293 0t0 TCP [::1]:8000->[::1]:51791 (ESTABLISHED)
firefox 6579 kjozwiak 55u IPv6 0xa30345732aa7e293 0t0 TCP [::1]:51781->[::1]:8000 (ESTABLISHED)
firefox 6579 kjozwiak 56u IPv6 0xa3034573255ecd53 0t0 TCP [::1]:51783->[::1]:8000 (ESTABLISHED)
firefox 6579 kjozwiak 75u IPv6 0xa3034573203aa293 0t0 TCP [::1]:51787->[::1]:8000 (ESTABLISHED)
firefox 6579 kjozwiak 76u IPv6 0xa303457320441d53 0t0 TCP [::1]:51791->[::1]:8000 (ESTABLISHED)

Other test cases used using the base example from above:

* ensured custom containers are using their own connections
* ensured PB is using it's own connection and not sharing them with containers/non-container tabs
* ensured that the same containers are not creating new connections
* ensured that two different containers that are named identically are not reusing the same connection
* ensured that the non-container tabs are not creating new connections
* ensured that the non-e10s window isn't creating a new connection if http://localhost:8000/ is already being used by a non-container tab
* restoring previous sessions using "Show my window and tabs from last time" via about:preferences#general
* restoring previous sessions using "Restore Previous Sessions" via about:home

Possible Issue:

With fx51, whenever you load http://localhost:8000/ into a container first, you'll notice that two connections will get established rather than just the one as expected. The two connections belong to the container being used and the default container:

STR:

* setup a basic node.js server that accepts incoming TCP connections (http://localhost:8000/)
* install the latest version of fx51
** https://archive.mozilla.org/pub/firefox/candidates/51.0b11-candidates/build1/
* enable containers under about:config via the following prefs and restart the browser
** privacy.userContext.enabled;true
** privacy.userContext.ui.enabled;true
* open a "Personal" container via the File Menu -> New Container Tab
* load http://localhost:8000/ in the personal container
* use "sudo lsof -i -n -P | grep TCP" under the terminal (macOS/Ubuntu) and you'll notice that two TCP connections will be established

If you use the same exact steps while using fx53 or fx52, you'll get a single established connection rather than the two. I'm not sure if this is a concern. I've also created a video [1] to illustrate what's happening just to make things easier :)

[1] https://youtu.be/-1IJfdwZTWY

Comment 51

[Kamil Jozwiak [:kjozwiak]]

Jonathan, could you take a quick look at the issue I mentioned above that's affecting FX51? Let me know if there's anything that I can help out with.

Comment 52

[Jonathan Hao (inactive) [:jhao]]

Thanks, Kamil.  Are you sure that the version you tested includes the patches in this bug?  The patches were landed at about 10pm CST.  I tried the nightly one day later, and I can't reproduce this bug.

The second issue you mentioned may or may not be the same bug.  If that version does include the above patches, can we use mozregression or something to narrow down what patch makes fx52 behaves correctly?

Thanks.

Comment 53

[Kamil Jozwiak [:kjozwiak]]

> Thanks, Kamil.  Are you sure that the version you tested includes the
> patches in this bug?  The patches were landed at about 10pm CST.  I tried
> the nightly one day later, and I can't reproduce this bug.

Are you referring to the first portion of comment#50? I just wanted to reproduce the original issue with an older build of m-c so I know the difference between a broken version and the fixed version when I'm going through the verification. Nightly is working as expected :) Apologies for the confusion.
 
> The second issue you mentioned may or may not be the same bug.  If that
> version does include the above patches, can we use mozregression or
> something to narrow down what patch makes fx52 behaves correctly?

I'll try running through mozregression tomorrow morning and see if I can narrow down the patch that's making fx52 and fx51 behave differently.

Comment 54

[Jonathan Hao (inactive) [:jhao]]

Sorry, I misunderstood :)

Comment 55

[Kamil Jozwiak [:kjozwiak]]

I ran through mozregression using "--find-fix" [1] on two different occasions and bug#1277803 was listed both times. It looks like bug#1277803 addressed the issue with favicons not obeying OA when they're being loaded through XUL:image as XUL:image loads recourses using the system principal. This explains why I'm seeing a non-container connection being established when I'm initially loading http://localhost:8000 in a container using fx51. As this landed in fx52, it doesn't affect m-c and m-a which aligns with the results in comment#50.

19:24.73 INFO: First good revision: 60137e6c353000f41aeed0bc4b63c43da718f951
19:24.73 INFO: Last bad revision: 3e153a5acd3597633d67532c4bca294e90ea75aa
19:24.73 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e153a5acd3597633d67532c4bca294e90ea75aa&tochange=60137e6c353000f41aeed0bc4b63c43da718f951

19:25.32 INFO: Looks like the following bug has the changes which introduced the fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=1277803

[1] mozregression --find-fix --bad 2016-09-01 --good 2017-01-04