Add cross origin to Blink-driven preconnect

net/socket/client_socket_pool_manager.cc

Index: net/socket/client_socket_pool_manager.cc
diff --git a/net/socket/client_socket_pool_manager.cc b/net/socket/client_socket_pool_manager.cc
index 7820cf9d9ab3d964ddc25a90a9b9d6059bc10fee..d527472dc400a476381bdebcf3dae744dd3644a9 100644
--- a/net/socket/client_socket_pool_manager.cc
+++ b/net/socket/client_socket_pool_manager.cc
@@ -215,8 +215,15 @@ int InitSocketPoolHelper(ClientSocketPoolManager::SocketGroupType group_type,
     }
   }
 
+  // Turn on privacy mode if it was enabled, or if the load flags indicate
+  // that credentials cannot be sent.
+  bool enable_privacy_mode = (load_flags & LOAD_DO_NOT_SEND_COOKIES) ||
+                             (load_flags & LOAD_DO_NOT_SAVE_COOKIES) ||
+                             (load_flags & LOAD_DO_NOT_SEND_AUTH_DATA) ||
+                             (privacy_mode == PRIVACY_MODE_ENABLED);

[mmenke, 2015/07/14 16:32:03]

I guess this is supposed to mirror the logic in URLRequestHttpJob::Start? 
There's a mismatch here, though.  URLRequestHttpJob::Start doesn't check
LOAD_DO_NOT_SEND_AUTH_DATA, but this does.

That seems problematic to me.  I'm no expert on Privacy Mode, but doesn't this
mean we may use channel ID on some privacy mode connections? 
HttpNetworkTransaction::Start has logic to disable channel ID on PM connections.
 Moreover, there's a whole bunch of other logic based on the presence or absence
of privacy mode at higher layers of the stack, which would now not apply to all
requests that go through the pm group.

Peppering the code with redundant (Or semi-redundant) checks also seems a bit
problematic, as code tends to diverge over time.  If this is something we want
to do in a lot of places, maybe we should use a shared function to perform the
logic?

[Ryan Sleevi, 2015/07/14 18:41:02]

No, it's to mirror
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...

This is still correct, however. The issue is that if you're employing
NTLM/Kerberos/Negotiate, then it's connection oriented - and the entire
connection is tainted with implicit authority that shouldn't be there.
I'm not sure I follow how that conclusion was reached?
Not sure I follow this either.
There's different concepts at different layers, and it's all gotten messy since
privacy mode was introduced. This is something we can/should clean-up.

The highest layer is the fetch() abstraction, which is the WebURLRequest bits
and (in parallel) the preconnect logic.
Below that is Chrome policy for determining privacy preserving behaviours of
non-fetch()'d requests. Realistically, we want these to all be consistent, but
there's something that load flags don't express - which is whether the omission
of (cookies, auth) is done for _privacy_ reasons, or if it's done for _other_
reasons. If it's for privacy, we should have separate socket pools. If it's for
some other reason, then it's on a case-by-case basis.


That said, this CL resolves the conflict by pushing it to the lowest layer and
assuming it is always done for privacy reasons, and that's... OK. I think.
Otherwise it means duplicating the logic each time the web platform glues with
//net, and I don't think we'll end up with common interfaces for that, since the
glue is necessarily uncommon.

[mmenke, 2015/07/14 18:52:12]

So then why doesn't
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
check that flag?

Also, what if only a subset of those flags are set?  Send cookies, but don't
save them...Should that really go to the privacy mode pool?  Or if auth is
allowed, but we don't want to send cookies?
HttpNetworkTransaction::Start disables channel ID if privacy mode was enabled. 
So suppose we just have a request issued with LOAD_DO_NOT_SEND_AUTH_DATA set,
but not privacy mode.  Then HttpNetworkTransaction::Start will not disable
channel ID, but the request will end up in the privacy mode connection pool.  We
have a similar check in URLRequestJob to enable privacy mode, but it only checks
the cookie flags, not the AUTH flag (And not sure how websockets comes into play
here, either, for that matter).

It seems to me like either we should bundle all those flags as one, or if not
all of them are set, we should not send requests to the pm pool (Which taints
the pool, no?)
Looks like I was wrong about other higher layer checks - I though QUIC and SPDY
had some, but SPDY only has them in tests, and QUIC doesn't go through this
code.

[Ryan Sleevi, 2015/07/14 18:55:32]

Oof, yeah, good catch on the bug.

Basically, the logic needs to be if you don't want one (for privacy reasons) you
shouldn't get any. You basically need a new socket any time the
fetch-without-credentials flag is set (the high level), but the low-level bits
are... messier. Which means we probably shouldn't be touching this here, because
of that.

[mmenke, 2015/07/14 19:10:52]

I'm skeptical of the idea of sharing a URLRequestContext between pm and non-pm
mode connections.  Seems like there's a huge amount of state that's potentially
shared, and we really don't want any of it.

+

[mmenke, 2015/07/14 16:32:03]

The existing test for privacy mode socket use is
HttpStreamFactoryTest.PrivacyModeUsesDifferentSocketPoolGroup.  You could just
add a test there.

That seems a bit high up the stack to be testing socket in net/socket, to me -
I'd really prefer a couple tests in net/socket for both PRIVACY_MODE_ENABLED and
the new logic - but I'm reluctant to ask you to do it, seeing as how much yak
shaving we've already done on this CL.

   // Change group name if privacy mode is enabled.
-  if (privacy_mode == PRIVACY_MODE_ENABLED)
+  if (enable_privacy_mode)
     connection_group = "pm/" + connection_group;
 
   // Deal with SSL - which layers on top of any given proxy.