HTMLPreloadScanner should only use valid <base> urls

HTMLPreloadScanner should only use valid <base> urls

Before this CL, HTMLPreloadScanner accepted invalid <base> urls and
used it to resolve urls encountered later in the scan.
This CL ensures that only valid urls specified in <base href> are
actually used as base urls.

BUG=471525
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193019

[kouhei (in TOK), 2015-03-31 10:08:32]

kouhei@chromium.org changed reviewers:
+ haraken@chromium.org, japhet@chromium.org

[Yoav Weiss, 2015-03-31 11:04:43]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-03-31 11:04:44]

https://codereview.chromium.org/1044133002/diff/1/LayoutTests/fast/parser/bad...
File LayoutTests/fast/parser/badurl-head-preloader-crash.html (right):

https://codereview.chromium.org/1044133002/diff/1/LayoutTests/fast/parser/bad...
LayoutTests/fast/parser/badurl-head-preloader-crash.html:2: <script
src=":"></script>
Can you add another test where we see that the invalid base URL is not taken
into account?

https://codereview.chromium.org/1044133002/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1044133002/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLPreloadScanner.cpp:522: }
What does the parser do in case of an invalid base href? Does it ignore it as
well?

[kouhei (in TOK), 2015-03-31 11:15:38]

Thanks for your comments! I'll address them tomorrow.

https://codereview.chromium.org/1044133002/diff/1/LayoutTests/fast/parser/bad...
File LayoutTests/fast/parser/badurl-head-preloader-crash.html (right):

https://codereview.chromium.org/1044133002/diff/1/LayoutTests/fast/parser/bad...
LayoutTests/fast/parser/badurl-head-preloader-crash.html:2: <script
src=":"></script>
On 2015/03/31 11:04:43, Yoav Weiss wrote:
> Can you add another test where we see that the invalid base URL is not taken
> into account?

Acknowledged.

https://codereview.chromium.org/1044133002/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1044133002/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLPreloadScanner.cpp:522: }
On 2015/03/31 11:04:43, Yoav Weiss wrote:
> What does the parser do in case of an invalid base href? Does it ignore it as
> well?
Good point. Let me verify.

[kouhei (in TOK), 2015-04-01 03:49:05]

PTAL

https://codereview.chromium.org/1044133002/diff/1/LayoutTests/fast/parser/bad...
File LayoutTests/fast/parser/badurl-head-preloader-crash.html (right):

https://codereview.chromium.org/1044133002/diff/1/LayoutTests/fast/parser/bad...
LayoutTests/fast/parser/badurl-head-preloader-crash.html:2: <script
src=":"></script>
On 2015/03/31 11:15:38, kouhei wrote:
> On 2015/03/31 11:04:43, Yoav Weiss wrote:
> > Can you add another test where we see that the invalid base URL is not taken
> > into account?
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1044133002/diff/1/Source/core/html/parser/HTM...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1044133002/diff/1/Source/core/html/parser/HTM...
Source/core/html/parser/HTMLPreloadScanner.cpp:522: }
On 2015/03/31 11:15:38, kouhei wrote:
> On 2015/03/31 11:04:43, Yoav Weiss wrote:
> > What does the parser do in case of an invalid base href? Does it ignore it
as
> > well?
> Good point. Let me verify.

It seems that the parser would set empty url as the base url when encountered
invalid base href.
I updated the code here to follow the behaviour.

[kouhei (in TOK), 2015-04-02 05:11:40]

yoav: Would you take a look?

[Yoav Weiss, 2015-04-02 05:29:09]

Sorry for the delay :/

I think the test can be improved (possibly by using some internals magic to see
which resource where downloaded/preloaded before the parser has a chance to
discover them)

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/badurl-base-preloader-crash.html (right):

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
LayoutTests/fast/parser/badurl-base-preloader-crash.html:1: <base
href="gopher:��&#279%�:0"></base>
Can you add a doctype?

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/badurl-base-should-be-ignored.html (right):

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
LayoutTests/fast/parser/badurl-base-should-be-ignored.html:1: <script
src="../../resources/js-test.js"></script>
Can you add a DOCTYPE?

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
LayoutTests/fast/parser/badurl-base-should-be-ignored.html:7: <script
src="resources/fail.js"></script>
Did that fail before your change?
AFAIU you're just testing if the JS was applied, not if the preloadScanner
picked it up and downloaded it.

[kouhei (in TOK), 2015-04-02 06:19:39]

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/badurl-base-preloader-crash.html (right):

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
LayoutTests/fast/parser/badurl-base-preloader-crash.html:1: <base
href="gopher:��&#279%�:0"></base>
On 2015/04/02 05:29:09, Yoav Weiss wrote:
> Can you add a doctype?

Done.

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
File LayoutTests/fast/parser/badurl-base-should-be-ignored.html (right):

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
LayoutTests/fast/parser/badurl-base-should-be-ignored.html:1: <script
src="../../resources/js-test.js"></script>
On 2015/04/02 05:29:09, Yoav Weiss wrote:
> Can you add a DOCTYPE?

Done.

https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
LayoutTests/fast/parser/badurl-base-should-be-ignored.html:7: <script
src="resources/fail.js"></script>
On 2015/04/02 05:29:09, Yoav Weiss wrote:
> Did that fail before your change?
No.

> AFAIU you're just testing if the JS was applied, not if the preloadScanner
> picked it up and downloaded it.
Yes. Currently we can't test the preloader reliably.

[Yoav Weiss, 2015-04-02 06:54:18]

On 2015/04/02 06:19:39, kouhei wrote:
>
https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
> File LayoutTests/fast/parser/badurl-base-preloader-crash.html (right):
> 
>
https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
> LayoutTests/fast/parser/badurl-base-preloader-crash.html:1: <base
> href="gopher:��&#279%�:0"></base>
> On 2015/04/02 05:29:09, Yoav Weiss wrote:
> > Can you add a doctype?
> 
> Done.
> 
>
https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
> File LayoutTests/fast/parser/badurl-base-should-be-ignored.html (right):
> 
>
https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
> LayoutTests/fast/parser/badurl-base-should-be-ignored.html:1: <script
> src="../../resources/js-test.js"></script>
> On 2015/04/02 05:29:09, Yoav Weiss wrote:
> > Can you add a DOCTYPE?
> 
> Done.
> 
>
https://codereview.chromium.org/1044133002/diff/20001/LayoutTests/fast/parser...
> LayoutTests/fast/parser/badurl-base-should-be-ignored.html:7: <script
> src="resources/fail.js"></script>
> On 2015/04/02 05:29:09, Yoav Weiss wrote:
> > Did that fail before your change?
> No.
> 
> > AFAIU you're just testing if the JS was applied, not if the preloadScanner
> > picked it up and downloaded it.
> Yes. Currently we can't test the preloader reliably.

I believe that we can :) You can take a look at
http/tests/loading/preload-{image,picture,video}* for examples

[kouhei (in TOK), 2015-04-02 06:56:29]

> > > AFAIU you're just testing if the JS was applied, not if the preloadScanner
> > > picked it up and downloaded it.
> > Yes. Currently we can't test the preloader reliably.
> 
> I believe that we can :) You can take a look at
> http/tests/loading/preload-{image,picture,video}* for examples
Thanks. I completely forgot about the server side testing.
Let me write a network-simulator.php test.

[kouhei (in TOK), 2015-04-02 07:45:26]

On 2015/04/02 06:56:29, kouhei wrote:
> > > > AFAIU you're just testing if the JS was applied, not if the
preloadScanner
> > > > picked it up and downloaded it.
> > > Yes. Currently we can't test the preloader reliably.
> > 
> > I believe that we can :) You can take a look at
> > http/tests/loading/preload-{image,picture,video}* for examples
> Thanks. I completely forgot about the server side testing.
> Let me write a network-simulator.php test.

Done.

[Yoav Weiss, 2015-04-02 08:10:27]

https://codereview.chromium.org/1044133002/diff/80001/LayoutTests/http/tests/...
File LayoutTests/http/tests/loading/preload-ignore-invalid-base.html (right):

https://codereview.chromium.org/1044133002/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/loading/preload-ignore-invalid-base.html:11:
shouldBeFalse("internals.isPreloaded('resources/fail.js');");
The script testing for isPreloaded should run inline (so blocking) above the tag
with the resource you're testing. Otherwise, the parser picks up that resource
before isPreloaded() is run, and unfortunately, isPreloaded can't deal with that
as it should...

[kouhei (in TOK), 2015-04-02 08:17:55]

On 2015/04/02 08:10:27, Yoav Weiss wrote:
>
https://codereview.chromium.org/1044133002/diff/80001/LayoutTests/http/tests/...
> File LayoutTests/http/tests/loading/preload-ignore-invalid-base.html (right):
> 
>
https://codereview.chromium.org/1044133002/diff/80001/LayoutTests/http/tests/...
> LayoutTests/http/tests/loading/preload-ignore-invalid-base.html:11:
> shouldBeFalse("internals.isPreloaded('resources/fail.js');");
> The script testing for isPreloaded should run inline (so blocking) above the
tag
> with the resource you're testing. Otherwise, the parser picks up that resource
> before isPreloaded() is run, and unfortunately, isPreloaded can't deal with
that
> as it should...

Moved the tag up. Would this work?

[Yoav Weiss, 2015-04-02 08:20:03]

On 2015/04/02 08:17:55, kouhei wrote:
> On 2015/04/02 08:10:27, Yoav Weiss wrote:
> >
>
https://codereview.chromium.org/1044133002/diff/80001/LayoutTests/http/tests/...
> > File LayoutTests/http/tests/loading/preload-ignore-invalid-base.html
(right):
> > 
> >
>
https://codereview.chromium.org/1044133002/diff/80001/LayoutTests/http/tests/...
> > LayoutTests/http/tests/loading/preload-ignore-invalid-base.html:11:
> > shouldBeFalse("internals.isPreloaded('resources/fail.js');");
> > The script testing for isPreloaded should run inline (so blocking) above the
> tag
> > with the resource you're testing. Otherwise, the parser picks up that
resource
> > before isPreloaded() is run, and unfortunately, isPreloaded can't deal with
> that
> > as it should...
> 
> Moved the tag up. Would this work?

Yeah, that'd work :)
 
LGTM

[kouhei (in TOK), 2015-04-02 08:29:04]

On 2015/04/02 08:20:03, Yoav Weiss wrote:
> On 2015/04/02 08:17:55, kouhei wrote:
> > On 2015/04/02 08:10:27, Yoav Weiss wrote:
> > >
> >
>
https://codereview.chromium.org/1044133002/diff/80001/LayoutTests/http/tests/...
> > > File LayoutTests/http/tests/loading/preload-ignore-invalid-base.html
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1044133002/diff/80001/LayoutTests/http/tests/...
> > > LayoutTests/http/tests/loading/preload-ignore-invalid-base.html:11:
> > > shouldBeFalse("internals.isPreloaded('resources/fail.js');");
> > > The script testing for isPreloaded should run inline (so blocking) above
the
> > tag
> > > with the resource you're testing. Otherwise, the parser picks up that
> resource
> > > before isPreloaded() is run, and unfortunately, isPreloaded can't deal
with
> > that
> > > as it should...
> > 
> > Moved the tag up. Would this work?
> 
> Yeah, that'd work :)
>  
> LGTM

Thanks!

[kouhei (in TOK), 2015-04-02 08:29:06]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2015-04-02 08:29:11]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-04-02 08:29:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1044133002/100001

[commit-bot: I haz the power, 2015-04-02 09:49:04]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193019