Add unit and layout test to <link rel=preconnect>

Add unit and layout test to <link rel=preconnect>

This CL adds unit tests for preconnect and dns-prefetch, and makes sure that HTTPS hosts are preconnected as such, even for schemeless URLs. It also adds Layout tests to that effect.

BUG=

Committed: https://crrev.com/a0e443f306acf0fa5f048fecb0d4518d1a1d806a
git-svn-id: svn://svn.chromium.org/blink/trunk@199505 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[Yoav Weiss, 2015-07-23 08:22:01]

yoav@yoav.ws changed reviewers:
+ mkwst@chromium.org

[Yoav Weiss, 2015-07-23 08:24:23]

Hey Mike! 
I bumped into this bug where <link rel=preconnect> doesn't work for href values
that have no scheme.
When looking at fixing that, I realized that current testing is lacking.

So, I added an interface for NetworkHints that enable us to actually unit test
these kind of things. Let me know what you think

[Mike West, 2015-07-23 09:52:23]

https://codereview.chromium.org/1245223008/diff/20001/Source/core/loader/Link...
File Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1245223008/diff/20001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:136: if (relAttribute.isPreconnect() &&
href.isValid() && (href.protocolIsInHTTPFamily() || href.protocolIs(""))) {
Does the spec say anything about this? I'd expect us to resolve the schemeless
URL against the page's URL before preconnecting rather than pushing through a
null scheme.

https://codereview.chromium.org/1245223008/diff/20001/Source/core/loader/Link...
File Source/core/loader/LinkLoader.h (right):

https://codereview.chromium.org/1245223008/diff/20001/Source/core/loader/Link...
Source/core/loader/LinkLoader.h:55: 
Nit: No newline.

[Yoav Weiss, 2015-07-23 10:21:41]

On 2015/07/23 09:52:23, Mike West wrote:
>
https://codereview.chromium.org/1245223008/diff/20001/Source/core/loader/Link...
> File Source/core/loader/LinkLoader.cpp (right):
> 
>
https://codereview.chromium.org/1245223008/diff/20001/Source/core/loader/Link...
> Source/core/loader/LinkLoader.cpp:136: if (relAttribute.isPreconnect() &&
> href.isValid() && (href.protocolIsInHTTPFamily() || href.protocolIs(""))) {
> Does the spec say anything about this? I'd expect us to resolve the schemeless
> URL against the page's URL before preconnecting rather than pushing through a
> null scheme.
> 

We do. And apparently I need moar coffee...
This "fix" and another one below it are not really needed, and fixed bugs that I
introduced in an earlier local commit and then fixed...

So, it appears everything was working fine and this CL is now not really fixing
anything other than adding more tests. I'll change its title accordingly.

[Yoav Weiss, 2015-07-23 10:43:39]

On 2015/07/23 10:21:41, Yoav Weiss wrote:
> On 2015/07/23 09:52:23, Mike West wrote:
> >
>
https://codereview.chromium.org/1245223008/diff/20001/Source/core/loader/Link...
> > File Source/core/loader/LinkLoader.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1245223008/diff/20001/Source/core/loader/Link...
> > Source/core/loader/LinkLoader.cpp:136: if (relAttribute.isPreconnect() &&
> > href.isValid() && (href.protocolIsInHTTPFamily() || href.protocolIs(""))) {
> > Does the spec say anything about this? I'd expect us to resolve the
schemeless
> > URL against the page's URL before preconnecting rather than pushing through
a
> > null scheme.
> > 
> 
> We do. And apparently I need moar coffee...
> This "fix" and another one below it are not really needed, and fixed bugs that
I
> introduced in an earlier local commit and then fixed...
> 
> So, it appears everything was working fine and this CL is now not really
fixing
> anything other than adding more tests. I'll change its title accordingly.

There's something I don't get here, because this *is* fixing the bug on a local
test case I have (and the bug *was* there. I wasn't dreaming).
Only, I don't get why it's fixing it now. I'll get more coffee and get to the
bottom of this.

[Mike West, 2015-07-23 11:12:59]

On 2015/07/23 at 10:43:39, yoav wrote:
> There's something I don't get here, because this *is* fixing the bug on a
local test case I have (and the bug *was* there. I wasn't dreaming).
> Only, I don't get why it's fixing it now. I'll get more coffee and get to the
bottom of this.

Ok. What is it fixing? The tests look totally reasonable, but it's not clear to
me why this is a solution, and what it's solving. :)

[Yoav Weiss, 2015-07-23 12:09:35]

On 2015/07/23 11:12:59, Mike West wrote:
> On 2015/07/23 at 10:43:39, yoav wrote:
> > There's something I don't get here, because this *is* fixing the bug on a
> local test case I have (and the bug *was* there. I wasn't dreaming).
> > Only, I don't get why it's fixing it now. I'll get more coffee and get to
the
> bottom of this.
> 
> Ok. What is it fixing? The tests look totally reasonable, but it's not clear
to
> me why this is a solution, and what it's solving. :)

That's not clear to me either :)

What I see locally is that preconnect doesn't work for schemeless URLs on the
master branch, but it does work on the local branch with that CL.
I suspect some race condition (maybe with the preloadScanner). In any case, I
want to get a good understanding of what's going on before we land this.
I'll report back once I know more.

[Yoav Weiss, 2015-07-24 10:04:12]

On 2015/07/23 12:09:35, Yoav Weiss wrote:
> On 2015/07/23 11:12:59, Mike West wrote:
> > On 2015/07/23 at 10:43:39, yoav wrote:
> > > There's something I don't get here, because this *is* fixing the bug on a
> > local test case I have (and the bug *was* there. I wasn't dreaming).
> > > Only, I don't get why it's fixing it now. I'll get more coffee and get to
> the
> > bottom of this.
> > 
> > Ok. What is it fixing? The tests look totally reasonable, but it's not clear
> to
> > me why this is a solution, and what it's solving. :)
> 
> That's not clear to me either :)
> 
> What I see locally is that preconnect doesn't work for schemeless URLs on the
> master branch, but it does work on the local branch with that CL.
> I suspect some race condition (maybe with the preloadScanner). In any case, I
> want to get a good understanding of what's going on before we land this.
> I'll report back once I know more.

OK, I no longer see the issue locally, with and without the patch.

I suspect this may be some race condition related to the preloadScanner, but I
no longer see it :/

So, I think the best way to go would be to add these tests, and keep digging
around the preloadScanner code in parallel.

[Mike West, 2015-07-24 10:18:35]

:)

Keeping the tests sounds good. Can you show me that patch?

[Yoav Weiss, 2015-07-24 12:26:38]

On 2015/07/24 10:18:35, Mike West wrote:
> :)
> 
> Keeping the tests sounds good. Can you show me that patch?

I can no longer recreate the issue with or without this CL, so I have no patch
of an actual fix.
I also have a CL with unit tests for the HTMLResourcePreloader, which all pass.

I think we can just land these tests, and see if the actual issue that I saw
will re-surface in the future.

[Mike West, 2015-07-24 13:10:26]

On 2015/07/24 at 12:26:38, yoav wrote:
> On 2015/07/24 10:18:35, Mike West wrote:
> > :)
> > 
> > Keeping the tests sounds good. Can you show me that patch?
> 
> I can no longer recreate the issue with or without this CL, so I have no patch
of an actual fix.
> I also have a CL with unit tests for the HTMLResourcePreloader, which all
pass.
> 
> I think we can just land these tests, and see if the actual issue that I saw
will re-surface in the future.

Ok. Are you planning on updating this CL to remove the code changes?

[Yoav Weiss, 2015-07-24 13:12:32]

On 2015/07/24 13:10:26, Mike West wrote:
> On 2015/07/24 at 12:26:38, yoav wrote:
> > On 2015/07/24 10:18:35, Mike West wrote:
> > > :)
> > > 
> > > Keeping the tests sounds good. Can you show me that patch?
> > 
> > I can no longer recreate the issue with or without this CL, so I have no
patch
> of an actual fix.
> > I also have a CL with unit tests for the HTMLResourcePreloader, which all
> pass.
> > 
> > I think we can just land these tests, and see if the actual issue that I saw
> will re-surface in the future.
> 
> Ok. Are you planning on updating this CL to remove the code changes?

I already did :)

Currently all the code changes are just adding an interface around NetworkHints,
making them testable with a unit test. They don't change any functionality.

[Mike West, 2015-07-27 09:10:18]

I see. LGTM!

[Yoav Weiss, 2015-07-27 09:21:07]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2015-07-27 09:21:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1245223008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1245223008/40001

[commit-bot: I haz the power, 2015-07-27 09:23:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-27 09:23:14]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[Yoav Weiss, 2015-07-27 10:07:22]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-07-27 10:07:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1245223008/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-27 10:07:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1245223008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1245223008/60001

[commit-bot: I haz the power, 2015-07-27 10:47:08]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199505

[commit-bot: I haz the power, 2015-09-23 11:48:23]

Patchset 4 (id:??) landed as
https://crrev.com/a0e443f306acf0fa5f048fecb0d4518d1a1d806a