Fix double resource request for script resources with integrity attr.

Fix double resource request for script resources with integrity attr.

Script tags with an integrity attribute were making two network requests
for the same resource for each tag. This would only happen if the script
tag appeared in the HTML of the page, but not if it was added
programmatically. This was because the preloader was requesting the
resource, but not checking the integrity attribute, and when the actual
request came around, it had to re-request the resource because of the
integrity metadata (see
https://chromium.googlesource.com/chromium/src/+/175dcb80d2d369bd7ce55b46d72971395c21a932
for a description of why this re-request for scripts is needed if there
is no integrity metadata on the first request).

Thus the solution to this is to check the integrity metadata in the
preloader. This adds a check in the preloader where, if the resource is
a script, it checks the integrity attribute and adds the appropriate
integrity metadata. When the script is requested for real later, the
returned resource now has the integrity metadata and no re-request is
needed.

BUG=573269
Committed: https://crrev.com/271be402092404bd04b2881ec7301a8fe0c58964
Cr-Commit-Position: refs/heads/master@{#367888}

[jww, 2016-01-06 01:20:57]

jww@chromium.org changed reviewers:
+ sigbjorn@opera.com

[jww, 2016-01-06 01:20:58]

sigbjorn@, would you mind taking a look at this? Thanks!

[tyoshino (SeeGerritForStatus), 2016-01-06 04:53:14]

On 2016/01/06 01:20:58, jww wrote:
> sigbjorn@, would you mind taking a look at this? Thanks!

Maybe not sigbjorn but sigbjornf (sof).

[tyoshino (SeeGerritForStatus), 2016-01-06 04:54:56]

Description was changed from

==========
Fix double resource request for script resources with integrity attr.

Script tags with an integrity attribute were making two network requests
for the same resource for each tag. This would only happen if the script
tag appeared in the HTML of the page, but not if it was added
programmatically. This was because the preloader was requesting the
resource, but not checking the integrity attribute, and when the actual
request came around, it had to re-request the resource because of the
integrity metadata (see
https://chromium.googlesource.com/chromium/src/+/175dcb80d2d369bd7ce55b46d729...
for a description of why this re-request for scripts is needed if there
is no integrity metadata on the first request).

Thus the solution to this is to check the integrity metadata in the
preloader. This adds a check in the preloader where, if the resource is
a script, it checks the integrity attribute and adds the appropriate
integrity metadata. When the script is requested for real later, the
returned resource now has the integrity metadata and no re-request is
needed.

BUG=573269
==========

to

==========
Fix double resource request for script resources with integrity attr.

Script tags with an integrity attribute were making two network requests
for the same resource for each tag. This would only happen if the script
tag appeared in the HTML of the page, but not if it was added
programmatically. This was because the preloader was requesting the
resource, but not checking the integrity attribute, and when the actual
request came around, it had to re-request the resource because of the
integrity metadata (see
https://chromium.googlesource.com/chromium/src/+/175dcb80d2d369bd7ce55b46d729...
for a description of why this re-request for scripts is needed if there
is no integrity metadata on the first request).

Thus the solution to this is to check the integrity metadata in the
preloader. This adds a check in the preloader where, if the resource is
a script, it checks the integrity attribute and adds the appropriate
integrity metadata. When the script is requested for real later, the
returned resource now has the integrity metadata and no re-request is
needed.

BUG=573269
==========

[tyoshino (SeeGerritForStatus), 2016-01-06 04:54:56]

tyoshino@chromium.org changed reviewers:
+ sigbjornf@opera.com

[tyoshino (SeeGerritForStatus), 2016-01-06 04:55:03]

On 2016/01/06 04:53:14, tyoshino wrote:
> On 2016/01/06 01:20:58, jww wrote:
> > sigbjorn@, would you mind taking a look at this? Thanks!
> 
> Maybe not sigbjorn but sigbjornf (sof).

Added sof

[sof, 2016-01-06 07:38:12]

sigbjornf@opera.com changed reviewers:
- sigbjorn@opera.com

[sof, 2016-01-06 07:38:13]

lgtm, quickly diagnosed and addressed.

(It's not the first time the preloader has given us late-breaking surprises, so
worth keeping it in mind for script & loading related changes.)

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html
(right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html:15:
function findRequestsByURL(url)
not for here, but a candidate addition to network-test.js (?)

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:304: IntegrityMetadataSet
metadataSet;
Looks like you can push this local variable into the "if" below.

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:385: 
accidental edit?

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/PreloadRequest.h:59:
IntegrityMetadataSet integrityMetadata() const
Would (const IntegrityMetadataSet&) work as return type?

[Yoav Weiss, 2016-01-06 07:48:28]

LGTM as well.

Is this a problem only with scripts? Is SRI enabled for other resource types?
(e.g. stylesheets)

[Yoav Weiss, 2016-01-06 07:48:43]

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

[Yoav Weiss, 2016-01-06 07:48:43]

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html
(right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html:5:
<script src="../network-test.js"></script>
Interesting! Didn't know about network-test. When testing for double downloads,
I used ResourceTiming which may be slightly less reliable, but has the advantage
of being portable (and can be written as a WebPlatformTest.

[jww, 2016-01-06 18:37:41]

On 2016/01/06 07:48:28, Yoav Weiss wrote:
> LGTM as well.
> 
> Is this a problem only with scripts? Is SRI enabled for other resource types?
> (e.g. stylesheets)

Yes, this is only a problem for scripts. While stylesheet resources also have
SRI enabled, they do not suffer from this re-request issue because they store
the raw resource around always. I've added a comment in HTMLPreloadScanner.cpp
to hopefully explain this.

[jww, 2016-01-06 18:37:53]

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html
(right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html:5:
<script src="../network-test.js"></script>
On 2016/01/06 07:48:43, Yoav Weiss wrote:
> Interesting! Didn't know about network-test. When testing for double
downloads,
> I used ResourceTiming which may be slightly less reliable, but has the
advantage
> of being portable (and can be written as a WebPlatformTest.

This test has the (slight) advantage of being an actual reproduction of how the
bug was discovered, so I'd like to keep this as the regression test. Let me know
if you feel strongly about having an additional, separate ResourceTiming test.

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html:15:
function findRequestsByURL(url)
On 2016/01/06 07:38:13, sof wrote:
> not for here, but a candidate addition to network-test.js (?)

Makes sense to me. I'll move it to network-test.js.

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:304: IntegrityMetadataSet
metadataSet;
On 2016/01/06 07:38:13, sof wrote:
> Looks like you can push this local variable into the "if" below.

Done.

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:385: 
On 2016/01/06 07:38:13, sof wrote:
> accidental edit?

Done.

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (right):

https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/PreloadRequest.h:59:
IntegrityMetadataSet integrityMetadata() const
On 2016/01/06 07:38:13, sof wrote:
> Would (const IntegrityMetadataSet&) work as return type?

Done.

[jww, 2016-01-06 18:37:57]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2016-01-06 18:38:46]

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

[commit-bot: I haz the power, 2016-01-06 20:16:28]

Description was changed from

==========
Fix double resource request for script resources with integrity attr.

Script tags with an integrity attribute were making two network requests
for the same resource for each tag. This would only happen if the script
tag appeared in the HTML of the page, but not if it was added
programmatically. This was because the preloader was requesting the
resource, but not checking the integrity attribute, and when the actual
request came around, it had to re-request the resource because of the
integrity metadata (see
https://chromium.googlesource.com/chromium/src/+/175dcb80d2d369bd7ce55b46d729...
for a description of why this re-request for scripts is needed if there
is no integrity metadata on the first request).

Thus the solution to this is to check the integrity metadata in the
preloader. This adds a check in the preloader where, if the resource is
a script, it checks the integrity attribute and adds the appropriate
integrity metadata. When the script is requested for real later, the
returned resource now has the integrity metadata and no re-request is
needed.

BUG=573269
==========

to

==========
Fix double resource request for script resources with integrity attr.

Script tags with an integrity attribute were making two network requests
for the same resource for each tag. This would only happen if the script
tag appeared in the HTML of the page, but not if it was added
programmatically. This was because the preloader was requesting the
resource, but not checking the integrity attribute, and when the actual
request came around, it had to re-request the resource because of the
integrity metadata (see
https://chromium.googlesource.com/chromium/src/+/175dcb80d2d369bd7ce55b46d729...
for a description of why this re-request for scripts is needed if there
is no integrity metadata on the first request).

Thus the solution to this is to check the integrity metadata in the
preloader. This adds a check in the preloader where, if the resource is
a script, it checks the integrity attribute and adds the appropriate
integrity metadata. When the script is requested for real later, the
returned resource now has the integrity metadata and no re-request is
needed.

BUG=573269
==========

[commit-bot: I haz the power, 2016-01-06 20:16:30]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-01-06 20:17:43]

Description was changed from

==========
Fix double resource request for script resources with integrity attr.

Script tags with an integrity attribute were making two network requests
for the same resource for each tag. This would only happen if the script
tag appeared in the HTML of the page, but not if it was added
programmatically. This was because the preloader was requesting the
resource, but not checking the integrity attribute, and when the actual
request came around, it had to re-request the resource because of the
integrity metadata (see
https://chromium.googlesource.com/chromium/src/+/175dcb80d2d369bd7ce55b46d729...
for a description of why this re-request for scripts is needed if there
is no integrity metadata on the first request).

Thus the solution to this is to check the integrity metadata in the
preloader. This adds a check in the preloader where, if the resource is
a script, it checks the integrity attribute and adds the appropriate
integrity metadata. When the script is requested for real later, the
returned resource now has the integrity metadata and no re-request is
needed.

BUG=573269
==========

to

==========
Fix double resource request for script resources with integrity attr.

Script tags with an integrity attribute were making two network requests
for the same resource for each tag. This would only happen if the script
tag appeared in the HTML of the page, but not if it was added
programmatically. This was because the preloader was requesting the
resource, but not checking the integrity attribute, and when the actual
request came around, it had to re-request the resource because of the
integrity metadata (see
https://chromium.googlesource.com/chromium/src/+/175dcb80d2d369bd7ce55b46d729...
for a description of why this re-request for scripts is needed if there
is no integrity metadata on the first request).

Thus the solution to this is to check the integrity metadata in the
preloader. This adds a check in the preloader where, if the resource is
a script, it checks the integrity attribute and adds the appropriate
integrity metadata. When the script is requested for real later, the
returned resource now has the integrity metadata and no re-request is
needed.

BUG=573269

Committed: https://crrev.com/271be402092404bd04b2881ec7301a8fe0c58964
Cr-Commit-Position: refs/heads/master@{#367888}
==========

[commit-bot: I haz the power, 2016-01-06 20:17:44]

Patchset 1 (id:??) landed as
https://crrev.com/271be402092404bd04b2881ec7301a8fe0c58964
Cr-Commit-Position: refs/heads/master@{#367888}

[Yoav Weiss, 2016-01-06 20:22:10]

On 2016/01/06 18:37:41, jww wrote:
> On 2016/01/06 07:48:28, Yoav Weiss wrote:
> > LGTM as well.
> > 
> > Is this a problem only with scripts? Is SRI enabled for other resource
types?
> > (e.g. stylesheets)
> 
> Yes, this is only a problem for scripts. While stylesheet resources also have
> SRI enabled, they do not suffer from this re-request issue because they store
> the raw resource around always. I've added a comment in HTMLPreloadScanner.cpp
> to hopefully explain this.

Maybe also add a comment to the parts in stylesheet that make sure that it
doesn't double download.
The scenario that scares me in that respect is that someone will modify the code
there and will cause double downloads, which take us time to figure out...

[Yoav Weiss, 2016-01-06 20:25:23]

On 2016/01/06 18:37:53, jww wrote:
>
https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html
> (right):
> 
>
https://codereview.chromium.org/1561903002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/http/tests/inspector/network/subresource-integrity-number-of-requests.html:5:
> <script src="../network-test.js"></script>
> On 2016/01/06 07:48:43, Yoav Weiss wrote:
> > Interesting! Didn't know about network-test. When testing for double
> downloads,
> > I used ResourceTiming which may be slightly less reliable, but has the
> advantage
> > of being portable (and can be written as a WebPlatformTest.
> 
> This test has the (slight) advantage of being an actual reproduction of how
the
> bug was discovered, so I'd like to keep this as the regression test. Let me
know
> if you feel strongly about having an additional, separate ResourceTiming test.
> 

I don't feel strongly about adding another test right now. Might make sense to
add one as part of a WPT test suite is there is one.

[Yoav Weiss, 2016-01-06 20:26:34]

On 2016/01/06 20:22:10, Yoav Weiss wrote:
> On 2016/01/06 18:37:41, jww wrote:
> > On 2016/01/06 07:48:28, Yoav Weiss wrote:
> > > LGTM as well.
> > > 
> > > Is this a problem only with scripts? Is SRI enabled for other resource
> types?
> > > (e.g. stylesheets)
> > 
> > Yes, this is only a problem for scripts. While stylesheet resources also
have
> > SRI enabled, they do not suffer from this re-request issue because they
store
> > the raw resource around always. I've added a comment in
HTMLPreloadScanner.cpp
> > to hopefully explain this.
> 
> Maybe also add a comment to the parts in stylesheet that make sure that it
> doesn't double download.
> The scenario that scares me in that respect is that someone will modify the
code
> there and will cause double downloads, which take us time to figure out...

Or even better yet, maybe add a similar test for stylesheets, to make sure they
don't start double downloading in the future.

[jww, 2016-01-06 21:10:19]

On 2016/01/06 20:26:34, Yoav Weiss wrote:
> On 2016/01/06 20:22:10, Yoav Weiss wrote:
> > On 2016/01/06 18:37:41, jww wrote:
> > > On 2016/01/06 07:48:28, Yoav Weiss wrote:
> > > > LGTM as well.
> > > > 
> > > > Is this a problem only with scripts? Is SRI enabled for other resource
> > types?
> > > > (e.g. stylesheets)
> > > 
> > > Yes, this is only a problem for scripts. While stylesheet resources also
> have
> > > SRI enabled, they do not suffer from this re-request issue because they
> store
> > > the raw resource around always. I've added a comment in
> HTMLPreloadScanner.cpp
> > > to hopefully explain this.
> > 
> > Maybe also add a comment to the parts in stylesheet that make sure that it
> > doesn't double download.
> > The scenario that scares me in that respect is that someone will modify the
> code
> > there and will cause double downloads, which take us time to figure out...
> 
> Or even better yet, maybe add a similar test for stylesheets, to make sure
they
> don't start double downloading in the future.

I messed up with this commit and failed to actually upload all of the changes I
said "done" to above :-P I'm opening a new CL () to commit those changes. To
respond to Yoav's latest comments:
- The default scenario is not to re-request. The only way to cause a double
request is to override Resource::mustRefetchDueToIntegrityMismatch(), so I'll
put a comment above that specifying that it should be left as-is for everything
other than scripts.
- I don't think we should add this test to the WPT suite. This is a very
implementation specific issue, I don't think it's something the Web platform
should be verifying (that is, this is only a quirk because of how we implement
Strings, and even more specifically, the fact that V8 needs us to keep the
decoded string around, and not the raw data).
- I'll add another test for stylesheets as well, as per your suggestion.

[jww, 2016-01-06 21:12:46]

On 2016/01/06 21:10:19, jww wrote:
> On 2016/01/06 20:26:34, Yoav Weiss wrote:
> > On 2016/01/06 20:22:10, Yoav Weiss wrote:
> > > On 2016/01/06 18:37:41, jww wrote:
> > > > On 2016/01/06 07:48:28, Yoav Weiss wrote:
> > > > > LGTM as well.
> > > > > 
> > > > > Is this a problem only with scripts? Is SRI enabled for other resource
> > > types?
> > > > > (e.g. stylesheets)
> > > > 
> > > > Yes, this is only a problem for scripts. While stylesheet resources also
> > have
> > > > SRI enabled, they do not suffer from this re-request issue because they
> > store
> > > > the raw resource around always. I've added a comment in
> > HTMLPreloadScanner.cpp
> > > > to hopefully explain this.
> > > 
> > > Maybe also add a comment to the parts in stylesheet that make sure that it
> > > doesn't double download.
> > > The scenario that scares me in that respect is that someone will modify
the
> > code
> > > there and will cause double downloads, which take us time to figure out...
> > 
> > Or even better yet, maybe add a similar test for stylesheets, to make sure
> they
> > don't start double downloading in the future.
> 
> I messed up with this commit and failed to actually upload all of the changes
I
> said "done" to above :-P I'm opening a new CL () to commit those changes. To
> respond to Yoav's latest comments:
> - The default scenario is not to re-request. The only way to cause a double
> request is to override Resource::mustRefetchDueToIntegrityMismatch(), so I'll
> put a comment above that specifying that it should be left as-is for
everything
> other than scripts.
> - I don't think we should add this test to the WPT suite. This is a very
> implementation specific issue, I don't think it's something the Web platform
> should be verifying (that is, this is only a quirk because of how we implement
> Strings, and even more specifically, the fact that V8 needs us to keep the
> decoded string around, and not the raw data).
> - I'll add another test for stylesheets as well, as per your suggestion.

And here's the other CL: https://codereview.chromium.org/1569623002