Initial Fetch integration for Subresource Integrity

Initial Fetch integration for Subresource Integrity

This adds support to the fetch() API for the integrity attribute, both
the setting of the attribute and the actual SRI enforcement.

For the regular integrity attribute on elements, this leaves in place
the checking of SRI in the Script element and Style element execution,
rather than integrating it more fully into the Resource Loader because
of technical complications. This may be worth doing in the future, but
will require some significant redesign of how Resource objects are
returned from the Memory Cache in ResourceFetcher::requestScript.

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

[jww, 2015-08-08 01:46:10]

horo@ and tyoshino@, this is my initial attempt at integrating the subresource
integrity spec into Fetch. Would you mind taking a look at the modules/fetch
changes and let me know if I'm on the right track? Notably, I know I still need
to make my IntegrityVerifiedDataConsumerHandle actually return data, but I
wanted to make sure I'm going down the right path before fixing that. Thanks!

[horo, 2015-08-10 08:15:11]

horo@chromium.org changed reviewers:
+ horo@chromium.org

[horo, 2015-08-10 08:15:12]

looks-good for me.
But yhirano@ knows more about Streams than I.
Added yhirano@ to CC.

[jww, 2015-08-10 13:32:41]

On 2015/08/10 08:15:12, horo wrote:
> looks-good for me.
> But yhirano@ knows more about Streams than I.
> Added yhirano@ to CC.

Ah, my mistake; I meant to CC yirano@ original. Thanks, Horo.

[yhirano, 2015-08-11 09:22:07]

yhirano@chromium.org changed reviewers:
+ yhirano@chromium.org

[yhirano, 2015-08-11 09:22:08]

WebDataConsumerHandle::obtainReader is called on any thread and the obtained
reader run on the thread. I think SRI operation can be divided into three parts:

A: reading data from the original handle (running on the reader thread(s))
B: verifying the data and reporting if necessary (running on the fetch thread)
C. dispatching read data or an error to the client (running on the reader
thread(s))

We need to post task B to the fetch thread when A is done. I talked with
hiroshige@ and he will provide a helper class[1] to do that. When verification
is done, you can "update" the handle with CompositeDataConsumerHandle.
Regarding C, you can use FetchFormDataConsumerHandle::create which will be
introduced shortly[2].

1: https://codereview.chromium.org/1281273004/
2: https://codereview.chromium.org/1265413002/

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/BodySt...
File Source/modules/fetch/BodyStreamBuffer.h (right):

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/BodySt...
Source/modules/fetch/BodyStreamBuffer.h:93: String m_integrity;
Is this needed? BodyStreamBuffer is the source object of the body stream. You
already have m_integrity in FetchRequestData and it should not be placed here.

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:93: while (m_reader->read(buf, 1000,
flags, &amt) == Ok)
Do you want to read all data from the pipe with this loop? If so, you cannot.

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:96: if
(!SubresourceIntegrity::CheckSubresourceIntegrity(m_integrityMetadata, content,
m_url, *m_loader->document(), errorMessage)) {
This function can be called on a different thread from the one in which the
loader and the document live, so I think you cannot use them synchronously.

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:127: Reader* obtainReaderInternal(Client*
client) override
A difficult point is that obtainReaderInternal can be called on any oilpan
thread and the obtained reader run on the thread.

[yhirano, 2015-08-11 15:02:36]

> WebDataConsumerHandle::obtainReader is called on any thread and the obtained
> reader run on the thread. I think SRI operation can be divided into three
parts:
> 
> A: reading data from the original handle (running on the reader thread(s))
> B: verifying the data and reporting if necessary (running on the fetch thread)
> C. dispatching read data or an error to the client (running on the reader
> thread(s))
> 
> We need to post task B to the fetch thread when A is done. I talked with
> hiroshige@ and he will provide a helper class[1] to do that. When verification
> is done, you can "update" the handle with CompositeDataConsumerHandle.
> Regarding C, you can use FetchFormDataConsumerHandle::create which will be
> introduced shortly[2].
> 
> 1: https://codereview.chromium.org/1281273004/
> 2: https://codereview.chromium.org/1265413002/
> 

Hmm, I noticed you can implement it a bit easily.

> A: reading data from the original handle (running on the reader thread(s))
> B: verifying the data and reporting if necessary (running on the fetch thread)
> C. dispatching read data or an error to the client (running on the reader
> thread(s))

You can implement A on the fetch thread and then there is no need to post a
task. you can verify the contents on the thread and update the handle after
that.
Specifically, you can implement SRIVerifier that inherits
WebDataConsumerHandle::Client.

class SRIVerifier : public WebDataConsumerHandle::Client {
public:
  // called on the fetch thread
  void didGetReadable() override {
     Result r = Ok;
     while(r == Ok) {
       r = m_reader->beginRead(...);
       if (r == ok) {
         copy the read data to the buffer.
         m_reader->endRead(..);
       }
     }
     if (r == ShouldWait) return;
     if (r == Done) {
        verify();
        if (verified successfully) {
          m_updater->update(FetchFormDataConsumerHandle::create(m_buffer));
        } else {
          m_updater->update(createUnexpectedErrorConsumerHandle());
     } else {
          m_updater->update(createUnexpectedErrorConsumerHandle());
     }
  }
  ...

private:
  Persistent<CompositeDataConsumerHandle::Updater> m_updater;
  ...
};

In FetchManager::Loader::didReceiveResponse, you can create a
CompositeDataConsumerHandle with createWaitingDataConsumerHandle, and pass it to
the client and pass the associated updater to the SRIVerifier.

One remaining point is life time management:
A solution is make FetchManager have SRIVerifier, though maybe you need to
modify notifyFinished() timing and it is hacky.
Or you can use CrossThreadHolder for the purpose, I think.

[jww, 2015-08-12 16:26:05]

On 2015/08/11 15:02:36, yhirano wrote:
> > WebDataConsumerHandle::obtainReader is called on any thread and the obtained
> > reader run on the thread. I think SRI operation can be divided into three
> parts:
> > 
> > A: reading data from the original handle (running on the reader thread(s))
> > B: verifying the data and reporting if necessary (running on the fetch
thread)
> > C. dispatching read data or an error to the client (running on the reader
> > thread(s))
> > 
> > We need to post task B to the fetch thread when A is done. I talked with
> > hiroshige@ and he will provide a helper class[1] to do that. When
verification
> > is done, you can "update" the handle with CompositeDataConsumerHandle.
> > Regarding C, you can use FetchFormDataConsumerHandle::create which will be
> > introduced shortly[2].
> > 
> > 1: https://codereview.chromium.org/1281273004/
> > 2: https://codereview.chromium.org/1265413002/
> > 
> 
> Hmm, I noticed you can implement it a bit easily.
> 
> > A: reading data from the original handle (running on the reader thread(s))
> > B: verifying the data and reporting if necessary (running on the fetch
thread)
> > C. dispatching read data or an error to the client (running on the reader
> > thread(s))
> 
> You can implement A on the fetch thread and then there is no need to post a
> task. you can verify the contents on the thread and update the handle after
> that.
> Specifically, you can implement SRIVerifier that inherits
> WebDataConsumerHandle::Client.
> 
> class SRIVerifier : public WebDataConsumerHandle::Client {
> public:
>   // called on the fetch thread
>   void didGetReadable() override {
>      Result r = Ok;
>      while(r == Ok) {
>        r = m_reader->beginRead(...);
>        if (r == ok) {
>          copy the read data to the buffer.
>          m_reader->endRead(..);
>        }
>      }
>      if (r == ShouldWait) return;
>      if (r == Done) {
>         verify();
>         if (verified successfully) {
>           m_updater->update(FetchFormDataConsumerHandle::create(m_buffer));
>         } else {
>           m_updater->update(createUnexpectedErrorConsumerHandle());
>      } else {
>           m_updater->update(createUnexpectedErrorConsumerHandle());
>      }
>   }
>   ...
> 
> private:
>   Persistent<CompositeDataConsumerHandle::Updater> m_updater;
>   ...
> };
> 
> In FetchManager::Loader::didReceiveResponse, you can create a
> CompositeDataConsumerHandle with createWaitingDataConsumerHandle, and pass it
to
> the client and pass the associated updater to the SRIVerifier.
> 
> One remaining point is life time management:
> A solution is make FetchManager have SRIVerifier, though maybe you need to
> modify notifyFinished() timing and it is hacky.
> Or you can use CrossThreadHolder for the purpose, I think.

Thanks, but based on my understanding, I'm not sure this works. According to
the spec, an integrity verification failure should be a network error, which is
why
I'm trying to avoid calling m_resolve.resolve() until I know if the verification
has
passed (because, in my understanding, once m_resolve.resolve() has been called,
it's
no longer possible to have a network error). Instead of calling
m_updater->update(...),
why shouldn't I pass the loader's resolver handle into SRIVerifier, and call
resolver.resolve(...) with either the response, or call performNetworkError() in
the
case of a failure?

[jww, 2015-08-12 16:40:14]

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/BodySt...
File Source/modules/fetch/BodyStreamBuffer.h (right):

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/BodySt...
Source/modules/fetch/BodyStreamBuffer.h:93: String m_integrity;
On 2015/08/11 09:22:08, yhirano wrote:
> Is this needed? BodyStreamBuffer is the source object of the body stream. You
> already have m_integrity in FetchRequestData and it should not be placed here.

Removed.

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:93: while (m_reader->read(buf, 1000,
flags, &amt) == Ok)
On 2015/08/11 09:22:08, yhirano wrote:
> Do you want to read all data from the pipe with this loop? If so, you cannot.

This is addressed by your suggestion of implementing all of this in a Client in
didGetReadable(), correct?

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:96: if
(!SubresourceIntegrity::CheckSubresourceIntegrity(m_integrityMetadata, content,
m_url, *m_loader->document(), errorMessage)) {
On 2015/08/11 09:22:08, yhirano wrote:
> This function can be called on a different thread from the one in which the
> loader and the document live, so I think you cannot use them synchronously.

This is no longer a problem if your suggestion of implementing this in a Client
in didGetReadable(), correct? That is, that guarantees that the logic will live
in the Fetch thread, so using the loader and the document should be safe.

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:127: Reader* obtainReaderInternal(Client*
client) override
On 2015/08/11 09:22:08, yhirano wrote:
> A difficult point is that obtainReaderInternal can be called on any oilpan
> thread and the obtained reader run on the thread.

Acknowledged.

[jww, 2015-08-13 04:12:53]

On 2015/08/12 16:40:14, jww wrote:
>
https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/BodySt...
> File Source/modules/fetch/BodyStreamBuffer.h (right):
> 
>
https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/BodySt...
> Source/modules/fetch/BodyStreamBuffer.h:93: String m_integrity;
> On 2015/08/11 09:22:08, yhirano wrote:
> > Is this needed? BodyStreamBuffer is the source object of the body stream.
You
> > already have m_integrity in FetchRequestData and it should not be placed
here.
> 
> Removed.
> 
>
https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
> File Source/modules/fetch/FetchManager.cpp (right):
> 
>
https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
> Source/modules/fetch/FetchManager.cpp:93: while (m_reader->read(buf, 1000,
> flags, &amt) == Ok)
> On 2015/08/11 09:22:08, yhirano wrote:
> > Do you want to read all data from the pipe with this loop? If so, you
cannot.
> 
> This is addressed by your suggestion of implementing all of this in a Client
in
> didGetReadable(), correct?
> 
>
https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
> Source/modules/fetch/FetchManager.cpp:96: if
> (!SubresourceIntegrity::CheckSubresourceIntegrity(m_integrityMetadata,
content,
> m_url, *m_loader->document(), errorMessage)) {
> On 2015/08/11 09:22:08, yhirano wrote:
> > This function can be called on a different thread from the one in which the
> > loader and the document live, so I think you cannot use them synchronously.
> 
> This is no longer a problem if your suggestion of implementing this in a
Client
> in didGetReadable(), correct? That is, that guarantees that the logic will
live
> in the Fetch thread, so using the loader and the document should be safe.
> 
>
https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
> Source/modules/fetch/FetchManager.cpp:127: Reader*
obtainReaderInternal(Client*
> client) override
> On 2015/08/11 09:22:08, yhirano wrote:
> > A difficult point is that obtainReaderInternal can be called on any oilpan
> > thread and the obtained reader run on the thread.
> 
> Acknowledged.

As a further point of note, it appears that didFinishLoading() gets called
before didGetReadable() in the Client, so at that point there's no way to access
the loader and thus no way to force a network failure. Any suggestions on how we
can block the loader from reaching didFinishLoader() until the integrity check
fails?

[tyoshino (SeeGerritForStatus), 2015-08-13 06:53:37]

tyoshino@chromium.org changed reviewers:
+ tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-13 06:53:38]

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:161: // surrounding numbers should be
corrected.)
now this step is numbered 23. it seems keeping the numbers in sync with the spec
doesn't help readability so much. just drop it?

[jww, 2015-08-13 14:32:11]

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:161: // surrounding numbers should be
corrected.)
On 2015/08/13 06:53:38, tyoshino wrote:
> now this step is numbered 23. it seems keeping the numbers in sync with the
spec
> doesn't help readability so much. just drop it?

Sure, happy to. Should I remove the others at the same time?

[tyoshino (SeeGerritForStatus), 2015-08-13 15:02:31]

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:161: // surrounding numbers should be
corrected.)
On 2015/08/13 14:32:11, jww wrote:
> On 2015/08/13 06:53:38, tyoshino wrote:
> > now this step is numbered 23. it seems keeping the numbers in sync with the
> spec
> > doesn't help readability so much. just drop it?
> 
> Sure, happy to. Should I remove the others at the same time?

Leaving them unchanged, removing all, either is fine!

[yhirano, 2015-08-13 18:10:28]

On 2015/08/12 16:26:05, jww wrote:
> 
> Thanks, but based on my understanding, I'm not sure this works. According to
> the spec, an integrity verification failure should be a network error, which
is
> why
> I'm trying to avoid calling m_resolve.resolve() until I know if the
verification
> has
> passed (because, in my understanding, once m_resolve.resolve() has been
called,
> it's
> no longer possible to have a network error).
Ah, right. I didn't understand that.

> Instead of calling
> m_updater->update(...),
> why shouldn't I pass the loader's resolver handle into SRIVerifier, and call
> resolver.resolve(...) with either the response, or call performNetworkError()
in
> the
> case of a failure?
That seems fine.

> As a further point of note, it appears that didFinishLoading() gets called
> before didGetReadable() in the Client, so at that point there's no way to
access
> the loader and thus no way to force a network failure. Any suggestions on how
we
> can block the loader from reaching didFinishLoader() until the integrity check
> fails?
As long as you are reading from the WebDataConsumerHandle, you can ignore the
notification for ThreadableLoaderClient (after didReceiveResponse).
You can get Done / Error signal as a read result.
One problematic point is that sometimes didFinishLoading comes a bit earlier
than you read Done from the handle, and it deletes FetchManager through
notifyFinished. That should be addressed in this case.

[yhirano, 2015-08-13 18:10:36]

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/1/Source/modules/fetch/FetchM...
Source/modules/fetch/FetchManager.cpp:93: while (m_reader->read(buf, 1000,
flags, &amt) == Ok)
On 2015/08/12 16:40:14, jww wrote:
> On 2015/08/11 09:22:08, yhirano wrote:
> > Do you want to read all data from the pipe with this loop? If so, you
cannot.
> 
> This is addressed by your suggestion of implementing all of this in a Client
in
> didGetReadable(), correct?

Yes.

[jww, 2015-08-13 23:57:13]

I believe this implementation is basically working at this point, modulo the
TODO in FetchManager.cpp ("TODO waiting for commit of
FetchFormDataConsumerHandle"). As soon as
https://codereview.chromium.org/1265413002/ is committed I will update this.

yhirano@, can you verify that this looks good? In particular, make sure that my
Oil Pan implementation for SRIVerifier looks good; it's the first time I've
written a GC'd Blink object. Thanks!

[yhirano, 2015-08-17 11:53:45]

Thanks, I think it will work.

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:129: DEFINE_INLINE_TRACE() {
visitor->trace(m_updater); }
You need to list m_loader and m_response in the trace function.

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:133: OwnPtr<Response> m_response;
Member<Response>

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:134: FetchManager::Loader* m_loader;
RawPtrWillBeMember<...>

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:268: m_integrityVerifier = new
SRIVerifier(handle, updater, adoptPtr(r), this, m_request->integrity(),
response.url());
adoptPtr is not needed.

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:276: return;
I think loadSucceeded will not be be called when the verifier is done faster
than didFinishLoading is dispatched, right?

[yhirano, 2015-08-17 11:54:44]

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:276: return;
On 2015/08/17 11:53:45, yhirano wrote:
> I think loadSucceeded will not be be called when the verifier is done faster
> than didFinishLoading is dispatched, right?

s/faster/earlier/

[jww, 2015-08-17 15:40:59]

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:129: DEFINE_INLINE_TRACE() {
visitor->trace(m_updater); }
On 2015/08/17 11:53:45, yhirano wrote:
> You need to list m_loader and m_response in the trace function.

Done.

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:133: OwnPtr<Response> m_response;
On 2015/08/17 11:53:45, yhirano wrote:
> Member<Response>

Done.

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:134: FetchManager::Loader* m_loader;
On 2015/08/17 11:53:45, yhirano wrote:
> RawPtrWillBeMember<...>

Done, although out of curiosity, why should it be RawPtrWillBeMember rather than
Member?

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:268: m_integrityVerifier = new
SRIVerifier(handle, updater, adoptPtr(r), this, m_request->integrity(),
response.url());
On 2015/08/17 11:53:45, yhirano wrote:
> adoptPtr is not needed.

Done.

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:276: return;
On 2015/08/17 11:54:44, yhirano wrote:
> On 2015/08/17 11:53:45, yhirano wrote:
> > I think loadSucceeded will not be be called when the verifier is done faster
> > than didFinishLoading is dispatched, right?
> 
> s/faster/earlier/

Yes, good call. I've removed the "|| m_finished" check.

[yhirano, 2015-08-18 09:31:54]

FetchFormDataConsumerHandle CL was landed.

ServiceWorker interaction is not clear to me (though it may be out of scope).
When a request is trapped by a serviceworker, should |integrity| be transferred?
Should verification run twice?

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:134: FetchManager::Loader* m_loader;
On 2015/08/17 15:40:59, jww wrote:
> On 2015/08/17 11:53:45, yhirano wrote:
> > RawPtrWillBeMember<...>
> 
> Done, although out of curiosity, why should it be RawPtrWillBeMember rather
than
> Member?

Because FetchManager::loader inherits NoBaseWillBeGarbageCollectedFinalized.

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:276: return;
On 2015/08/17 15:40:59, jww wrote:
> On 2015/08/17 11:54:44, yhirano wrote:
> > On 2015/08/17 11:53:45, yhirano wrote:
> > > I think loadSucceeded will not be be called when the verifier is done
faster
> > > than didFinishLoading is dispatched, right?
> > 
> > s/faster/earlier/
> 
> Yes, good call. I've removed the "|| m_finished" check.

I think you need isFinished predicated here.

if (m_integrityVerifier && !m_integrityVerifier->isFinished())
    return;

[jww, 2015-08-18 19:26:20]

Your Service Worker question is a good one, but I think that it is orthogonal to
this CL.

My rough understanding is that SWs should be in control of the integrity
attribute, and should be able to set it and modify it at will, which I believe
could result in two verification checks if it fetches and then returns (since it
may have modified the integrity property). However, I want to ping Anne vK to
discuss this before implementing and checking it.

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/40001/Source/modules/fetch/Fe...
Source/modules/fetch/FetchManager.cpp:276: return;
On 2015/08/18 09:31:53, yhirano wrote:
> On 2015/08/17 15:40:59, jww wrote:
> > On 2015/08/17 11:54:44, yhirano wrote:
> > > On 2015/08/17 11:53:45, yhirano wrote:
> > > > I think loadSucceeded will not be be called when the verifier is done
> faster
> > > > than didFinishLoading is dispatched, right?
> > > 
> > > s/faster/earlier/
> > 
> > Yes, good call. I've removed the "|| m_finished" check.
> 
> I think you need isFinished predicated here.
> 
> if (m_integrityVerifier && !m_integrityVerifier->isFinished())
>     return;
> 

Done.

[yhirano, 2015-08-19 10:42:36]

Can you add integrity property settings tests (the simplest case and the no-cors
case) in /fetch/script-tests/request.js?

https://codereview.chromium.org/1279163005/diff/100001/LayoutTests/http/tests...
File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html
(right):

https://codereview.chromium.org/1279163005/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:27:
.then(test.step_func(function(response) {
You can use promise_test. Then you don't have to insert test.step_func,
assert_unreached, test.done, etc.

https://codereview.chromium.org/1279163005/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:30:
consume(response.body.getReader(), test, expectedValue, '');
You can use response.text().

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:169: Member<SRIVerifier>
m_integrityVerifier;
PersistentWillBeMember

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:195: visitor->trace(m_request);
+ visitor->trace(m_integrityVerifier);

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:165: 
-empty line

[jww, 2015-08-19 16:43:40]

Let me know if the added Request tests are what you're looking for.

https://codereview.chromium.org/1279163005/diff/100001/LayoutTests/http/tests...
File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html
(right):

https://codereview.chromium.org/1279163005/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:27:
.then(test.step_func(function(response) {
On 2015/08/19 10:42:36, yhirano wrote:
> You can use promise_test. Then you don't have to insert test.step_func,
> assert_unreached, test.done, etc.

Done.

https://codereview.chromium.org/1279163005/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:30:
consume(response.body.getReader(), test, expectedValue, '');
On 2015/08/19 10:42:36, yhirano wrote:
> You can use response.text().

Done.

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:169: Member<SRIVerifier>
m_integrityVerifier;
On 2015/08/19 10:42:36, yhirano wrote:
> PersistentWillBeMember

Done.

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:195: visitor->trace(m_request);
On 2015/08/19 10:42:36, yhirano wrote:
> + visitor->trace(m_integrityVerifier);

Done.

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1279163005/diff/100001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:165: 
On 2015/08/19 10:42:36, yhirano wrote:
> -empty line

Done.

[jww, 2015-08-19 18:08:00]

jww@chromium.org changed reviewers:
+ mkwst@chromium.org

[jww, 2015-08-19 18:08:00]

mkwst@, can you take a look at changes to:
Source/core/dom/ScriptLoader.cpp
Source/core/frame/SubresourceIntegrity.h
Source/core/frame/SubresourceIntegrity.cpp
Source/core/html/HTMLLinkElement.cpp

[yhirano, 2015-08-20 02:23:38]

lgtm

https://codereview.chromium.org/1279163005/diff/120001/LayoutTests/http/tests...
File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html
(right):

https://codereview.chromium.org/1279163005/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:30:
if (!pass)
assert_true(pass, ...);

https://codereview.chromium.org/1279163005/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:33:
if (pass && expectedValue) {
|pass| must be true here, you don't have to check it.

https://codereview.chromium.org/1279163005/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:39:
if (pass)
assert_false(pass, ...);

[jww, 2015-08-20 02:57:28]

https://codereview.chromium.org/1279163005/diff/120001/LayoutTests/http/tests...
File
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html
(right):

https://codereview.chromium.org/1279163005/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:30:
if (!pass)
On 2015/08/20 02:23:37, yhirano wrote:
> assert_true(pass, ...);

Done.

https://codereview.chromium.org/1279163005/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:33:
if (pass && expectedValue) {
On 2015/08/20 02:23:37, yhirano wrote:
> |pass| must be true here, you don't have to check it.

Done.

https://codereview.chromium.org/1279163005/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-fetch.html:39:
if (pass)
On 2015/08/20 02:23:37, yhirano wrote:
> assert_false(pass, ...);

Done.

[Mike West, 2015-08-20 20:13:14]

On 2015/08/19 at 18:08:00, jww wrote:
> mkwst@, can you take a look at changes to:
> Source/core/dom/ScriptLoader.cpp
> Source/core/frame/SubresourceIntegrity.h
> Source/core/frame/SubresourceIntegrity.cpp
> Source/core/html/HTMLLinkElement.cpp

These files LGTM.

[sof, 2015-08-20 20:29:29]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-08-20 20:29:30]

https://codereview.chromium.org/1279163005/diff/140001/Source/core/frame/Subr...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/1279163005/diff/140001/Source/core/frame/Subr...
Source/core/frame/SubresourceIntegrity.cpp:119: logErrorToConsole(errorMessage,
document);
Don't you need to consider 'result' before logging?

[jww, 2015-08-20 23:16:45]

https://codereview.chromium.org/1279163005/diff/140001/Source/core/frame/Subr...
File Source/core/frame/SubresourceIntegrity.cpp (right):

https://codereview.chromium.org/1279163005/diff/140001/Source/core/frame/Subr...
Source/core/frame/SubresourceIntegrity.cpp:119: logErrorToConsole(errorMessage,
document);
On 2015/08/20 20:29:29, sof wrote:
> Don't you need to consider 'result' before logging?

Yes, fantastic catch. Thanks, and fixed.

[jww, 2015-08-20 23:17:00]

The CQ bit was checked by jww@chromium.org

[jww, 2015-08-20 23:17:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1279163005/#ps160001
(title: "Rebase on ToT")

[commit-bot: I haz the power, 2015-08-20 23:17:29]

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

[commit-bot: I haz the power, 2015-08-21 00:24:13]

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

[commit-bot: I haz the power, 2015-08-21 00:24:14]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jww, 2015-08-21 05:28:32]

The CQ bit was checked by jww@chromium.org

[jww, 2015-08-21 05:28:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1279163005/#ps180001
(title: "Fix test expectations")

[commit-bot: I haz the power, 2015-08-21 05:28:54]

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

[commit-bot: I haz the power, 2015-08-21 07:59:59]

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

[commit-bot: I haz the power, 2015-08-21 08:00:01]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jww, 2015-08-21 13:10:47]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2015-08-21 13:11:05]

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

[commit-bot: I haz the power, 2015-08-21 14:52:07]

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

[commit-bot: I haz the power, 2015-08-21 14:52:08]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jww, 2015-08-21 16:35:02]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2015-08-21 16:35:22]

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

[commit-bot: I haz the power, 2015-08-21 17:40:07]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200995