|
|
Created:
5 years, 10 months ago by Pat Meenan Modified:
5 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEliminated the logic that accumulated multiple preconnect requests
Eliminated the logic that accumulated multiple preconnect requests and
inferred a connection count from the number of requests. The logic
doesn't play well with the preload scanner and I'm working with the spec
authors to come up with a more deterministic way to indicate "many
connections" to differ from the case where devs just want a single
connection.
BUG=450682
Committed: https://crrev.com/16b951d63b306f03ca2825f58489bf6ea9bede8d
Cr-Commit-Position: refs/heads/master@{#317839}
Patch Set 1 #Patch Set 2 : Added browser test #
Total comments: 18
Patch Set 3 : Everything except for custom observer #Patch Set 4 : Switched to custom NetLog observer #
Total comments: 10
Patch Set 5 : Addressed review feedback #
Total comments: 4
Patch Set 6 : Use existing switches and fix runloop #
Messages
Total messages: 32 (2 generated)
pmeenan@chromium.org changed reviewers: + mmenke@chromium.org, ttuttle@chromium.org
One option would be to accumulate preconnect requests in the browser process, and just preconnect cumulatively, since preconnecting one socket, and then preconnecting two to the same domain should only connect a total of two sockets...Though that could go *very* badly if the socket pool is at its connection cap, I suppose. Anyhow, just thought I'd mention it - haven't really thought it through. Have you given any thought to how to write tests for this? We really should have some chrome/ browser tests for the feature, to make sure the preconnects make it to the browser process.
On 2015/02/12 15:42:38, mmenke wrote: > One option would be to accumulate preconnect requests in the browser process, > and just preconnect cumulatively, since preconnecting one socket, and then > preconnecting two to the same domain should only connect a total of two > sockets...Though that could go *very* badly if the socket pool is at its > connection cap, I suppose. Anyhow, just thought I'd mention it - haven't really > thought it through. > > Have you given any thought to how to write tests for this? We really should > have some chrome/ browser tests for the feature, to make sure the preconnects > make it to the browser process. The existing predictor browser tests just wait until it sees the DNS lookup for the request. At a minimum, we should do the same for preconnects, to make sure we're at least doing something. I'd really like to do more, if we can manage it without adding new hooks into net/.
On 2015/02/12 15:47:27, mmenke wrote: > On 2015/02/12 15:42:38, mmenke wrote: > > One option would be to accumulate preconnect requests in the browser process, > > and just preconnect cumulatively, since preconnecting one socket, and then > > preconnecting two to the same domain should only connect a total of two > > sockets...Though that could go *very* badly if the socket pool is at its > > connection cap, I suppose. Anyhow, just thought I'd mention it - haven't > really > > thought it through. > > > > Have you given any thought to how to write tests for this? We really should > > have some chrome/ browser tests for the feature, to make sure the preconnects > > make it to the browser process. > > The existing predictor browser tests just wait until it sees the DNS lookup for > the request. At a minimum, we should do the same for preconnects, to make sure > we're at least doing something. I'd really like to do more, if we can manage it > without adding new hooks into net/. Simplest solution may be to start a second embedded test server, preconnect to it by IP address (Insert JS with the new address or use the spawned test server, which supports substituting one string with another), and watch NetLog (Accessible on the UI thread by g_browser->net_log()) for an event with source type NetLog::SOURCE_SOCKET, event type NetLog::TYPE_TCP_CONNECT, and a source_address parameter matching the ip:port of the test server. I'm open to other ideas, but this seems like just the sort of feature that could be broken for years with no one noticing it. Should have brought this up on your earlier CL.
On 2015/02/12 15:57:05, mmenke wrote: > On 2015/02/12 15:47:27, mmenke wrote: > > On 2015/02/12 15:42:38, mmenke wrote: > > > One option would be to accumulate preconnect requests in the browser > process, > > > and just preconnect cumulatively, since preconnecting one socket, and then > > > preconnecting two to the same domain should only connect a total of two > > > sockets...Though that could go *very* badly if the socket pool is at its > > > connection cap, I suppose. Anyhow, just thought I'd mention it - haven't > > really > > > thought it through. > > > > > > Have you given any thought to how to write tests for this? We really should > > > have some chrome/ browser tests for the feature, to make sure the > preconnects > > > make it to the browser process. > > > > The existing predictor browser tests just wait until it sees the DNS lookup > for > > the request. At a minimum, we should do the same for preconnects, to make > sure > > we're at least doing something. I'd really like to do more, if we can manage > it > > without adding new hooks into net/. > > Simplest solution may be to start a second embedded test server, preconnect to > it by IP address (Insert JS with the new address or use the spawned test server, > which supports substituting one string with another), and watch NetLog > (Accessible on the UI thread by g_browser->net_log()) for an event with source > type NetLog::SOURCE_SOCKET, event type NetLog::TYPE_TCP_CONNECT, and a > source_address parameter matching the ip:port of the test server. > > I'm open to other ideas, but this seems like just the sort of feature that could > be broken for years with no one noticing it. Should have brought this up on > your earlier CL. Thanks, I'll take a look. I had looked earlier but as you noticed the existing predictor tests only tested DNS and not connections and I was concerned about making connections to actual servers. I haven't looked at the embedded test server logic (or for that matter a good chunk of the chrome browser-side testing) so it may be a little while before I come back with an update.
On 2015/02/12 16:02:21, Pat Meenan wrote: > On 2015/02/12 15:57:05, mmenke wrote: > > On 2015/02/12 15:47:27, mmenke wrote: > > > On 2015/02/12 15:42:38, mmenke wrote: > > > > One option would be to accumulate preconnect requests in the browser > > process, > > > > and just preconnect cumulatively, since preconnecting one socket, and then > > > > preconnecting two to the same domain should only connect a total of two > > > > sockets...Though that could go *very* badly if the socket pool is at its > > > > connection cap, I suppose. Anyhow, just thought I'd mention it - haven't > > > really > > > > thought it through. > > > > > > > > Have you given any thought to how to write tests for this? We really > should > > > > have some chrome/ browser tests for the feature, to make sure the > > preconnects > > > > make it to the browser process. > > > > > > The existing predictor browser tests just wait until it sees the DNS lookup > > for > > > the request. At a minimum, we should do the same for preconnects, to make > > sure > > > we're at least doing something. I'd really like to do more, if we can > manage > > it > > > without adding new hooks into net/. > > > > Simplest solution may be to start a second embedded test server, preconnect to > > it by IP address (Insert JS with the new address or use the spawned test > server, > > which supports substituting one string with another), and watch NetLog > > (Accessible on the UI thread by g_browser->net_log()) for an event with source > > type NetLog::SOURCE_SOCKET, event type NetLog::TYPE_TCP_CONNECT, and a > > source_address parameter matching the ip:port of the test server. > > > > I'm open to other ideas, but this seems like just the sort of feature that > could > > be broken for years with no one noticing it. Should have brought this up on > > your earlier CL. > > Thanks, I'll take a look. I had looked earlier but as you noticed the existing > predictor tests only tested DNS and not connections and I was concerned about > making connections to actual servers. I haven't looked at the embedded test > server logic (or for that matter a good chunk of the chrome browser-side > testing) so it may be a little while before I come back with an update. We don't actually need two test servers - we could use a data URL or a URLRequestMockHttpJob to have the page that triggers the preconnect not create a socket.
On 2015/02/12 16:23:07, mmenke wrote: > On 2015/02/12 16:02:21, Pat Meenan wrote: > > On 2015/02/12 15:57:05, mmenke wrote: > > > On 2015/02/12 15:47:27, mmenke wrote: > > > > On 2015/02/12 15:42:38, mmenke wrote: > > > > > One option would be to accumulate preconnect requests in the browser > > > process, > > > > > and just preconnect cumulatively, since preconnecting one socket, and > then > > > > > preconnecting two to the same domain should only connect a total of two > > > > > sockets...Though that could go *very* badly if the socket pool is at its > > > > > connection cap, I suppose. Anyhow, just thought I'd mention it - > haven't > > > > really > > > > > thought it through. > > > > > > > > > > Have you given any thought to how to write tests for this? We really > > should > > > > > have some chrome/ browser tests for the feature, to make sure the > > > preconnects > > > > > make it to the browser process. > > > > > > > > The existing predictor browser tests just wait until it sees the DNS > lookup > > > for > > > > the request. At a minimum, we should do the same for preconnects, to make > > > sure > > > > we're at least doing something. I'd really like to do more, if we can > > manage > > > it > > > > without adding new hooks into net/. > > > > > > Simplest solution may be to start a second embedded test server, preconnect > to > > > it by IP address (Insert JS with the new address or use the spawned test > > server, > > > which supports substituting one string with another), and watch NetLog > > > (Accessible on the UI thread by g_browser->net_log()) for an event with > source > > > type NetLog::SOURCE_SOCKET, event type NetLog::TYPE_TCP_CONNECT, and a > > > source_address parameter matching the ip:port of the test server. > > > > > > I'm open to other ideas, but this seems like just the sort of feature that > > could > > > be broken for years with no one noticing it. Should have brought this up on > > > your earlier CL. > > > > Thanks, I'll take a look. I had looked earlier but as you noticed the existing > > predictor tests only tested DNS and not connections and I was concerned about > > making connections to actual servers. I haven't looked at the embedded test > > server logic (or for that matter a good chunk of the chrome browser-side > > testing) so it may be a little while before I come back with an update. > > We don't actually need two test servers - we could use a data URL or a > URLRequestMockHttpJob to have the page that triggers the preconnect not create a > socket. btw, while I'm in here does it make sense to add mmenke@ as an owner for the network_hints module? Right now it's just ttuttle@ and jar@.
On 2015/02/12 17:09:09, Pat Meenan wrote: > On 2015/02/12 16:23:07, mmenke wrote: > > On 2015/02/12 16:02:21, Pat Meenan wrote: > > > On 2015/02/12 15:57:05, mmenke wrote: > > > > On 2015/02/12 15:47:27, mmenke wrote: > > > > > On 2015/02/12 15:42:38, mmenke wrote: > > > > > > One option would be to accumulate preconnect requests in the browser > > > > process, > > > > > > and just preconnect cumulatively, since preconnecting one socket, and > > then > > > > > > preconnecting two to the same domain should only connect a total of > two > > > > > > sockets...Though that could go *very* badly if the socket pool is at > its > > > > > > connection cap, I suppose. Anyhow, just thought I'd mention it - > > haven't > > > > > really > > > > > > thought it through. > > > > > > > > > > > > Have you given any thought to how to write tests for this? We really > > > should > > > > > > have some chrome/ browser tests for the feature, to make sure the > > > > preconnects > > > > > > make it to the browser process. > > > > > > > > > > The existing predictor browser tests just wait until it sees the DNS > > lookup > > > > for > > > > > the request. At a minimum, we should do the same for preconnects, to > make > > > > sure > > > > > we're at least doing something. I'd really like to do more, if we can > > > manage > > > > it > > > > > without adding new hooks into net/. > > > > > > > > Simplest solution may be to start a second embedded test server, > preconnect > > to > > > > it by IP address (Insert JS with the new address or use the spawned test > > > server, > > > > which supports substituting one string with another), and watch NetLog > > > > (Accessible on the UI thread by g_browser->net_log()) for an event with > > source > > > > type NetLog::SOURCE_SOCKET, event type NetLog::TYPE_TCP_CONNECT, and a > > > > source_address parameter matching the ip:port of the test server. > > > > > > > > I'm open to other ideas, but this seems like just the sort of feature that > > > could > > > > be broken for years with no one noticing it. Should have brought this up > on > > > > your earlier CL. > > > > > > Thanks, I'll take a look. I had looked earlier but as you noticed the > existing > > > predictor tests only tested DNS and not connections and I was concerned > about > > > making connections to actual servers. I haven't looked at the embedded test > > > server logic (or for that matter a good chunk of the chrome browser-side > > > testing) so it may be a little while before I come back with an update. > > > > We don't actually need two test servers - we could use a data URL or a > > URLRequestMockHttpJob to have the page that triggers the preconnect not create > a > > socket. > > btw, while I'm in here does it make sense to add mmenke@ as an owner for the > network_hints module? Right now it's just ttuttle@ and jar@. Since I've never worked on it, and tend to do a lot of reviews, I'd rather not expand what I own, unless it's needed, though, admittedly, this isn't exactly a high traffic directory.
On 2015/02/12 17:14:12, mmenke wrote: > On 2015/02/12 17:09:09, Pat Meenan wrote: > > On 2015/02/12 16:23:07, mmenke wrote: > > > On 2015/02/12 16:02:21, Pat Meenan wrote: > > > > On 2015/02/12 15:57:05, mmenke wrote: > > > > > On 2015/02/12 15:47:27, mmenke wrote: > > > > > > On 2015/02/12 15:42:38, mmenke wrote: > > > > > > > One option would be to accumulate preconnect requests in the browser > > > > > process, > > > > > > > and just preconnect cumulatively, since preconnecting one socket, > and > > > then > > > > > > > preconnecting two to the same domain should only connect a total of > > two > > > > > > > sockets...Though that could go *very* badly if the socket pool is at > > its > > > > > > > connection cap, I suppose. Anyhow, just thought I'd mention it - > > > haven't > > > > > > really > > > > > > > thought it through. > > > > > > > > > > > > > > Have you given any thought to how to write tests for this? We > really > > > > should > > > > > > > have some chrome/ browser tests for the feature, to make sure the > > > > > preconnects > > > > > > > make it to the browser process. > > > > > > > > > > > > The existing predictor browser tests just wait until it sees the DNS > > > lookup > > > > > for > > > > > > the request. At a minimum, we should do the same for preconnects, to > > make > > > > > sure > > > > > > we're at least doing something. I'd really like to do more, if we can > > > > manage > > > > > it > > > > > > without adding new hooks into net/. > > > > > > > > > > Simplest solution may be to start a second embedded test server, > > preconnect > > > to > > > > > it by IP address (Insert JS with the new address or use the spawned test > > > > server, > > > > > which supports substituting one string with another), and watch NetLog > > > > > (Accessible on the UI thread by g_browser->net_log()) for an event with > > > source > > > > > type NetLog::SOURCE_SOCKET, event type NetLog::TYPE_TCP_CONNECT, and a > > > > > source_address parameter matching the ip:port of the test server. > > > > > > > > > > I'm open to other ideas, but this seems like just the sort of feature > that > > > > could > > > > > be broken for years with no one noticing it. Should have brought this > up > > on > > > > > your earlier CL. > > > > > > > > Thanks, I'll take a look. I had looked earlier but as you noticed the > > existing > > > > predictor tests only tested DNS and not connections and I was concerned > > about > > > > making connections to actual servers. I haven't looked at the embedded > test > > > > server logic (or for that matter a good chunk of the chrome browser-side > > > > testing) so it may be a little while before I come back with an update. > > > > > > We don't actually need two test servers - we could use a data URL or a > > > URLRequestMockHttpJob to have the page that triggers the preconnect not > create > > a > > > socket. > > > > btw, while I'm in here does it make sense to add mmenke@ as an owner for the > > network_hints module? Right now it's just ttuttle@ and jar@. > > Since I've never worked on it, and tend to do a lot of reviews, I'd rather not > expand what I own, unless it's needed, though, admittedly, this isn't exactly a > high traffic directory. Any chance you know how to enable experimental blink features in browser_tests? Looks like there are a few bugs open against it but I haven't seen anything that indicates that it has been fixed and preconnect is currently behind an experimental feature flag so it's not running under the browser_tests harness. I have the test harness and netlog hooks mostly in place but there's no way to actually trigger it right now.
On 2015/02/12 21:46:24, Pat Meenan wrote: > On 2015/02/12 17:14:12, mmenke wrote: > > On 2015/02/12 17:09:09, Pat Meenan wrote: > > > On 2015/02/12 16:23:07, mmenke wrote: > > > > On 2015/02/12 16:02:21, Pat Meenan wrote: > > > > > On 2015/02/12 15:57:05, mmenke wrote: > > > > > > On 2015/02/12 15:47:27, mmenke wrote: > > > > > > > On 2015/02/12 15:42:38, mmenke wrote: > > > > > > > > One option would be to accumulate preconnect requests in the > browser > > > > > > process, > > > > > > > > and just preconnect cumulatively, since preconnecting one socket, > > and > > > > then > > > > > > > > preconnecting two to the same domain should only connect a total > of > > > two > > > > > > > > sockets...Though that could go *very* badly if the socket pool is > at > > > its > > > > > > > > connection cap, I suppose. Anyhow, just thought I'd mention it - > > > > haven't > > > > > > > really > > > > > > > > thought it through. > > > > > > > > > > > > > > > > Have you given any thought to how to write tests for this? We > > really > > > > > should > > > > > > > > have some chrome/ browser tests for the feature, to make sure the > > > > > > preconnects > > > > > > > > make it to the browser process. > > > > > > > > > > > > > > The existing predictor browser tests just wait until it sees the DNS > > > > lookup > > > > > > for > > > > > > > the request. At a minimum, we should do the same for preconnects, > to > > > make > > > > > > sure > > > > > > > we're at least doing something. I'd really like to do more, if we > can > > > > > manage > > > > > > it > > > > > > > without adding new hooks into net/. > > > > > > > > > > > > Simplest solution may be to start a second embedded test server, > > > preconnect > > > > to > > > > > > it by IP address (Insert JS with the new address or use the spawned > test > > > > > server, > > > > > > which supports substituting one string with another), and watch NetLog > > > > > > (Accessible on the UI thread by g_browser->net_log()) for an event > with > > > > source > > > > > > type NetLog::SOURCE_SOCKET, event type NetLog::TYPE_TCP_CONNECT, and a > > > > > > source_address parameter matching the ip:port of the test server. > > > > > > > > > > > > I'm open to other ideas, but this seems like just the sort of feature > > that > > > > > could > > > > > > be broken for years with no one noticing it. Should have brought this > > up > > > on > > > > > > your earlier CL. > > > > > > > > > > Thanks, I'll take a look. I had looked earlier but as you noticed the > > > existing > > > > > predictor tests only tested DNS and not connections and I was concerned > > > about > > > > > making connections to actual servers. I haven't looked at the embedded > > test > > > > > server logic (or for that matter a good chunk of the chrome browser-side > > > > > testing) so it may be a little while before I come back with an update. > > > > > > > > We don't actually need two test servers - we could use a data URL or a > > > > URLRequestMockHttpJob to have the page that triggers the preconnect not > > create > > > a > > > > socket. > > > > > > btw, while I'm in here does it make sense to add mmenke@ as an owner for the > > > network_hints module? Right now it's just ttuttle@ and jar@. > > > > Since I've never worked on it, and tend to do a lot of reviews, I'd rather not > > expand what I own, unless it's needed, though, admittedly, this isn't exactly > a > > high traffic directory. > > Any chance you know how to enable experimental blink features in browser_tests? > Looks like there are a few bugs open against it but I haven't seen anything that > indicates that it has been fixed and preconnect is currently behind an > experimental feature flag so it's not running under the browser_tests harness. > I have the test harness and netlog hooks mostly in place but there's no way to > actually trigger it right now. A quick search shows that there is a "--enable-experimental-web-platform-features" switch. Whether it works, or enables preconnecting, I couldn't say.
On 2015/02/12 22:08:55, mmenke wrote: > On 2015/02/12 21:46:24, Pat Meenan wrote: > > On 2015/02/12 17:14:12, mmenke wrote: > > > On 2015/02/12 17:09:09, Pat Meenan wrote: > > > > On 2015/02/12 16:23:07, mmenke wrote: > > > > > On 2015/02/12 16:02:21, Pat Meenan wrote: > > > > > > On 2015/02/12 15:57:05, mmenke wrote: > > > > > > > On 2015/02/12 15:47:27, mmenke wrote: > > > > > > > > On 2015/02/12 15:42:38, mmenke wrote: > > > > > > > > > One option would be to accumulate preconnect requests in the > > browser > > > > > > > process, > > > > > > > > > and just preconnect cumulatively, since preconnecting one > socket, > > > and > > > > > then > > > > > > > > > preconnecting two to the same domain should only connect a total > > of > > > > two > > > > > > > > > sockets...Though that could go *very* badly if the socket pool > is > > at > > > > its > > > > > > > > > connection cap, I suppose. Anyhow, just thought I'd mention it > - > > > > > haven't > > > > > > > > really > > > > > > > > > thought it through. > > > > > > > > > > > > > > > > > > Have you given any thought to how to write tests for this? We > > > really > > > > > > should > > > > > > > > > have some chrome/ browser tests for the feature, to make sure > the > > > > > > > preconnects > > > > > > > > > make it to the browser process. > > > > > > > > > > > > > > > > The existing predictor browser tests just wait until it sees the > DNS > > > > > lookup > > > > > > > for > > > > > > > > the request. At a minimum, we should do the same for preconnects, > > to > > > > make > > > > > > > sure > > > > > > > > we're at least doing something. I'd really like to do more, if we > > can > > > > > > manage > > > > > > > it > > > > > > > > without adding new hooks into net/. > > > > > > > > > > > > > > Simplest solution may be to start a second embedded test server, > > > > preconnect > > > > > to > > > > > > > it by IP address (Insert JS with the new address or use the spawned > > test > > > > > > server, > > > > > > > which supports substituting one string with another), and watch > NetLog > > > > > > > (Accessible on the UI thread by g_browser->net_log()) for an event > > with > > > > > source > > > > > > > type NetLog::SOURCE_SOCKET, event type NetLog::TYPE_TCP_CONNECT, and > a > > > > > > > source_address parameter matching the ip:port of the test server. > > > > > > > > > > > > > > I'm open to other ideas, but this seems like just the sort of > feature > > > that > > > > > > could > > > > > > > be broken for years with no one noticing it. Should have brought > this > > > up > > > > on > > > > > > > your earlier CL. > > > > > > > > > > > > Thanks, I'll take a look. I had looked earlier but as you noticed the > > > > existing > > > > > > predictor tests only tested DNS and not connections and I was > concerned > > > > about > > > > > > making connections to actual servers. I haven't looked at the embedded > > > test > > > > > > server logic (or for that matter a good chunk of the chrome > browser-side > > > > > > testing) so it may be a little while before I come back with an > update. > > > > > > > > > > We don't actually need two test servers - we could use a data URL or a > > > > > URLRequestMockHttpJob to have the page that triggers the preconnect not > > > create > > > > a > > > > > socket. > > > > > > > > btw, while I'm in here does it make sense to add mmenke@ as an owner for > the > > > > network_hints module? Right now it's just ttuttle@ and jar@. > > > > > > Since I've never worked on it, and tend to do a lot of reviews, I'd rather > not > > > expand what I own, unless it's needed, though, admittedly, this isn't > exactly > > a > > > high traffic directory. > > > > Any chance you know how to enable experimental blink features in > browser_tests? > > Looks like there are a few bugs open against it but I haven't seen anything > that > > indicates that it has been fixed and preconnect is currently behind an > > experimental feature flag so it's not running under the browser_tests harness. > > > I have the test harness and netlog hooks mostly in place but there's no way to > > actually trigger it right now. > > A quick search shows that there is a > "--enable-experimental-web-platform-features" switch. Whether it works, or > enables preconnecting, I couldn't say. Oh, and if the switch works, you'd need to add it to the CommandLine object in the SetUp method of the test (Not SetUpOnMainThread).
Oh, if browser_tests can use the same command-line options as Chrome then --enable-blink-features=LinkPreconnect will do it. Thanks for the pointer. On Thu, Feb 12, 2015 at 5:09 PM, <mmenke@chromium.org> wrote: > On 2015/02/12 22:08:55, mmenke wrote: > >> On 2015/02/12 21:46:24, Pat Meenan wrote: >> > On 2015/02/12 17:14:12, mmenke wrote: >> > > On 2015/02/12 17:09:09, Pat Meenan wrote: >> > > > On 2015/02/12 16:23:07, mmenke wrote: >> > > > > On 2015/02/12 16:02:21, Pat Meenan wrote: >> > > > > > On 2015/02/12 15:57:05, mmenke wrote: >> > > > > > > On 2015/02/12 15:47:27, mmenke wrote: >> > > > > > > > On 2015/02/12 15:42:38, mmenke wrote: >> > > > > > > > > One option would be to accumulate preconnect requests in >> the >> > browser >> > > > > > > process, >> > > > > > > > > and just preconnect cumulatively, since preconnecting one >> socket, >> > > and >> > > > > then >> > > > > > > > > preconnecting two to the same domain should only connect a >> > total > >> > of >> > > > two >> > > > > > > > > sockets...Though that could go *very* badly if the socket >> pool >> is >> > at >> > > > its >> > > > > > > > > connection cap, I suppose. Anyhow, just thought I'd >> mention >> > it > >> - >> > > > > haven't >> > > > > > > > really >> > > > > > > > > thought it through. >> > > > > > > > > >> > > > > > > > > Have you given any thought to how to write tests for >> this? We >> > > really >> > > > > > should >> > > > > > > > > have some chrome/ browser tests for the feature, to make >> sure >> the >> > > > > > > preconnects >> > > > > > > > > make it to the browser process. >> > > > > > > > >> > > > > > > > The existing predictor browser tests just wait until it >> sees the >> DNS >> > > > > lookup >> > > > > > > for >> > > > > > > > the request. At a minimum, we should do the same for >> > preconnects, > >> > to >> > > > make >> > > > > > > sure >> > > > > > > > we're at least doing something. I'd really like to do >> more, if >> > we > >> > can >> > > > > > manage >> > > > > > > it >> > > > > > > > without adding new hooks into net/. >> > > > > > > >> > > > > > > Simplest solution may be to start a second embedded test >> server, >> > > > preconnect >> > > > > to >> > > > > > > it by IP address (Insert JS with the new address or use the >> > spawned > >> > test >> > > > > > server, >> > > > > > > which supports substituting one string with another), and >> watch >> NetLog >> > > > > > > (Accessible on the UI thread by g_browser->net_log()) for an >> event >> > with >> > > > > source >> > > > > > > type NetLog::SOURCE_SOCKET, event type >> NetLog::TYPE_TCP_CONNECT, >> > and > >> a >> > > > > > > source_address parameter matching the ip:port of the test >> server. >> > > > > > > >> > > > > > > I'm open to other ideas, but this seems like just the sort of >> feature >> > > that >> > > > > > could >> > > > > > > be broken for years with no one noticing it. Should have >> brought >> this >> > > up >> > > > on >> > > > > > > your earlier CL. >> > > > > > >> > > > > > Thanks, I'll take a look. I had looked earlier but as you >> noticed >> > the > >> > > > existing >> > > > > > predictor tests only tested DNS and not connections and I was >> concerned >> > > > about >> > > > > > making connections to actual servers. I haven't looked at the >> > embedded > >> > > test >> > > > > > server logic (or for that matter a good chunk of the chrome >> browser-side >> > > > > > testing) so it may be a little while before I come back with an >> update. >> > > > > >> > > > > We don't actually need two test servers - we could use a data URL >> or a >> > > > > URLRequestMockHttpJob to have the page that triggers the >> preconnect >> > not > >> > > create >> > > > a >> > > > > socket. >> > > > >> > > > btw, while I'm in here does it make sense to add mmenke@ as an >> owner for >> the >> > > > network_hints module? Right now it's just ttuttle@ and jar@. >> > > >> > > Since I've never worked on it, and tend to do a lot of reviews, I'd >> rather >> not >> > > expand what I own, unless it's needed, though, admittedly, this isn't >> exactly >> > a >> > > high traffic directory. >> > >> > Any chance you know how to enable experimental blink features in >> browser_tests? >> > Looks like there are a few bugs open against it but I haven't seen >> anything >> that >> > indicates that it has been fixed and preconnect is currently behind an >> > experimental feature flag so it's not running under the browser_tests >> > harness. > > > I have the test harness and netlog hooks mostly in place but there's no >> way >> > to > >> > actually trigger it right now. >> > > A quick search shows that there is a >> "--enable-experimental-web-platform-features" switch. Whether it works, >> or >> enables preconnecting, I couldn't say. >> > > Oh, and if the switch works, you'd need to add it to the CommandLine > object in > the SetUp method of the test (Not SetUpOnMainThread). > > https://codereview.chromium.org/922533003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/12 22:11:20, chromium-reviews wrote: > Oh, if browser_tests can use the same command-line options as Chrome > then --enable-blink-features=LinkPreconnect will do it. Thanks for the > pointer. Yes, they can. They basically run all of Chrome, go through normal startup, etc. Once the first page is loaded, they enter the test body on the UI thread.
On 2015/02/12 22:13:06, mmenke wrote: > On 2015/02/12 22:11:20, chromium-reviews wrote: > > Oh, if browser_tests can use the same command-line options as Chrome > > then --enable-blink-features=LinkPreconnect will do it. Thanks for the > > pointer. > > Yes, they can. They basically run all of Chrome, go through normal startup, > etc. Once the first page is loaded, they enter the test body on the UI thread. ok, browser test has been added. I verified that it passes when preconnect is working and fails when it is not.
https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:220: net::CapturingNetLogObserver net_log; Calling a NetLogObserver net_log is a little confusing. I'd suggest net_log_observer - don't think the extra characters hurt us. I can also live with just observer. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:222: net::NetLog::LOG_ALL_BUT_BYTES); nit: Should indent 4, and move &net_log on to the next line. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:227: // server will be as a result of parsing and making the preconnect request. Claiming "The only netlog activity" is anything is a pretty strong claim. I suggest removing it. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:234: data_uri += encoded; nit: Suggest merging this line with the one where you declare data_url. std::string data_uri = "data:text/html;base64," + encoded; Think it's more readable that way https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:234: data_uri += encoded; optional: Do we have to base64 encode it? "data:text/html;<link rel...." seems to work fine, though I suppose it's not exactly pretty. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:236: ui_test_utils::NavigateToURL(browser(), GURL(data_uri)); For this to be non-racy, we're depending on the preconnect to make it to the network stack's HostResolver before we get news of the navigation completion on the UI thread. Two problems with this: I'd like to check for it making it to an actually connection attempt, and relying on everything up to that point that we do on the IO thread happening synchronously seems like a bad idea. To check for a connection attempt, we can just check that |entry. source.type| (Or something like that) is of type net::NetLog::SOURCE_SOCKET. To make sure there's no race, I suggest switching from a CapturingNetLogObserver to a custom one with a RunLoop that exits the loop when it sees the event we're waiting for. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:238: // Look through the recorded netlog events for the URL that we specified nit: netlog -> NetLog https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:241: g_browser_process->net_log()->RemoveThreadSafeObserver(&net_log); nit: This should probably go above this code block, just after the navigation. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:251: ASSERT_TRUE(saw_preconnect_request); nit: EXPECT_TRUE?
Addressed everything except for the custom observer. Working on that now (though if you have thoughts on how to fix my issue with exiting the run loop correctly I'd love to hear). https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:220: net::CapturingNetLogObserver net_log; On 2015/02/13 16:42:17, mmenke wrote: > Calling a NetLogObserver net_log is a little confusing. I'd suggest > net_log_observer - don't think the extra characters hurt us. I can also live > with just observer. Done. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:222: net::NetLog::LOG_ALL_BUT_BYTES); On 2015/02/13 16:42:16, mmenke wrote: > nit: Should indent 4, and move &net_log on to the next line. Done. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:227: // server will be as a result of parsing and making the preconnect request. On 2015/02/13 16:42:17, mmenke wrote: > Claiming "The only netlog activity" is anything is a pretty strong claim. I > suggest removing it. Done. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:234: data_uri += encoded; On 2015/02/13 16:42:17, mmenke wrote: > optional: Do we have to base64 encode it? "data:text/html;<link rel...." seems > to work fine, though I suppose it's not exactly pretty. We either need to base64 encode or URL encode it. The fact that it works as plain text seems to be luck (it shouldn't). Base64 encoding it was easy so I just went that way. Thanks for the pointer on being able to append the strings with a static string as a l-value. I didn't realize that was possible and cleans things up a lot. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:236: ui_test_utils::NavigateToURL(browser(), GURL(data_uri)); On 2015/02/13 16:42:16, mmenke wrote: > For this to be non-racy, we're depending on the preconnect to make it to the > network stack's HostResolver before we get news of the navigation completion on > the UI thread. > > Two problems with this: I'd like to check for it making it to an actually > connection attempt, and relying on everything up to that point that we do on the > IO thread happening synchronously seems like a bad idea. > > To check for a connection attempt, we can just check that |entry. source.type| > (Or something like that) is of type net::NetLog::SOURCE_SOCKET. > > To make sure there's no race, I suggest switching from a CapturingNetLogObserver > to a custom one with a RunLoop that exits the loop when it sees the event we're > waiting for. My original implementation had a custom observer with a run loop (like the DNS one) and a wait check. When I forced it to not fire until after entering the runloop I couldn't get the code to actually quit the runloop successfully (tried both base::MessageLoop::current()->Quit(); and base::MessageLoopForUI::current()->Quit();). I'll go back and give it another shot but may upload an intermediate implementation with the same issue if I can't figure it out. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:238: // Look through the recorded netlog events for the URL that we specified On 2015/02/13 16:42:17, mmenke wrote: > nit: netlog -> NetLog Done. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:241: g_browser_process->net_log()->RemoveThreadSafeObserver(&net_log); On 2015/02/13 16:42:17, mmenke wrote: > nit: This should probably go above this code block, just after the navigation. Done. https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:251: ASSERT_TRUE(saw_preconnect_request); On 2015/02/13 16:42:16, mmenke wrote: > nit: EXPECT_TRUE? Done.
https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:236: ui_test_utils::NavigateToURL(browser(), GURL(data_uri)); On 2015/02/13 20:45:26, Pat Meenan wrote: > On 2015/02/13 16:42:16, mmenke wrote: > > For this to be non-racy, we're depending on the preconnect to make it to the > > network stack's HostResolver before we get news of the navigation completion > on > > the UI thread. > > > > Two problems with this: I'd like to check for it making it to an actually > > connection attempt, and relying on everything up to that point that we do on > the > > IO thread happening synchronously seems like a bad idea. > > > > To check for a connection attempt, we can just check that |entry. source.type| > > (Or something like that) is of type net::NetLog::SOURCE_SOCKET. > > > > To make sure there's no race, I suggest switching from a > CapturingNetLogObserver > > to a custom one with a RunLoop that exits the loop when it sees the event > we're > > waiting for. > > My original implementation had a custom observer with a run loop (like the DNS > one) and a wait check. When I forced it to not fire until after entering the > runloop I couldn't get the code to actually quit the runloop successfully (tried > both base::MessageLoop::current()->Quit(); and > base::MessageLoopForUI::current()->Quit();). I'll go back and give it another > shot but may upload an intermediate implementation with the same issue if I > can't figure it out. I must be missing something...Why doesn't this work: class ObserverThing : public net::NetLog::ThreadSafeObserver { public: explicit ObserverThing(std::string host_port_pair) : host_port_pair_(host_port_pair) {} ~ObserverThing() {} void RunUntilThingWeCareAbout() { run_loop_.Run(); } private: OnNetLogEvent(...) { if (<event is the one we care about>) { run_loop_.Quit(); } } base::RunLoop run_loop_; const std::string host_port_pair_; } And then: ObserverThing thing(host_port_pair_we_care_about); g_browser_process->net_log()->AddThreadSafeObserver(thing); ui_test_utils::NavigateToURL(browser(), GURL(data_uri)); thing.RunUntilThingWeCareAbout(); g_browser_process->net_log()->RemoveThreadSafeObserver(thing);
Doh, I thought I had to pump the UI thread and was copying what the DNS observer was doing. That works great, thanks. Finishing up now and should have it ready in a few minutes. On Fri, Feb 13, 2015 at 3:55 PM, <mmenke@chromium.org> wrote: > > https://codereview.chromium.org/922533003/diff/20001/ > chrome/browser/net/predictor_browsertest.cc > File chrome/browser/net/predictor_browsertest.cc (right): > > https://codereview.chromium.org/922533003/diff/20001/ > chrome/browser/net/predictor_browsertest.cc#newcode236 > chrome/browser/net/predictor_browsertest.cc:236: > ui_test_utils::NavigateToURL(browser(), GURL(data_uri)); > On 2015/02/13 20:45:26, Pat Meenan wrote: > >> On 2015/02/13 16:42:16, mmenke wrote: >> > For this to be non-racy, we're depending on the preconnect to make >> > it to the > >> > network stack's HostResolver before we get news of the navigation >> > completion > >> on >> > the UI thread. >> > >> > Two problems with this: I'd like to check for it making it to an >> > actually > >> > connection attempt, and relying on everything up to that point that >> > we do on > >> the >> > IO thread happening synchronously seems like a bad idea. >> > >> > To check for a connection attempt, we can just check that |entry. >> > source.type| > >> > (Or something like that) is of type net::NetLog::SOURCE_SOCKET. >> > >> > To make sure there's no race, I suggest switching from a >> CapturingNetLogObserver >> > to a custom one with a RunLoop that exits the loop when it sees the >> > event > >> we're >> > waiting for. >> > > My original implementation had a custom observer with a run loop (like >> > the DNS > >> one) and a wait check. When I forced it to not fire until after >> > entering the > >> runloop I couldn't get the code to actually quit the runloop >> > successfully (tried > >> both base::MessageLoop::current()->Quit(); and >> base::MessageLoopForUI::current()->Quit();). I'll go back and give it >> > another > >> shot but may upload an intermediate implementation with the same issue >> > if I > >> can't figure it out. >> > > I must be missing something...Why doesn't this work: > > class ObserverThing : public net::NetLog::ThreadSafeObserver { > public: > explicit ObserverThing(std::string host_port_pair) > : host_port_pair_(host_port_pair) {} > ~ObserverThing() {} > > void RunUntilThingWeCareAbout() { > run_loop_.Run(); > } > > private: > OnNetLogEvent(...) { > if (<event is the one we care about>) { > run_loop_.Quit(); > } > } > > base::RunLoop run_loop_; > const std::string host_port_pair_; > } > > And then: > > ObserverThing thing(host_port_pair_we_care_about); > g_browser_process->net_log()->AddThreadSafeObserver(thing); > ui_test_utils::NavigateToURL(browser(), GURL(data_uri)); > thing.RunUntilThingWeCareAbout(); > g_browser_process->net_log()->RemoveThreadSafeObserver(thing); > > https://codereview.chromium.org/922533003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, the custom observer is in place now and I verified it fails as expected when the preconnect isn't enabled. The only thing it is sensitive to is if we change the format of the group names for the connection pools then this will have to be updated as well (doesn't seem likely but worth mentioning). I used the connection pool because the host/port pair was predictable and I wasn't positive what format the URL would be in from the test_server() in the case of IPv6 or remote android tests and matching that to the host field in the NetLog seemed a lot riskier.
Quick comments. https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:107: explicit ConnectNetLogObserver(const std::string host_port_pair) nit: const std::String& https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:108: : host_port_pair_(host_port_pair), nit: +2 indent https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:109: saw_connect_event_(false) { nit: +4 indent https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:111: this, net::NetLog::LOG_ALL_BUT_BYTES); Adding during the constructor and removing during the destructor is just a bad idea with the NetLog, due to threadsafety issues (The NetLog has some protection, but if you start watching here, it could call into you on another thread before your constructor completes, and down that path lies madness. Could call into the observer while in the middle of the destructor with the removal there as well) https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:120: if (!saw_connect_event_) This isn't needed. Run loop can be quit before it's started and it works as expected. Moreover, OnAddEntry is called on the IO thread, so this isn't threadsafe.
https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:107: explicit ConnectNetLogObserver(const std::string host_port_pair) On 2015/02/13 22:49:09, mmenke wrote: > nit: const std::String& Done. https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:108: : host_port_pair_(host_port_pair), On 2015/02/13 22:49:08, mmenke wrote: > nit: +2 indent Done. https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:109: saw_connect_event_(false) { On 2015/02/13 22:49:08, mmenke wrote: > nit: +4 indent Done. https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:111: this, net::NetLog::LOG_ALL_BUT_BYTES); On 2015/02/13 22:49:08, mmenke wrote: > Adding during the constructor and removing during the destructor is just a bad > idea with the NetLog, due to threadsafety issues (The NetLog has some > protection, but if you start watching here, it could call into you on another > thread before your constructor completes, and down that path lies madness. > Could call into the observer while in the middle of the destructor with the > removal there as well) Done. https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:120: if (!saw_connect_event_) On 2015/02/13 22:49:08, mmenke wrote: > This isn't needed. Run loop can be quit before it's started and it works as > expected. Moreover, OnAddEntry is called on the IO thread, so this isn't > threadsafe. Done.
https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predi... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:32: "--enable-experimental-web-platform-features"; Should get these strings from the header (public/common/content_switches.h), rather than duplicate them here. That way, if they're ever changed or remove, get a compile error instead of a test error. Looks like you'll still need to have "LinkPreconnect" explicitly in this file. https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:126: if (!saw_connect_event_) Per earlier comment, need to get rid of this. RunLoop handles quit before start correctly (Never starts the loop, I believe), and this isn't currently threadsafe.
https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predi... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:32: "--enable-experimental-web-platform-features"; On 2015/02/17 16:53:33, mmenke wrote: > Should get these strings from the header (public/common/content_switches.h), > rather than duplicate them here. That way, if they're ever changed or remove, > get a compile error instead of a test error. Looks like you'll still need to > have "LinkPreconnect" explicitly in this file. Done. https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predi... chrome/browser/net/predictor_browsertest.cc:126: if (!saw_connect_event_) On 2015/02/17 16:53:33, mmenke wrote: > Per earlier comment, need to get rid of this. RunLoop handles quit before start > correctly (Never starts the loop, I believe), and this isn't currently > threadsafe. Done.
LGTM! Thanks for adding the test!
ttuttle@ if you get a chance could you do an OWNERS review?
On 2015/02/17 18:47:39, Pat Meenan wrote: > ttuttle@ if you get a chance could you do an OWNERS review? Looking at this now, sorry.
On 2015/02/24 16:43:37, ttuttle wrote: > On 2015/02/17 18:47:39, Pat Meenan wrote: > > ttuttle@ if you get a chance could you do an OWNERS review? > > Looking at this now, sorry. lgtm!
The CQ bit was checked by pmeenan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/922533003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/16b951d63b306f03ca2825f58489bf6ea9bede8d Cr-Commit-Position: refs/heads/master@{#317839} |