Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 9 years ago

The fetch spec is now better integrated with SRI:

  https://github.com/whatwg/fetch/issues/85

Our SRI implementation is still in progress in bug 992096, but we should make sure its integrated properly once its done.

	François Marier [:francois]
	Comment 1 • 9 years ago

I've just documented how the SRI code hooks into the script and style elements: https://wiki.mozilla.org/Security/Subresource_Integrity#Engineering

	Tom Tung [:tt, :ttung] Assignee
	Comment 2 • 8 years ago

I would like to try on this bug.

	Frederik Braun [:freddy]
	Comment 3 • 8 years ago

Tom, what's the timeline for this bug?

	Tom Tung [:tt, :ttung] Assignee
	Comment 4 • 8 years ago

Hi Frederik,

Currently, I'm working on another bug and just investigating the background of this bug. I will focus on this bug recently. Does this bug blocks something important?

	Frederik Braun [:freddy]
	Comment 5 • 8 years ago

Nah, just curious.

	Tom Tung [:tt, :ttung] Assignee
	Comment 6 • 8 years ago

WIP patch
current progress: 
1. add integrity metadata attribute in Request & InternalRequest
2. part of Step 15 in "Main fetch" (not in this patch since it haven't been completed yet) 
Next Step:
1. Modify type of mIntegrity from "nsString" to "SRIMetadata" in |InternalRequest|
2. Finish the Step 15 in "Main fetch"

	Tom Tung [:tt, :ttung] Assignee
	Comment 7 • 8 years ago

Current progress: 
1. It seems that we don't to store Integrity via SRIMetadata in InternalRequest, so keeping store it via nsString.
2. Get stuck at the final step of "3.3.5 Does response match metadataList?" [1]

At step 15 in Main-Fetch, we need to check whether does response match request's integrity metadata or not. To do this, we need to implement [1] and steps among it. However, I cannot find representation data for response to match request's integrity metadata [2]. It seems that we don't have it in fetch api. Could you provide any hint for this, Ben? Thanks!

[1] https://w3c.github.io/webappsec-subresource-integrity/#does-response-match-metadatalist
[2] https://w3c.github.io/webappsec-subresource-integrity/#apply-algorithm-to-response

	François Marier [:francois]
	Comment 8 • 8 years ago

(In reply to Tom Tung from comment #7)
> Current progress: 
> 1. It seems that we don't to store Integrity via SRIMetadata in
> InternalRequest, so keeping store it via nsString.

The SRIMetadata constructor takes a string and converts it to the right structure:

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/security/SRIMetadata.cpp#27-72

> 2. Get stuck at the final step of "3.3.5 Does response match metadataList?"
> [1]
> 
> At step 15 in Main-Fetch, we need to check whether does response match
> request's integrity metadata or not. To do this, we need to implement [1]
> and steps among it.

That algorithm is already implemented here in SRICheck::Verify():

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/security/SRICheck.cpp#340-385

	Tom Tung [:tt, :ttung] Assignee
	Comment 9 • 8 years ago

(In reply to François Marier [:francois] from comment #8)
Thanks, I'll try to use it!

	Tom Tung [:tt, :ttung] Assignee
	Comment 10 • 8 years ago

Thanks for hints from Francois, but I still have somethings need to be figured out. I address them with my solutions for them below.

1. In Main fetch step 15-1, "Wait for response's body". In my native think, I need to implement it in funtion "OnStreamComplete()". However, we don't have it in FetchDriver, so I'll try to implement one in FetchDriver.cpp.

2. Although function SRICheck::Verify() can verify response, I still need to update the hash value as [1]. Thus, I'll try to call VerifyIntegrity() for verify the response. The problem is I don't know what should be put into this function, such as "aLoader", "aString" and "aCORSMode". "aLoader" could be a argument from funtion OnStreamComplete(). I guess "aString" could be the stream value from response's body and "aCORSMode" could be get like [3]. I'll check whether it's correct or not.

[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/security/SRICheck.cpp#208
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1895
[3] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#1890

	Tom Tung [:tt, :ttung] Assignee
	Comment 11 • 8 years ago

Sorry for late reply, I was working on another bug and study another api. 

Thanks for François! After face-to-face teaching, I figure out more about this bug and how should SRI works with fetch. This patch have almost been done. The only thing left is setting response and internalResponse to a network error. I believe that I could finish it soon.

	Tom Tung [:tt, :ttung] Assignee
	Comment 12 • 8 years ago

Current status:
Can pass all the test in web-platform/tests/fetch/api/basic/integrity.html but fail in integrity-worker.html. The reason is worker don't set a document in FetchDriver because of bug 1224855. However, SRICheck::IntegrityMetadata() need document to be set.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 13 • 8 years ago

As far as I can tell that's just needed for a couple ReportToConsole() calls in SRICheck::IntegrityMetadata(), right?

	Tom Tung [:tt, :ttung] Assignee
	Comment 14 • 8 years ago

Yes, it looks like that. So do we need documnet object in worker or just create another SRICheck function without ReportToConsole()? I'm curious about whether we need ReportToConsole() for worker?

	Frederik Braun [:freddy]
	Comment 15 • 8 years ago

Debugging would be hard if there's no devtools reporting for stuff happening in a worker.
But maybe this could be done as a follow-up?

	Tom Tung [:tt, :ttung] Assignee
	Comment 16 • 8 years ago

Do you means I could write a patch(write another SRICheck::IntegrityMetadata() without calling ReportToConsole()) and file a follow-up bug for finding another way to have devtools report?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 17 • 8 years ago

You could probably change SRICheck::Metadata() to take an nsIConsoleReportCollector and then flush it to the right place.  If the FetchDriver has an mDocument, it can flush it there.  If it doesn't then it would need to get it to the right worker-related documents.  Thats a bit complicated.

For a service worker we actually have a heuristic for finding documents defined in ServiceWorkerManager::ReportToAllClients().  We could create a new version of that method that takes an nsIConsoleReportCollector instead of the message directly.  It could then flush to each client document.

Dedicated workers might have their own document associated with them.  Shared workers also have a number of shared documents.  We could make a method on WorkerPrivate that takes an nsIConsoleReportCollector and flushes the reports to the right place.

It would be nice to do that here, but it could also be a follow-up.  If we allow a nullptr document to be passed to ReportToConsole() it will just send the message to the browser console instead of the window's web console.

	Tom Tung [:tt, :ttung] Assignee
	Comment 18 • 8 years ago

Sure, I'll try to use nsIConsoleReportCollector here. Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 19 • 8 years ago

(In reply to Tom Tung from comment #12)
> in FetchDriver because of bug 1224855. However,
It's bug 1224865. Sorry for that.

	Tom Tung [:tt, :ttung] Assignee
	Comment 20 • 8 years ago

Hi Ben,

  I divided this bug into two part and this patch is for part one. Part one will be responsible for integrating SRI with Fetch API and part two will be responsible for handling Console message. I'm working on part two. Hopefully, I could complete part two soon, but I would like to ask to review for part one first. Could you help me to review this patch, Ben?

  In this patch, I add an attribute "integrity" into Request/InternalRequest and implement step 17 in Main-fetch. I init the Integrity in OnStartRequest(), copy the stream among OnDataAvilable() and verify the integrity in OnStopRequest(). 
  Secondly, I move the code [1] from HttpFetch() into OnStopRequest(). The main reason is that we should process response after checking integrity and integrity should be verified after OnStopRequest(). 
  Then, in order to allow function IsEligible() SRICheck to check whether is same-origin [2], I add SetDocumentURI() in FetchDriver and SRICheck. 
  Finally, after the changes, we should pass relevant tests in wpt, so I remove the code in testing/web-platform/meta/fetch/api/basic/integrity-worker.html.ini & testing/web-platform/meta/fetch/api/basic/integrity.html.ini

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp?from=fetchdriver.cpp#408-414
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#77

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 21 • 8 years ago

Comment on attachment 8750215 [details] [diff] [review]
Bug 1243791 - v1 - P1 Integrate fetch with SRI - Fetch API & SRICheck changes.

Review of attachment 8750215 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry, but I don't think we can delay resolving the Response for all fetch() calls to OnStopRequest().  I also would like to change how we are passing data down to IsEligible.

I really think we should do the nsIConsoleLogCollector changes in this bug.  It would make it clearer where the document is actually required for other things vs just reporting.

Finally, I think this should ultimately get review from Christoph or Jonas for the SRI changes we are making.

Thanks for working on this!

::: dom/fetch/Fetch.cpp
@@ +137,5 @@
>      MOZ_ASSERT(loadGroup);
>      RefPtr<FetchDriver> fetch = new FetchDriver(mRequest, principal, loadGroup);
> +    nsIDocument* doc = proxy->GetWorkerPrivate()->GetDocument();
> +    if (doc) {
> +      fetch->SetDocumentURI(doc->GetDocumentURI());

I don't think this is correct.  Not all workers have a document.

Also, for the purposes of the SRI IsEligible() check, I think you could just use the worker script.

Ultimately, though, I think what we should really be doing is passing the LoadTainting value down to IsEligible():

  https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadTainting.h#34

You can get this value from here:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#122

If the tainting is Basic or CORS, then IsEligible should return NS_OK.  If its Opaque, then IsEligible() should return NS_ERROR_SRI_NOT_ELIGIBLE.

Again we have the logging issue if IsEligible() fails.

You probably will need to make a different version of SRICheckDataVerifier() that takes the LoadTainting as well in order to pass the value down.

::: dom/fetch/FetchDriver.cpp
@@ +696,5 @@
>    } else {
>      MOZ_ASSERT(mResponse);
>      MOZ_ASSERT(!mResponse->IsError());
>  
> +    // From "Main Fetch" step 17: SRI-part3.

See my note below, but this needs to be done earlier than OnStopRequest().

@@ +729,5 @@
>  
>    if (mObserver) {
> +    //From "Main Fetch" step 23: Process response.
> +    MOZ_ASSERT(mResponse);
> +    mObserver->OnResponseAvailable(mResponse);

So, this is implementing step 17.1 from Main Fetch:

  "Wait for response body"

But its doing it unconditionally.  This will block resolving the Response until the body is fully present even for requests not using SRI.

I don't think we should include this perf hit here.  We need to implement the Main Fetch step 17 conditional check:

  "If response is not a network error and request's integrity metadata is not the empty string, run these substeps:"

So we resolve the Response in BeginAndGetFilteredResponse() normally, but wait to resolve it until OnStopRequest() if SRI is in play.

	Tom Tung [:tt, :ttung] Assignee
	Comment 22 • 8 years ago

Thanks for the feedback! I'm sorry about wrong direction about patch. I'll make it better next time. I have some questions for comments and I leave them below inline. Could you help me to figure them out?

(In reply to Ben Kelly [:bkelly] from comment #21)
> I'm sorry, but I don't think we can delay resolving the Response for all
> fetch() calls to OnStopRequest().  I also would like to change how we are
> passing data down to IsEligible.
> 
> I really think we should do the nsIConsoleLogCollector changes in this bug. 
> It would make it clearer where the document is actually required for other
> things vs just reporting.

Yeah, I plan to do it in another patch(P2).

> Finally, I think this should ultimately get review from Christoph or Jonas
> for the SRI changes we are making.

Sure, thanks for reminder!
 
> ::: dom/fetch/Fetch.cpp
> @@ +137,5 @@
> >      MOZ_ASSERT(loadGroup);
> >      RefPtr<FetchDriver> fetch = new FetchDriver(mRequest, principal, loadGroup);
> > +    nsIDocument* doc = proxy->GetWorkerPrivate()->GetDocument();
> > +    if (doc) {
> > +      fetch->SetDocumentURI(doc->GetDocumentURI());
> 
> I don't think this is correct.  Not all workers have a document.
> 
> Also, for the purposes of the SRI IsEligible() check, I think you could just
> use the worker script.
> 
> Ultimately, though, I think what we should really be doing is passing the
> LoadTainting value down to IsEligible():
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadTainting.h#34
> 
> You can get this value from here:
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalResponse.
> cpp#122

Could you explain more about why we should use LoadTainting rather than ResponseType since SRI spec, step 2 [1] links to response type.

[1] https://www.w3.org/TR/SRI/#is-response-eligible-for-integrity-validation

> 
> If the tainting is Basic or CORS, then IsEligible should return NS_OK.  If
> its Opaque, then IsEligible() should return NS_ERROR_SRI_NOT_ELIGIBLE.
> 
> Again we have the logging issue if IsEligible() fails.
> 
> You probably will need to make a different version of SRICheckDataVerifier()
> that takes the LoadTainting as well in order to pass the value down.

I do the mapping then call verify because I asked François before. He suggested that I could map from ResponseType::Cors to CORS_ANONYMOUS and translate others to CORS_NONE then call SRIDataVerifier::Verify(). 

Did you mean I should translate these enum(CORS, DEFAULT, ... etc) into CORS_xxx in SRIDataVerifier::Verify() instead of in FetchDriver::OnStopRequest()?

> @@ +729,5 @@
> >  
> >    if (mObserver) {
> > +    //From "Main Fetch" step 23: Process response.
> > +    MOZ_ASSERT(mResponse);
> > +    mObserver->OnResponseAvailable(mResponse);
> 
> So, this is implementing step 17.1 from Main Fetch:
> 
>   "Wait for response body"
> 
> But its doing it unconditionally.  This will block resolving the Response
> until the body is fully present even for requests not using SRI.
> 
> I don't think we should include this perf hit here.  We need to implement
> the Main Fetch step 17 conditional check:
> 
>   "If response is not a network error and request's integrity metadata is
> not the empty string, run these substeps:"
> 
> So we resolve the Response in BeginAndGetFilteredResponse() normally, but
> wait to resolve it until OnStopRequest() if SRI is in play.

Sure. Just want to make sure that I understand. I need to add if condition at [1] to check if response is not a network error and request's integrity metadata is not the empty string. If it's true then do it in OnStopRequest(), otherwise process filteredResponse as usual, right? 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#408-413

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 23 • 8 years ago

(In reply to Tom Tung from comment #22)
> Could you explain more about why we should use LoadTainting rather than
> ResponseType since SRI spec, step 2 [1] links to response type.
> 
> [1] https://www.w3.org/TR/SRI/#is-response-eligible-for-integrity-validation

I'm saying to use LoadTainting instead of calling CheckSameOriginURI() in IsEligible().  So you don't need the documentURI.  The LoadTainting is already the result of performing same origin checks, so it provides the same information.

I prefer LoadTainting over ResponseType because it makes the SRICheck API less specific to Fetch.  LoadTainting is defined in our netwerk stack regardless of fetch().  Any nsIChannel created with AsyncOpen2() will have a valid LoadTainting value that can be used with the API.

The LoadTainting also combines the information in the documentURI and CORSMode into a single enum:

1) LoadTainting::Basic == CORS_NONE with same-origin documentURI
2) LoadTainting::CORS == CORS_ANONYMOUS or CORS_USE_CREDENTIALS with cross-origin documentURI
3) LoadTainting::Opaque == CORS_NONE with cross-origin documentURI

We basically just need to prevent SRI when we have LoadTainting::Opaque.

> Sure. Just want to make sure that I understand. I need to add if condition
> at [1] to check if response is not a network error and request's integrity
> metadata is not the empty string. If it's true then do it in
> OnStopRequest(), otherwise process filteredResponse as usual, right? 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#408-
> 413

Correct.  You only want to delay resolving the Response if SRI is actually going to be checked.

	François Marier [:francois]
	Comment 24 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #21)
> Finally, I think this should ultimately get review from Christoph or Jonas
> for the SRI changes we are making.

I'm happy to review the SRI changes once they're ready.

	Tom Tung [:tt, :ttung] Assignee
	Comment 25 • 8 years ago

I'm trying on change arguments for IsEligible from CORSMode to LoadTainting. After I changed that, I got failed at test_style_crossdomain.html, test "valid non-CORS sha256 hash in a data: URL" [1].  I expected that I can get LoadTainting::opaque here but I can only get LoadTainting::Basic. Still try to figure out why...


[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/sri/iframe_style_crossdomain.html#78

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 26 • 8 years ago

(In reply to Tom Tung [:tt] from comment #25)
> I'm trying on change arguments for IsEligible from CORSMode to LoadTainting.
> After I changed that, I got failed at test_style_crossdomain.html, test
> "valid non-CORS sha256 hash in a data: URL" [1].  I expected that I can get
> LoadTainting::opaque here but I can only get LoadTainting::Basic. Still try
> to figure out why...
> 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/security/test/sri/
> iframe_style_crossdomain.html#78

I think that test is wrong.  Our stylesheet loaded is supposed to inherit the origin:

https://dxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#1617

I think this is a bug in our existing SRI implementation.

Francois, what do you think?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 27 • 8 years ago

Sorry, I was trying to say the origin should be inherited when loading a stylesheet with a data URI.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 28 • 8 years ago

Also note the SRI spec says to check the result tainting and not just compare the URI:

https://w3c.github.io/webappsec-subresource-integrity/#is-response-eligible-for-integrity-validation

So I'm fairly confident that test is wrong.

	Tom Tung [:tt, :ttung] Assignee
	Comment 29 • 8 years ago

Thanks for helping, Ben. 
That test expected to get false because it is not eligible. I thought the test should be correct but something is wrong due to the fourth note "Note Since the response type for data URLs will always be “opaque” for script and link elements ..." [1]. Thus, I expected to get LoadTainting::Opaque since response type is Opaque rather than LoadTainting::Basic. 

[1] https://w3c.github.io/webappsec-subresource-integrity/#is-response-eligible-for-integrity-validation

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 30 • 8 years ago

Hmm, interesting.  I think that note overstates things a bit.  AFAIK the stylesheet loading is not specified here.  In gecko we do inherit the origin and treat them as same-origin.

I wrote a spec issue against SRI:

https://github.com/w3c/webappsec-subresource-integrity/issues/35

In any case, whether stylesheet treats data URL as same-origin or not is a separate issue.  SRI should simply work based on the tainting value.  If we change stylesheet not to inherit the origin for data URL in the future then it will start getting blocked automatically.

I think we should change the two data URL test cases to pass.  We should then add another test case for "http://example.com/tests/dom/security/test/sri/style1.css" that does not use CORS to show that an opaque response is blocked.

Does that sound good?

	Tom Tung [:tt, :ttung] Assignee
	Comment 31 • 8 years ago

Ok, I see. I'll try to do that. Thanks!

	François Marier [:francois]
	Comment 32 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #30)
> I think we should change the two data URL test cases to pass.  We should
> then add another test case for
> "http://example.com/tests/dom/security/test/sri/style1.css" that does not
> use CORS to show that an opaque response is blocked.
> 
> Does that sound good?

Pending resolution of the spec issue you raised, I'd say that's a reasonable plan for now.

	Tom Tung [:tt, :ttung] Assignee
	Comment 33 • 8 years ago

WIP patch for SRI changes. Addressed comments expect changing method for ReportToConsole.

	Tom Tung [:tt, :ttung] Assignee
	Comment 34 • 8 years ago

This is also WIP patch and it's for change about SRI test.

	Tom Tung [:tt, :ttung] Assignee
	Comment 35 • 8 years ago

WIP patch for Fetch API modify. 

Sorry for the late reply, I was working on another bug and studying storage API. I'll try to solve this bug as soon as possible.

	Tom Tung [:tt, :ttung] Assignee
	Comment 36 • 8 years ago

WIP patch for wpt tests enable

	Tom Tung [:tt, :ttung] Assignee
	Comment 37 • 8 years ago

WIP patch. Add a function FlushReportToAllClients() in order to provide a method to report to all SW's clients.

	Tom Tung [:tt, :ttung] Assignee
	Comment 38 • 8 years ago

WIP patch. The changes in this patch is mainly for report to worker's console. 

Next Step: Provide a mechanism for workers(dedicated and shared) Fetch API to pass document to SRICheck and servicewroker to pass its scope.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 39 • 8 years ago

(In reply to Tom Tung [:tt] from comment #39)
> Sorry for delay these day. I have few questions about how to pass document
> object to SRICheck API in worker case. I address them below inline.
> Could you give me some hints about these, Ben? Thanks!

So what I was proposing was that code that wants to use SRI do something like:

1) Create a ConsoleReportCollector object.
2) Pass the collector to the SRI API.
3) After returning from the SRI API, call ConsoleReportCollector::FlushConsoleReports(document).

The document in (3) would depend on who is using SRI.

- In a window you can just use the windows document.
- If the WorkerPrivate or its parent change has a document, then you can use that.
- On shared worker you would need to call it once for each document attached to the SharedWorker.
- For a service worker you would need to make a new version of ServiceWorkerManager::ReportToAllClients() that takes an nsIConsoleReportCollector and flushes repeatedly to each document found.

For the last two here, you need to make a way for the collector to "flush" without losing its list of reports.  Maybe an optional second argument to FlushConsoleReport(document, forgetReports=true).

> 1. In order to flush report to worker's document, I should pass dedicated
> worker's "loading" document(which is from WorkerPrivate), right? Or is there
> any other ways to get directly dedicated worker's document from
> main-thread(FetchDriver)?
> 2. If right, for dedicated worker case, should I pass a "loading" document
> from FetchRequest() into FetchDriver at [1]? I ask this question because bug
> bug 1224865 remove it.

You probably want to add a virtual method to FetchDriverObserver like GetReportCollector().  The FetchDriverObserver implementation can then be responsible for flushing any reports when it sees the FetchDriver complete.  The WorkerFetchResolver implementation of FetchDriverObserver should have access to the WorkerPrivate in question.

Sorry its still a bit hand wavy.  Does that make sense?

	Tom Tung [:tt, :ttung] Assignee
	Comment 40 • 8 years ago

Sorry for delay these day. I have few questions about how to pass document object to SRICheck API in worker case. I address them below inline.
Could you give me some hints about these, Ben? Thanks!

1. In order to flush report to worker's document, I should pass dedicated worker's "loading" document(which is from WorkerPrivate), right? Or is there any other ways to get directly dedicated worker's document from main-thread(FetchDriver)?
2. If right, for dedicated worker case, should I pass a "loading" document from FetchRequest() into FetchDriver at [1]? I ask this question because bug bug 1224865 remove it.
[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp?from=fetch.cpp#139

	Tom Tung [:tt, :ttung] Assignee
	Comment 41 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #40)
> Sorry its still a bit hand wavy.  Does that make sense?
Yes, it much clearer for me. Thanks for teaching me that!

	Tom Tung [:tt, :ttung] Assignee
	Comment 42 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62ef1c148578

	Tom Tung [:tt, :ttung] Assignee
	Comment 48 • 8 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e3e4d0c558e&selectedJob=21670534

It seems that it still have few problems when running try, but I could pass tests in my environment (mac debug mode). I'll send them to review once I fix them.

	Tom Tung [:tt, :ttung] Assignee
	Comment 49 • 8 years ago

Comment on attachment 8757860 [details] [diff] [review]
Bug-1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed.

Hi Ben,

In this patch, I find a problem when I implement nsIConsoleReportCollector as your comment#40. I could not pass a collector to FetchDriverObserver since the collector is reference to nsIChannel which cannot run on different thread(main thread and worker thread). Besides, I cannot pass nsIDocument to FetchDriverObserver either for same reason. Thus, I pass worker's document to FetchDriver and flush it at OnStopRequest() after finishing SRICheck::Verify().

However, when I run test integrity-worker.html [1], I can only find the logs in browser console rather than web console. It seems that I flush report in wrong timing. Could you give some hint to solve that? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/api/basic/integrity-worker.html?from=integrity-worker.html

	Tom Tung [:tt, :ttung] Assignee
	Comment 50 • 8 years ago

(In reply to Tom Tung [:tt] from comment #48)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1e3e4d0c558e&selectedJob=21670534

The failed tests happened because I call SRIDataVerifier::Update() on FetchDriver::OnDataAvailable() which is not called on the main thread. However, I call SRIDataVerifier::Verify() on FetchDriver::OnStopRequest which is called on the main thread and 'nsCryptoHash' is not threadsafe. Thus, I hit the MOZ_CRASH.

I'll try to implement a Runnable to avoid they will be called on different threads.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=21673090&repo=try#L17146

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 51 • 8 years ago

The other option is to just not do the retarget if SRI is play:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp?from=FetchDriver.cpp#604

It should all work without retargeting to the stream transport service since some channel configurations don't support it anyway.

	Tom Tung [:tt, :ttung] Assignee
	Comment 52 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #51)
> The other option is to just not do the retarget if SRI is play:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.
> cpp?from=FetchDriver.cpp#604
> 
> It should all work without retargeting to the stream transport service since
> some channel configurations don't support it anyway.

This approach is much better than create a runnable to dispatch another thread. I'll implement that! Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 53 • 8 years ago

Comment on attachment 8757860 [details] [diff] [review]
Bug-1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed.

Review of attachment 8757860 [details] [diff] [review]:
-----------------------------------------------------------------

This is moving in the right direction, but I think it would be preferable to remove conditional logic where possible.  So I'd rather we just always used a nsIConsoleReportCollector.  The SRI code should just be document agnostic.  Where the collector is flushed to should totally be determined with the FetchObserver.

::: dom/fetch/FetchDriver.cpp
@@ +602,5 @@
> +  mRequest->GetIntegrity(integrity);
> +  if (mResponse->Type() != ResponseType::Error && !integrity.IsEmpty() &&
> +      mSRIMetadata.IsEmpty()) {
> +    if (!mDocument) {
> +      nsCOMPtr<nsIConsoleReportCollector> collector = do_QueryInterface(channel);

I was more thinking you would do:

  nsCOMPtr<nsIConsoleReportCollector> collector = new ConsoleReportCollector();

From here:

  https://dxr.mozilla.org/mozilla-central/source/dom/base/ConsoleReportCollector.h#16

And I think the observer should probably own the collector.  So here you would call mObserver->GetCollector() and the collector would be responsible for creating/returning a collector.  It can then decide to flush when it is convenient for it (probably in OnResponseEnd).

@@ +604,5 @@
> +      mSRIMetadata.IsEmpty()) {
> +    if (!mDocument) {
> +      nsCOMPtr<nsIConsoleReportCollector> collector = do_QueryInterface(channel);
> +      MOZ_ASSERT(collector);
> +      mSRIMetadata.SetReporter(collector);

I don't see the changes to SRIMetadata in this patch.

@@ +701,5 @@
> +    if (mResponse->Type() != ResponseType::Error && !integrity.IsEmpty()) {
> +      MOZ_ASSERT(mSRIDataVerifier);
> +
> +      nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
> +      nsresult rv = mSRIDataVerifier->Verify(mSRIMetadata, channel, mDocument);

I was thinking we would pass the collector explicitly to SRI instead of a document.  Just make the SRI code document-agnostic.

@@ +705,5 @@
> +      nsresult rv = mSRIDataVerifier->Verify(mSRIMetadata, channel, mDocument);
> +
> +      // Only worker need to flush console report.
> +      if (mObserver && !mDocument) {
> +        mObserver->FlushConsoleReport(channel, mWorkerDocument);

I think it would be simpler to just always flush through the reporter whether you are main thread or worker thread.

	Tom Tung [:tt, :ttung] Assignee
	Comment 54 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #53)

Thanks for feedback! 

> Comment on attachment 8757860 [details] [diff] [review]
> Bug-1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector
> to show console report - fetch API changed.
> This is moving in the right direction, but I think it would be preferable to
> remove conditional logic where possible.  So I'd rather we just always used
> a nsIConsoleReportCollector.  The SRI code should just be document agnostic.
> Where the collector is flushed to should totally be determined with the
> FetchObserver.

Does "SRI code be document agnostic" means removing nsIDocument argument for all the function in SRI?
If it is, should I file follow-up bug for that since it need to few changes in Loader.cpp and nsScriptLoader.cpp (find a place to flush the report)?

> ::: dom/fetch/FetchDriver.cpp
> @@ +602,5 @@
> > +      nsCOMPtr<nsIConsoleReportCollector> collector = do_QueryInterface(channel);
> I was more thinking you would do:
> 
>   nsCOMPtr<nsIConsoleReportCollector> collector = new
> ConsoleReportCollector();
> 
> From here:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/base/
> ConsoleReportCollector.h#16 


> And I think the observer should probably own the collector.  So here you
> would call mObserver->GetCollector() and the collector would be responsible
> for creating/returning a collector.  It can then decide to flush when it is
> convenient for it (probably in OnResponseEnd).
> 
> @@ +604,5 @@
> > +      mSRIMetadata.IsEmpty()) {
> > +    if (!mDocument) {
> > +      nsCOMPtr<nsIConsoleReportCollector> collector = do_QueryInterface(channel);
> > +      MOZ_ASSERT(collector);
> > +      mSRIMetadata.SetReporter(collector);
> 
> I don't see the changes to SRIMetadata in this patch.

Oh, I do them in P2. Sorry for that, but I wants to check whether I do the right way for flushing report to worker first. I would like to send review request for them after I complete that.

> 
> @@ +701,5 @@
> > +    if (mResponse->Type() != ResponseType::Error && !integrity.IsEmpty()) {
> > +      MOZ_ASSERT(mSRIDataVerifier);
> > +
> > +      nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
> > +      nsresult rv = mSRIDataVerifier->Verify(mSRIMetadata, channel, mDocument);
> 
> I was thinking we would pass the collector explicitly to SRI instead of a
> document.  Just make the SRI code document-agnostic.

The reason is that I want to use nsContentUtils::ReportToConsole() if having a nsIDocument. Actually I'm not sure about different between using nsContentUtils::ReportToConsole() directly and calling AddConsoleReport() then FlushConsoleReports() if we can get a nsIDocument object. Thus, I call AddConsoleReport() only if we cannot get a nsIDocument object. Besides, it makes less changes for Loader and ScriptLoader. :P

Could you tell me more about different between them when we already have a nsDocument? Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 55 • 8 years ago

(In reply to Tom Tung [:tt] from comment #54)
> > Comment on attachment 8757860 [details] [diff] [review]
> > Bug-1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector
> > to show console report - fetch API changed.
> > This is moving in the right direction, but I think it would be preferable to
> > remove conditional logic where possible.  So I'd rather we just always used
> > a nsIConsoleReportCollector.  The SRI code should just be document agnostic.
> > Where the collector is flushed to should totally be determined with the
> > FetchObserver.
> 
> Does "SRI code be document agnostic" means removing nsIDocument argument for
> all the function in SRI?
> If it is, should I file follow-up bug for that since it need to few changes
> in Loader.cpp and nsScriptLoader.cpp (find a place to flush the report)?

I mean the main methods with significant code in them in SRI should not reference nsIDocument if we can just use nsIConsoleReportCollector instead.

There are two options to handle Loader.cpp and nsScriptLoader.cpp:

1) Make them create a ConsoleReportCollector and flush to their document manually.
2) Make a wrapper method in SRI that does this, but leaves the main SRI algorithms document-agnostic.

> > @@ +701,5 @@
> > > +    if (mResponse->Type() != ResponseType::Error && !integrity.IsEmpty()) {
> > > +      MOZ_ASSERT(mSRIDataVerifier);
> > > +
> > > +      nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
> > > +      nsresult rv = mSRIDataVerifier->Verify(mSRIMetadata, channel, mDocument);
> > 
> > I was thinking we would pass the collector explicitly to SRI instead of a
> > document.  Just make the SRI code document-agnostic.
> 
> The reason is that I want to use nsContentUtils::ReportToConsole() if having
> a nsIDocument. Actually I'm not sure about different between using
> nsContentUtils::ReportToConsole() directly and calling AddConsoleReport()
> then FlushConsoleReports() if we can get a nsIDocument object. Thus, I call
> AddConsoleReport() only if we cannot get a nsIDocument object. Besides, it
> makes less changes for Loader and ScriptLoader. :P
> 
> Could you tell me more about different between them when we already have a
> nsDocument? Thanks!

Yea, I just think it makes for a lot of special case logic that makes it hard to follow when the document is used or not.  I really think it would be easier to maintain if we always do the same thing (nsIConsoleReportCollector::AddConsoleReport()) and then flush to the document after the SRI call.

	Tom Tung [:tt, :ttung] Assignee
	Comment 56 • 8 years ago

Hi Ben, 

  Sorry for the really late reply, I was stuck with cannot find the debug messages in web console. :( 
  I find the reason causing this problem finally. The reason is that I didn't pass SourceFileURI for AddConsoleReport(...) [1] and the pending reports are ignore while calling FlushConsoleReposrts(...) [2]. I did this becuase a SourceFileURI is not provide in SRI API [3], and ReportToConsole(...) can be called when there is no SourceFileURI object. Therefore I pass a EmptyCString() for no SourceFileURI as calling AddConsoleReport. 
  Right now, I just comment [2] to make the debugging reports flush to console. Is there anything wrong for passing a EmptyCString() for SourceFileURI? Could you help me to fix that? Besides, would you mind taking a look at the Console reporting mechanism that I implement? Thanks!

I create this patch for modifying the mechanism to report to console in SRI API and corresponding caller. 
In this patch: 1. Add a function for reporting to console in SRI API.
               2. Using LoadTainting to decide whehter is CORS or not.
               3. Removing passing all nsIDocument object for SRI API.
               4. Let caller class(Loader/nsScriptLoader) hold a nsIConsoleReportCollector pointer, create it as the class init, flush the report at suitable timing and pass it into SRI when call SRI's API.

Note: The reason that "I implement #4 rather than let SRIMetadata hold a ConsoleReportCollector" is there may have more than one SRIMetadata while Checking SRI [4] and I believe that it(#4) is also a well mechanism for reporting to console. I'll make fetch API(Patch 4) do the same mechanism if it is ok for you.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/ConsoleReportCollector.h?from=ConsoleReportCollector#24
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/ConsoleReportCollector.cpp#58-60
[3] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#86-91
[4] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#160

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 57 • 8 years ago

Comment on attachment 8760056 [details] [diff] [review]
Bug-1187335 - P2 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not

Review of attachment 8760056 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  Thanks.

::: dom/base/ConsoleReportCollector.cpp
@@ +55,5 @@
>      // just turns around and converts it back to a spec.
>      nsCOMPtr<nsIURI> uri;
>      nsresult rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);
>      if (NS_FAILED(rv)) {
> +    //  continue;

So this fails when mSourceFileURI is empty string?  I guess wrap the NS_NewURI() call and its error checking in a "if (!report.mSourceFileURI.IsEmpty())".  And then maybe change the error checking to an assertion with MOZ_ALWAYS_SUCCEEDS(rv).

What do you think?

::: dom/security/SRIMetadata.h
@@ +8,5 @@
>  #define mozilla_dom_SRIMetadata_h
>  
>  #include "nsTArray.h"
>  #include "nsString.h"
> +#include "nsIConsoleReportCollector.h"

I don't think this is the right place for this include.  You probably want to forward declare in SRICheck.h and include the header in SRICheck.cpp.

	Tom Tung [:tt, :ttung] Assignee
	Comment 58 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #57)
Thanks for the feedback!
 
> ::: dom/base/ConsoleReportCollector.cpp
> @@ +55,5 @@
> >      // just turns around and converts it back to a spec.
> >      nsCOMPtr<nsIURI> uri;
> >      nsresult rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);
> >      if (NS_FAILED(rv)) {
> > +    //  continue;
> 
> So this fails when mSourceFileURI is empty string?  I guess wrap the

Yes, it fails when mSourceFileURI is empty string.

> NS_NewURI() call and its error checking in a "if
> (!report.mSourceFileURI.IsEmpty())".  And then maybe change the error
> checking to an assertion with MOZ_ALWAYS_SUCCEEDS(rv).
> 
> What do you think?

Yeah, adding an "if" condition can avoid failure at empty string. However, I wonder about once we change the assert, It will also change the current behavior. If we change the assertion to MOZ_ALWAYS_SUCCEEDS, it may affect all the other pending reports once a bad report hit the assert. The current behavior will leave bad report behind and keep process the other pending reports. Should we do that or I misunderstand something?
 
> ::: dom/security/SRIMetadata.h
> @@ +8,5 @@
> >  #define mozilla_dom_SRIMetadata_h
> >  
> >  #include "nsTArray.h"
> >  #include "nsString.h"
> > +#include "nsIConsoleReportCollector.h"
> 
> I don't think this is the right place for this include.  You probably want
> to forward declare in SRICheck.h and include the header in SRICheck.cpp.

Oh, I forget to remove it. Thanks for the notice! It was used to let the SRIMetadata carry a consolereportcollector.

	Tom Tung [:tt, :ttung] Assignee
	Comment 59 • 8 years ago

(In reply to Tom Tung [:tt][Dayoff: 6/9~6/10, PTO: 6/20] from comment #58)
> > What do you think?
> Should we do that or I misunderstand something?

I just find that I may misunderstand something. Did you mean that just change the assertion and keep "if(..) {continue;}" still there?

	Tom Tung [:tt, :ttung] Assignee
	Comment 60 • 8 years ago

Hi Ben, 

  Could you help me to review this patch? I address your comment, but I put ConsoleReportCollector into P2-1. Thanks!

  As I said in comment #56, this patch is for:
   1. Add a function for reporting to console in SRI API.
   2. Using LoadTainting to decide whehter is CORS or not.
   3. Removing passing all nsIDocument object for SRI API.
   4. Let caller class(Loader/nsScriptLoader) hold a nsIConsoleReportCollector pointer, create it as the class init, flush the report at suitable timing and pass it into SRI when call SRI's API.

	Tom Tung [:tt, :ttung] Assignee
	Comment 61 • 8 years ago

This patch is for ConsoleReportCollector accept an empty SourceURI and add an assertion as SourceURI fail to convert. 

Could you help to review this patch, Ben? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 62 • 8 years ago

Comment on attachment 8757856 [details] [diff] [review]
Bug-1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager for flushing report to all clients.

Hi Ben,

This patch is for ServiceWorker to Flush Reports to right windows and it will be used in Fetch API at Patch #4.  Would you mind helping me to review this patch? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 63 • 8 years ago

Hi Ben,

Sorry for send few review request in a short time, but I would like to ask you review this patch, too. Would you mind helping me to review it? Thanks!

This your patch is mainly for integrity fetch with SRI. 
In this patch, 1. add Integirty attribute to Fetch (Request/InternalRequest)
               2. handle ConsoleReportCollector in FetchDriverObserver and its derived class (create as ctor, pass it to SRI API and flush it at OnRespondEnd())
               3. handle integrity with SRI API in FetchDriver.

	Tom Tung [:tt, :ttung] Assignee
	Comment 64 • 8 years ago

Comment on attachment 8757859 [details] [diff] [review]
Bug-1187335 - P3 - Modify SRI test to match current behavior.

This patch is addressing the comment #30 for correcting the SRI tests. Could you help me to review it, Ben? Thanks!!

	Tom Tung [:tt, :ttung] Assignee
	Comment 65 • 8 years ago

Comment on attachment 8757861 [details] [diff] [review]
Bug-1187335- P5 - Enable SRI's tests and Request's integrity attribute in wpt.

Hi Ben,

This patch is for removing the masks (expect fail) in web-platform test after adding SRI into Fetch API. Could you help me to review this one? Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 66 • 8 years ago

Comment on attachment 8757856 [details] [diff] [review]
Bug-1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager for flushing report to all clients.

Review of attachment 8757856 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +1472,5 @@
> +    }
> +
> +    windows.AppendElement(doc->InnerWindowID());
> +
> +    aReporter->FlushConsoleReports(doc);

So this will flush the reports to the first document seen, free the reports, and then the remaining documents will not get any reports.  See:

https://dxr.mozilla.org/mozilla-central/source/dom/base/ConsoleReportCollector.cpp#48

You need to add an argument to FlushConsoleReports() that lets the caller avoid dropping the reports.  Something like:

enum class ReportAction {
  Forget,
  Save
};

void
FlushConsoleReports(nsIDocument* aDoc, ReportAction aAction = ReportAction::Forget);

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 67 • 8 years ago

Comment on attachment 8760753 [details] [diff] [review]
Bug 1187335 - P2-1 - Add an assertion as SourceURI fail and accept an empty SourceURI for ConsoleReportCollector.

Review of attachment 8760753 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ConsoleReportCollector.cpp
@@ +56,5 @@
>      nsCOMPtr<nsIURI> uri;
> +    if (!report.mSourceFileURI.IsEmpty()) {
> +      nsresult rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);
> +      if (NS_FAILED(rv)) {
> +        MOZ_ALWAYS_SUCCEEDS(rv);

nit: I would put this before if NS_FAILED(rv).  Or if you want to keep it in the if-statement make it MOZ_ASSERT_UNREACHABLE().

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 68 • 8 years ago

Comment on attachment 8760796 [details] [diff] [review]
Bug 1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed

Review of attachment 8760796 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/Fetch.cpp
@@ +494,5 @@
> +      swm->FlushReportToAllClients(worker->WorkerName(), mReporter);
> +    }
> +  } else {
> +    MOZ_ASSERT(worker->GetDocument());
> +    mReporter->FlushConsoleReports(worker->GetDocument());

You're missing a few cases here:

1) Nested workers will have a parent that ultimately chains to a document, but GetDocument() here will return nullptr.  You probably need to walk this chain.

2) SharedWorkers will have multiple documents.  You probably want to use the same FlushConsoleReports() call on each document that SWM does.

I would recommend making a WorkerPrivate::FlushConsoleReports(nsIConsoleReporter* aReporter) that does the right thing depending on the worker type.  That way we can reuse this logic elsewhere.

::: dom/fetch/FetchDriver.cpp
@@ +672,5 @@
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }
> +
> +      rv = mSRIDataVerifier->Update(aCount, (uint8_t*)buffer.Elements());

This should be passing aRead instead of aCount.

@@ +676,5 @@
> +      rv = mSRIDataVerifier->Update(aCount, (uint8_t*)buffer.Elements());
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      rv = mPipeOutputStream->Write(reinterpret_cast<char*>(buffer.Elements()),
> +                                    aCount, &aWrite);

Same here.  aRead instead of aCount.

::: dom/fetch/Request.cpp
@@ +494,5 @@
> +    if (!integrity.IsEmpty()) {
> +      nsAutoCString method;
> +      request->GetMethod(method);
> +      NS_ConvertUTF8toUTF16 label(method);
> +      aRv.ThrowTypeError<MSG_INVALID_REQUEST_METHOD>(label);

This error message seems cut and paste?

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1259,5 @@
>      , mRequestCredentials(RequestCredentials::Same_origin)
>      , mContentPolicyType(nsIContentPolicy::TYPE_INVALID)
>      , mReferrer(kFETCH_CLIENT_REFERRER_STR)
>      , mReferrerPolicy(ReferrerPolicy::_empty)
> +    , mIntegrity(EmptyString())

Shouldn't we be pulling this from the nsIChannel so that we reflect the requests real integrity value?  We should have a test case that shows this as well.

	Tom Tung [:tt, :ttung] Assignee
	Comment 69 • 8 years ago

Hi Ben,

Thanks for the review and the feedback!

(In reply to Ben Kelly [:bkelly] from comment #66) 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +1472,5 @@
> > +    }
> > +
> > +    windows.AppendElement(doc->InnerWindowID());
> > +
> > +    aReporter->FlushConsoleReports(doc);
> 
> So this will flush the reports to the first document seen, free the reports,
> and then the remaining documents will not get any reports.  See:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/base/
> ConsoleReportCollector.cpp#48
> 
> You need to add an argument to FlushConsoleReports() that lets the caller
> avoid dropping the reports.  Something like:
> 
> enum class ReportAction {
>   Forget,
>   Save
> };
> 
> void
> FlushConsoleReports(nsIDocument* aDoc, ReportAction aAction =
> ReportAction::Forget);

Oh, I should notice the report must be clear once it was flushed. Sorry for that. 
I'll implement that.

(In reply to Ben Kelly [:bkelly] from comment #67)
> Comment on attachment 8760753 [details] [diff] [review]
> Bug 1187335 - P2-1 - Add an assertion as SourceURI fail and accept an empty
> SourceURI for ConsoleReportCollector.
> 
> Review of attachment 8760753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/ConsoleReportCollector.cpp
> @@ +56,5 @@
> >      nsCOMPtr<nsIURI> uri;
> > +    if (!report.mSourceFileURI.IsEmpty()) {
> > +      nsresult rv = NS_NewURI(getter_AddRefs(uri), report.mSourceFileURI);
> > +      if (NS_FAILED(rv)) {
> > +        MOZ_ALWAYS_SUCCEEDS(rv);
> 
> nit: I would put this before if NS_FAILED(rv).  Or if you want to keep it in
> the if-statement make it MOZ_ASSERT_UNREACHABLE().

Sure, I would like to put MOZ_ALWAYS_SUCCEEDS before the if condition.

(In reply to Ben Kelly [:bkelly] from comment #68)
> Comment on attachment 8760796 [details] [diff] [review]
> Bug 1187335 - P4 - Integrity fetch with SRI & add nsIConsoleReportCollector
> to show console report - fetch API changed
> 
> Review of attachment 8760796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fetch/Fetch.cpp
> @@ +494,5 @@
> > +      swm->FlushReportToAllClients(worker->WorkerName(), mReporter);
> > +    }
> > +  } else {
> > +    MOZ_ASSERT(worker->GetDocument());
> > +    mReporter->FlushConsoleReports(worker->GetDocument());
> 
> You're missing a few cases here:
> 
> 1) Nested workers will have a parent that ultimately chains to a document,
> but GetDocument() here will return nullptr.  You probably need to walk this
> chain.
> 
> 2) SharedWorkers will have multiple documents.  You probably want to use the
> same FlushConsoleReports() call on each document that SWM does.
> 
> I would recommend making a
> WorkerPrivate::FlushConsoleReports(nsIConsoleReporter* aReporter) that does
> the right thing depending on the worker type.  That way we can reuse this
> logic elsewhere.

Sorry for missing them. I'll try to implement them in next patch! 
BTW, should I create few tests for make sure that I do the behavior correctly? But, I have no idea how to do that. Could you show some existing tests for me or teach me how to write tests for ensuring I report to correct console for each kind of workers? 

> ::: dom/fetch/FetchDriver.cpp
> @@ +672,5 @@
> > +      if (NS_WARN_IF(NS_FAILED(rv))) {
> > +        return rv;
> > +      }
> > +
> > +      rv = mSRIDataVerifier->Update(aCount, (uint8_t*)buffer.Elements());
> 
> This should be passing aRead instead of aCount.
> @@ +676,5 @@
> > +      rv = mSRIDataVerifier->Update(aCount, (uint8_t*)buffer.Elements());
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      rv = mPipeOutputStream->Write(reinterpret_cast<char*>(buffer.Elements()),
> > +                                    aCount, &aWrite);
> 
> Same here.  aRead instead of aCount.

Yeah, sorry of that. I'll correct them. 

> 
> ::: dom/fetch/Request.cpp
> @@ +494,5 @@
> > +    if (!integrity.IsEmpty()) {
> > +      nsAutoCString method;
> > +      request->GetMethod(method);
> > +      NS_ConvertUTF8toUTF16 label(method);
> > +      aRv.ThrowTypeError<MSG_INVALID_REQUEST_METHOD>(label);
> 
> This error message seems cut and paste?

Yes, I thought it is kind of general type error message, but I was wrong. I'll create a new error message for empty integrity.
> 
> ::: dom/workers/ServiceWorkerPrivate.cpp
> @@ +1259,5 @@
> >      , mRequestCredentials(RequestCredentials::Same_origin)
> >      , mContentPolicyType(nsIContentPolicy::TYPE_INVALID)
> >      , mReferrer(kFETCH_CLIENT_REFERRER_STR)
> >      , mReferrerPolicy(ReferrerPolicy::_empty)
> > +    , mIntegrity(EmptyString())
> 
> Shouldn't we be pulling this from the nsIChannel so that we reflect the
> requests real integrity value?  We should have a test case that shows this
> as well.
Could you tell me more about this? I am not sure how to do that. Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 70 • 8 years ago

(In reply to Tom Tung [:tt][Dayoff: 6/9~6/10, PTO: 6/20] from comment #69)
> > > +    , mIntegrity(EmptyString())
> > 
> > Shouldn't we be pulling this from the nsIChannel so that we reflect the
> > requests real integrity value?  We should have a test case that shows this
> > as well.
> Could you tell me more about this? I am not sure how to do that. Thanks!

Oh, I see.  The integrity value is never passed through the channel.  The thing doing the integrity check has to live on top of the channel to and do the comparison manually itself.

So what we need to do is pass the integrity metadata string through the nsIChannel.  You can add a SetIntegrityMetadata() and GetIntegrityMetadata() in nsIHttpChannelInternal.  (Maybe intregrity applies to other schemes, but we only support SW interception on HTTP at the moment).

Then the code initiating the nsIChannel that will have its integrity checked must call SetIntegrityMetadata().  The ServiceWorker code would then call GetIntegrityMetadata() when creating a Request object from the intercepted channel.

To avoid missing the SetIntegrityMetadata() call, I would probably make the existing code use GetIntegrityMetadata() as well instead of looking at its local variable.

Does that make sense?

	Tom Tung [:tt, :ttung] Assignee
	Comment 71 • 8 years ago

Addressed the comment.

	Tom Tung [:tt, :ttung] Assignee
	Comment 73 • 8 years ago

Hi François,

Would you mind helping me to review the modifications for SRI API? Thanks! The reason that I did this patch is trying not to use nsIDocument to reporting to console since workers' Document is loading and it's not suitable for reporting to console.
This patch is mainly done:
   1. Add a function for reporting to console in SRI API.
   2. Using LoadTainting to decide whehter is CORS or not.
   3. Removing passing all nsIDocument object for SRI API.
   4. Let caller class(Loader/nsScriptLoader) hold a nsIConsoleReportCollector pointer, create it as the class init, flush the report at suitable timing and pass it into SRI when call SRI's API.

	Tom Tung [:tt, :ttung] Assignee
	Comment 74 • 8 years ago

Hi François,

Could you also take a look for this patch? Thanks! 
This patch is addressing the comment #30 for correcting the SRI tests.

	Tom Tung [:tt, :ttung] Assignee
	Comment 75 • 8 years ago

rebase the patch into current m-c

	Tom Tung [:tt, :ttung] Assignee
	Comment 76 • 8 years ago

Hi Ben,

I not sure about whether the direction of this patch is correct or not. Thus, I send a feedback request to you for checking. Could you give me some feedback? Thanks!

In this patch, I addressed the comments(add an enum and a clearing function into nsConsoleReportCollector, and add a flushing reports for sharedworekr). I list the problems which I'm not sure below inline. 

1. Should I modify nsIConsoleReportCollector.h?
To address comment #66 (add enum Action), I also implement a function to clear the pending reports. I implement them into nsIConsoleReportCollector.h and do the corresponding change to its derived classes. 

2. Does preventDefault have any effect on ConsoleReportCollector? There are few code to deal with it when sharedworker calls BroadcastErrorToSharedWorkers(..)  as [1]. 
I do not deal with preventDefault case in this patch. If it is necessary, I'll implement it in next patch.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp?q=WorkerPrivate.cpp&redirect_type=direct#3351-3378
Besides, I also add a function to flush report for sharedworker

	Tom Tung [:tt, :ttung] Assignee
	Comment 77 • 8 years ago

Could you help me to review this patch, Ben? Thanks!

In this patch, I addressed the comment #68 except the test case for pulling integrity form nsIChannel. I'll do it later.

	Tom Tung [:tt, :ttung] Assignee
	Comment 78 • 8 years ago

I create this patch for implementing request.integrity into Cache API. Do you mind helping me to review it also, Ben? Thanks!

BTW, should I add a test for it at test_migration.js?

	Tom Tung [:tt, :ttung] Assignee
	Comment 79 • 8 years ago

I add a wpt test for testing request.integrity to shared worker. Could you help me to review this patch, Ben? Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 80 • 8 years ago

Comment on attachment 8765368 [details] [diff] [review]
Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and SharedWorker.

Review of attachment 8765368 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/console/nsIConsoleReportCollector.h
@@ +93,5 @@
>    //                ownership of our currently console reports.
>    virtual void
>    FlushConsoleReports(nsIConsoleReportCollector* aCollector) = 0;
> +
> +  // Clear all pending reports. Main thread only.

Why is this documented as main thread only?  It seems it could be safely called from any thread.

::: dom/workers/WorkerPrivate.cpp
@@ +3604,5 @@
> +
> +  if (sharedWorkers.IsEmpty()) {
> +    aReporter->FlushConsoleReports((nsIDocument*)nullptr);
> +    return;
> +  }

Lets just let it fall through the loops below and trigger the final flush to null document.  I don't think this short circuit really buys us much here.

@@ +3608,5 @@
> +  }
> +
> +  // First fire the error event at all SharedWorker objects. This may include
> +  // multiple objects in a single window as well as objects in different
> +  // windows.

This comment seems wrong.  Cut&paste mistake?

@@ +3626,5 @@
> +  // If there are no windows to consider further then we're done.
> +  if (windowActions.IsEmpty()) {
> +    aReporter->FlushConsoleReports((nsIDocument*)nullptr);
> +    return;
> +  }

Same here.  Lets remove the short circuit to simplify this method.

@@ +3635,5 @@
> +  for (uint32_t index = 0; index < windowActions.Length(); index++) {
> +    WindowAction& windowAction = windowActions[index];
> +
> +    // If there is no window or the script already called preventDefault then
> +    // skip this window.

Is this comment about preventDefault also stale from a copy/paste?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 81 • 8 years ago

Comment on attachment 8765370 [details] [diff] [review]
Bug 1187335 - P4-part1 - Integrate fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed

Review of attachment 8765370 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/fetch/Fetch.cpp
@@ +463,5 @@
> +  } else if (worker->IsSharedWorker()) {
> +    worker->FlushReportToShraredWorkers(mReporter);
> +  } else {
> +    mReporter->FlushConsoleReports(worker->GetDocument());
> +  }

nit: Can you break up this if/else chain into if-statements with early return?  So:

if (IsServiceWorker) {
  // flush to service worker
  return;
}

if (IsSharedWorker) {
  // flush to shared worker
  return;
}

// flush to dedicated worker

::: dom/fetch/FetchDriver.cpp
@@ +318,5 @@
>      mRequest->MaybeSkipCacheIfPerformingRevalidation();
>      internalChan->SetFetchCacheMode(static_cast<uint32_t>(mRequest->GetCacheMode()));
> +    nsString integrity;
> +    mRequest->GetIntegrity(integrity);
> +    internalChan->SetIntegrityMetadata(integrity);

See my note about making InternalRequest::GetIntegrity() just return const nsString& to make this a bit nicer here.

::: dom/fetch/FetchDriver.h
@@ +97,5 @@
>    nsCOMPtr<nsIOutputStream> mPipeOutputStream;
>    RefPtr<FetchDriverObserver> mObserver;
>    nsCOMPtr<nsIDocument> mDocument;
> +  nsAutoPtr<mozilla::dom::SRICheckDataVerifier> mSRIDataVerifier;
> +  mozilla::dom::SRIMetadata mSRIMetadata;

nit: I don't think you need the mozilla::dom namespace here.  FetchDriver is already in the mozilla::dom namespace itself.

::: dom/fetch/InternalRequest.h
@@ +99,5 @@
>      , mCredentialsMode(RequestCredentials::Omit)
>      , mResponseTainting(LoadTainting::Basic)
>      , mCacheMode(RequestCache::Default)
>      , mRedirectMode(RequestRedirect::Follow)
> +    , mIntegrity(EmptyString())

This isn't necessary.  nsString already defaults to EmptyString().

@@ +349,5 @@
>      mRedirectMode = aRedirectMode;
>    }
>  
> +  void
> +  GetIntegrity(nsAString& aIntegrity) const

This might be a little nicer if it was:

const nsString& GetIntegrity() const;

::: dom/fetch/Request.cpp
@@ +502,5 @@
> +    if (!integrity.IsEmpty()) {
> +      nsAutoCString method;
> +      request->GetMethod(method);
> +      NS_ConvertUTF8toUTF16 label(method);
> +      aRv.ThrowTypeError<MSG_INVALID_REQUEST_INTEGRITY>(label);

This doesn't seem quite right.  It will produce a message like:

  Invalid request integrity POST.

It seems we should either be setting the integrity value as the message label or the message should mention the method is wrong.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +289,5 @@
> +
> +    /**
> +     * Set to indicate Request.integrity.
> +     */
> +    attribute AString IntegrityMetadata;

This patch sets the integrity metadata when its added via fetch(), but we also need to set this attribute when SRI is being used for scripts, stylesheets, etc.  Please set it in those cases as well.

You can do this in another patch, though.  Might be easier than reviewing this whole patch again.

nit: We normally make idl attributes begin with lower cases letters.  So integrityMetadata.  This then gets automatically converted to uppercase for c++ accessors to it.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 82 • 8 years ago

Comment on attachment 8765372 [details] [diff] [review]
Bug 1187335 - P4-part2 - Integrate cache with request.integrity

Review of attachment 8765372 [details] [diff] [review]:
-----------------------------------------------------------------

Did anything change in this patch?  I feel like I reviewed it before.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 83 • 8 years ago

Can you add some test cases here that verify the service worker sees the correct value in event.request.integrity?

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event.https.html

That test is mostly working with fetch(), but it would be nice to also have a case for stylesheets and scripts using SRI.  I suppose those could go in that same test or this test might work:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-request-resources.https.html

	Tom Tung [:tt, :ttung] Assignee
	Comment 84 • 8 years ago

Hi Ben,

Thanks for the reviews and feedback! Your comments help me a lot! I will address them in next patch.

(In reply to Ben Kelly [:bkelly] from comment #80)
> Comment on attachment 8765368 [details] [diff] [review]
> Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and
> SharedWorker.
> 
> Review of attachment 8765368 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/console/nsIConsoleReportCollector.h
> @@ +93,5 @@
> >    //                ownership of our currently console reports.
> >    virtual void
> >    FlushConsoleReports(nsIConsoleReportCollector* aCollector) = 0;
> > +
> > +  // Clear all pending reports. Main thread only.
> 
> Why is this documented as main thread only?  It seems it could be safely
> called from any thread.

Oh, it shouldn't. I commented that because I thought it should be called at the same thread as |FlushConsoleReports(document)|. 

> ::: dom/workers/WorkerPrivate.cpp
> @@ +3604,5 @@
> > +
> > +  if (sharedWorkers.IsEmpty()) {
> > +    aReporter->FlushConsoleReports((nsIDocument*)nullptr);
> > +    return;
> > +  }
> 
> Lets just let it fall through the loops below and trigger the final flush to
> null document.  I don't think this short circuit really buys us much here.

Sure. 
 
> @@ +3608,5 @@
> > +  }
> > +
> > +  // First fire the error event at all SharedWorker objects. This may include
> > +  // multiple objects in a single window as well as objects in different
> > +  // windows.
> 
> This comment seems wrong.  Cut&paste mistake?

Sorry for that. I just want to write a simple version of flushing report for sharedworker to ask for the direction about this patch and make sure I don't need to worry about preventDefault. I should be more careful about these before sending the requests.

(In reply to Ben Kelly [:bkelly] from comment #81)
> Comment on attachment 8765370 [details] [diff] [review]
> Bug 1187335 - P4-part1 - Integrate fetch with SRI & add
> nsIConsoleReportCollector to show console report - fetch API changed
> 
> Review of attachment 8765370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: dom/fetch/Request.cpp
> @@ +502,5 @@
> > +    if (!integrity.IsEmpty()) {
> > +      nsAutoCString method;
> > +      request->GetMethod(method);
> > +      NS_ConvertUTF8toUTF16 label(method);
> > +      aRv.ThrowTypeError<MSG_INVALID_REQUEST_INTEGRITY>(label);
> 
> This doesn't seem quite right.  It will produce a message like:
> 
>   Invalid request integrity POST.
> 
> It seems we should either be setting the integrity value as the message
> label or the message should mention the method is wrong.

Sure, I prefer to pass the integrity value as the message label one.

> ::: netwerk/protocol/http/nsIHttpChannelInternal.idl
> @@ +289,5 @@
> > +
> > +    /**
> > +     * Set to indicate Request.integrity.
> > +     */
> > +    attribute AString IntegrityMetadata;
> 
> This patch sets the integrity metadata when its added via fetch(), but we
> also need to set this attribute when SRI is being used for scripts,
> stylesheets, etc.  Please set it in those cases as well.
> 
> You can do this in another patch, though.  Might be easier than reviewing
> this whole patch again.
>
> nit: We normally make idl attributes begin with lower cases letters.  So
> integrityMetadata.  This then gets automatically converted to uppercase for
> c++ accessors to it.

Ok! I'll create another patch to implement that!

(In reply to Ben Kelly [:bkelly] from comment #82)
> Comment on attachment 8765372 [details] [diff] [review]
> Bug 1187335 - P4-part2 - Integrate cache with request.integrity
> 
> Review of attachment 8765372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did anything change in this patch?  I feel like I reviewed it before.

No, I implement them after the fetch api part is almost done.

	Tom Tung [:tt, :ttung] Assignee
	Comment 85 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #83)
> Can you add some test cases here that verify the service worker sees the
> correct value in event.request.integrity?
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> service-workers/service-worker/fetch-event.https.html
> 
> That test is mostly working with fetch(), but it would be nice to also have
> a case for stylesheets and scripts using SRI.  I suppose those could go in
> that same test or this test might work:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> service-workers/service-worker/fetch-request-resources.https.html

Sure, I'll write a patch for adding test cases for this bug.

	Tom Tung [:tt, :ttung] Assignee
	Comment 86 • 8 years ago

Hi Ben,

Sorry for the late reply. This patch address the comments. It mainly to implement to flush report to all service worker' clients and to all shared worker. 

Could you help me to review this patch? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 87 • 8 years ago

I addressed the comments, but there are few of them that I want to ensure that they are correct. Could you help me to review this patch again?

note: For error message log (Errors,msg), I changed my mind. I think that it's better to tell users realize that request.integrity should be empty as no-cors mode.

	Tom Tung [:tt, :ttung] Assignee
	Comment 88 • 8 years ago

Rebase this patch since we changed the method to get integrity.

	Tom Tung [:tt, :ttung] Assignee
	Comment 91 • 8 years ago

Comment on attachment 8767126 [details] [diff] [review]
Bug 1187335 - P5-part3 - Add test for SRI in service worker

This patch is to add a test for ensuring that service worker get correct value of request.integrity. Could you help me to review this one? Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 92 • 8 years ago

Comment on attachment 8767122 [details] [diff] [review]
Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and SharedWorker.

Review of attachment 8767122 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.  Thanks!

::: dom/workers/ServiceWorkerManager.cpp
@@ +1513,5 @@
> +
> +    windows.AppendElement(doc->InnerWindowID());
> +
> +    aReporter->FlushConsoleReports(doc,
> +                                   ConsoleReportCollector::ReportAction::Save);

nit: I think you could shorten this to aReporter::ReportAction::Save.  Here and elsewhere in the patch.

::: dom/workers/ServiceWorkerManager.h
@@ +189,5 @@
>                       uint32_t aColumnNumber,
>                       uint32_t aFlags);
>  
> +  void
> +  FlushReportToAllClients(const nsCString& aScope,

nit: s/FlushReport/FlushReports/g

::: dom/workers/WorkerPrivate.cpp
@@ +3592,5 @@
>  
>  template <class Derived>
> +void
> +WorkerPrivateParent<Derived>::FlushReportToShraredWorkers(
> +                                           nsIConsoleReportCollector* aReporter)

Have you at least tested this function locally to see if it works for a SharedWorker attached to two different windows?  Having an automated test would be ideal, but I just wanted to make sure we at least verified with local testing.

@@ +3608,5 @@
> +    // May be null.
> +    nsPIDOMWindowInner* window = sharedWorker->GetOwner();
> +
> +    // Add the owning window to our list so that we will flush the reports later.
> +    if (!windowActions.Contains(window)) {

If window is nullptr, we probably don't want to add it to windowActions, right?

@@ +3613,5 @@
> +      windowActions.AppendElement(WindowAction(window));
> +    }
> +  }
> +
> +  bool reportErrorToConsole = false;

nit: Maybe clarify this variable bit by indicating its a fallback to the browser console.  Maybe `reportErrorToBrowserConsole`.

@@ +3621,5 @@
> +    WindowAction& windowAction = windowActions[index];
> +
> +    if (!windowAction.mWindow) {
> +      continue;
> +    }

If we just avoid adding nullptr's to windowActions above, then we can remove this check.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 93 • 8 years ago

Comment on attachment 8767123 [details] [diff] [review]
Bug 1187335 - P4-part1 - Integrate fetch with SRI & add nsIConsoleReportCollector to show console report - fetch API changed.

Review of attachment 8767123 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/bindings/Errors.msg
@@ +58,5 @@
>  MSG_DEF(MSG_INVALID_HEADER_VALUE, 1, JSEXN_TYPEERR, "{0} is an invalid header value.")
>  MSG_DEF(MSG_INVALID_HEADER_SEQUENCE, 0, JSEXN_TYPEERR, "Headers require name/value tuples when being initialized by a sequence.")
>  MSG_DEF(MSG_PERMISSION_DENIED_TO_PASS_ARG, 1, JSEXN_TYPEERR, "Permission denied to pass cross-origin object as {0}.")
>  MSG_DEF(MSG_MISSING_REQUIRED_DICTIONARY_MEMBER, 1, JSEXN_TYPEERR, "Missing required {0}.")
> +MSG_DEF(MSG_REQUEST_INTEGRITY_METADATA_NOT_EMPTY, 0, JSEXN_TYPEERR, "Request integrity metadata should be a empty string when no-cors mode.")

s/a empty/an empty/g
s/when no-cors/when in no-cors/g

::: dom/fetch/Fetch.cpp
@@ +466,5 @@
> +  }
> +
> +  if (worker->IsSharedWorker()) {
> +    // Flush to shared worker
> +    worker->FlushReportToShraredWorkers(mReporter);

s/Shrared/Shared/g

Sorry I missed this typo in the other patch.

::: dom/fetch/InternalRequest.h
@@ +356,5 @@
> +
> +  void
> +  SetIntegrity(const nsAString& aIntegrity)
> +  {
> +    mIntegrity.Assign(aIntegrity);

Is there every any reason the integrity value could change?  Can we MOZ_ASSERT(mIntegrity.IsEmpty()) here?

::: dom/fetch/Request.h
@@ +87,5 @@
> +    aIntegrity = mRequest->GetIntegrity();
> +  }
> +
> +  void
> +  SetIntegrity(const nsAString& aIntegrity)

Is this actually called from anywhere?  The binding is read-only and our internal code can use InternalRequest::SetIntegrity().  Can we remove this?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 94 • 8 years ago

Comment on attachment 8767126 [details] [diff] [review]
Bug 1187335 - P5-part3 - Add test for SRI in service worker

Review of attachment 8767126 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  I think we still need tests to verify event.request.integrity is set properly when a service worker intercepts <script integrity="foo"> and <link rel="stylesheet" integrity="foo">.

	Tom Tung [:tt, :ttung] Assignee
	Comment 95 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #93)
Hi Ben, 

Thanks for your time and the review! I address comments except one and I leave my question below inline. 

> Comment on attachment 8767123 [details] [diff] [review]
> Bug 1187335 - P4-part1 - Integrate fetch with SRI & add
> nsIConsoleReportCollector to show console report - fetch API changed.
> 
> Review of attachment 8767123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: dom/fetch/InternalRequest.h
> @@ +356,5 @@
> > +
> > +  void
> > +  SetIntegrity(const nsAString& aIntegrity)
> > +  {
> > +    mIntegrity.Assign(aIntegrity);
> 
> Is there every any reason the integrity value could change?  Can we
> MOZ_ASSERT(mIntegrity.IsEmpty()) here?

I did similar thing (like [1]) here due to step 24 [2]. Hence, the integrity could be changed. I think it could be passed a null string?  If it can, we cannot put this assert here.

[1] http://searchfox.org/mozilla-central/source/dom/fetch/Request.cpp#441-443
[2] https://fetch.spec.whatwg.org/#dom-request

> ::: dom/fetch/Request.h
> @@ +87,5 @@
> > +    aIntegrity = mRequest->GetIntegrity();
> > +  }
> > +
> > +  void
> > +  SetIntegrity(const nsAString& aIntegrity)
> 
> Is this actually called from anywhere?  The binding is read-only and our
> internal code can use InternalRequest::SetIntegrity().  Can we remove this?

Yes, it should not be existed for a read-only attribute. Sorry for that.

	Tom Tung [:tt, :ttung] Assignee
	Comment 96 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #92)
> Comment on attachment 8767122 [details] [diff] [review]
> Bug 1187335 - P1 - Add FlushReportToAllClient() to ServiceWorkerManager and
> SharedWorker.
> 
> Review of attachment 8767122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.  Thanks! 

Hi Ben, 

Thanks again!

> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +1513,5 @@
> > +
> > +    windows.AppendElement(doc->InnerWindowID());
> > +
> > +    aReporter->FlushConsoleReports(doc,
> > +                                   ConsoleReportCollector::ReportAction::Save);
> 
> nit: I think you could shorten this to aReporter::ReportAction::Save.  Here
> and elsewhere in the patch.

I'm not sure what you meant. I will get build failed if I do so. I change them into |nsIConsoleReportCollector::ReportAction::Save| in this patch.

> ::: dom/workers/WorkerPrivate.cpp
> @@ +3592,5 @@
> >  
> >  template <class Derived>
> > +void
> > +WorkerPrivateParent<Derived>::FlushReportToShraredWorkers(
> > +                                           nsIConsoleReportCollector* aReporter)
> 
> Have you at least tested this function locally to see if it works for a
> SharedWorker attached to two different windows?  Having an automated test
> would be ideal, but I just wanted to make sure we at least verified with
> local testing.

I did it and I could get expected results (flush reports to each of documents attaching to the sharedworker) when opening the browser console, but hit an assertion when opening the web console. 
Furthermore, I also found another testcase(a sharedworker with windows) will also hit this assertion. Thus, I file Bug 1284420 for that.

> @@ +3608,5 @@
> > +    // May be null.
> > +    nsPIDOMWindowInner* window = sharedWorker->GetOwner();
> > +
> > +    // Add the owning window to our list so that we will flush the reports later.
> > +    if (!windowActions.Contains(window)) {
> 
> If window is nullptr, we probably don't want to add it to windowActions,
> right?
Yes, I address that in this patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 97 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #94)
> Looks good!  I think we still need tests to verify event.request.integrity
> is set properly when a service worker intercepts <script integrity="foo">
> and <link rel="stylesheet" integrity="foo">.

There is a problem when I try to store script's and stylesheet's integrity into ServiceWorker. The problem is they store their integrity metadata in SRIMetadata and I store it in nsString for InternalRequst and ServiceWorker. The reason is it may need to be called by GetIntegrity() in Request and it's easier to test.

Currently, I have two simple solution for this problem. One is either to modify storing type in both script and stylesheet from SRIMetadata to nsString or modify storing type in InternalRequest and ServiceWorker. The other is to add a helper function (maybe ToString) in SRIMetadata. But I am not sure about which one is better.

How do you think about this, François? Any suggestion?

	Tom Tung [:tt, :ttung] Assignee
	Comment 98 • 8 years ago

Hi Ben,

This is a patch for sharedWokrer local test and it need to based on patch P5-part2. As I mention in comment #96, I can get error reports in browser console but get an assertion failure in web console. 

I am not sure about whether the test is as your expected. Could you take a look at this patch? Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 99 • 8 years ago

(In reply to Tom Tung [:tt] from comment #95)
> > > +  void
> > > +  SetIntegrity(const nsAString& aIntegrity)
> > > +  {
> > > +    mIntegrity.Assign(aIntegrity);
> > 
> > Is there every any reason the integrity value could change?  Can we
> > MOZ_ASSERT(mIntegrity.IsEmpty()) here?
> 
> I did similar thing (like [1]) here due to step 24 [2]. Hence, the integrity
> could be changed. I think it could be passed a null string?  If it can, we
> cannot put this assert here.
> 
> [1] http://searchfox.org/mozilla-central/source/dom/fetch/Request.cpp#441-443
> [2] https://fetch.spec.whatwg.org/#dom-request

I think it should be safe to put the MOZ_ASSERT(mIntegrity.IsEmpty()) here.  Generally this SetIntegrity() method should only be used when initializing a new InternalRequest.  For now its an error to change the integrity value once its set.

(In reply to Tom Tung [:tt] from comment #97)
> (In reply to Ben Kelly [:bkelly] from comment #94)
> > Looks good!  I think we still need tests to verify event.request.integrity
> > is set properly when a service worker intercepts <script integrity="foo">
> > and <link rel="stylesheet" integrity="foo">.
> 
> There is a problem when I try to store script's and stylesheet's integrity
> into ServiceWorker. The problem is they store their integrity metadata in
> SRIMetadata and I store it in nsString for InternalRequst and ServiceWorker.
> The reason is it may need to be called by GetIntegrity() in Request and it's
> easier to test.

I think you can just use SRIMetadata::GetHash() and concatenate each hash together separated by spaces.  This is the format specified in the SRI spec and what the SRIMetadata constructor expects:

https://dxr.mozilla.org/mozilla-central/source/dom/security/SRIMetadata.cpp#42

As you said, this could be added as a simple ToString() on SRIMetadata as well.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 100 • 8 years ago

Comment on attachment 8767893 [details] [diff] [review]
simple_local_test.patch

Review of attachment 8767893 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this is useful for checking how we handle a SharedWorker attached to two windows, but its not quite what I was going for.

It would be really nice if we could validate the logging actually happens.  Do to this you would probably need a browser-chrome test like this:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/test/test_console_serviceworker.html

Also, can you check the console output locally in a non-debug build?  Maybe the logging will work even though we are hitting an unexpected assertion if you turn assertions off.

	Tom Tung [:tt, :ttung] Assignee
	Comment 101 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #99)
> I think it should be safe to put the MOZ_ASSERT(mIntegrity.IsEmpty()) here. 
> Generally this SetIntegrity() method should only be used when initializing a
> new InternalRequest.  For now its an error to change the integrity value
> once its set.

Oh, I see. I misunderstood that you ask me to put the MOZ_ASSERT('a'Integrity.IsEmpty()), and I was wrong.
Sure, it's ok to put the MOZ_ASSERT(mIntegrity.IsEmpty()) here. I'll apply that in next patch.

> I think you can just use SRIMetadata::GetHash() and concatenate each hash
> together separated by spaces.  This is the format specified in the SRI spec
> and what the SRIMetadata constructor expects:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/security/SRIMetadata.
> cpp#42
> 
> As you said, this could be added as a simple ToString() on SRIMetadata as
> well.

I just find that SRIMetadata may modify the original integrity metadata when its value is invalid. For example, a integrity metadata with a question mark will be truncated when store into class SRIMetadata [1]. Do we need to take care about these cases (e.g. [2])? 

If we need, maybe store them in nsString at class Loader/ScriptLoader is better? If it's not necessary, ToString() is enough since we just need to store the valid value.  

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/SRIMetadata.cpp?q=SRIMetadata.cpp&redirect_type=direct#48-60
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/sri/iframe_script_sameorigin.html#133

	Tom Tung [:tt, :ttung] Assignee
	Comment 102 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #100)
> Comment on attachment 8767893 [details] [diff] [review]
> simple_local_test.patch
> 
> Review of attachment 8767893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this is useful for checking how we handle a SharedWorker attached to
> two windows, but its not quite what I was going for.
> 
> It would be really nice if we could validate the logging actually happens. 
> Do to this you would probably need a browser-chrome test like this:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/
> test/test_console_serviceworker.html

Sure, I'll do that latter.
 
> Also, can you check the console output locally in a non-debug build?  Maybe
> the logging will work even though we are hitting an unexpected assertion if
> you turn assertions off.

Sure, I have checked the console output in a non-debug build and each of messages is presented in twice(one for whole html, the other for iframe). I think the behavior is shown as expected?

	Tom Tung [:tt, :ttung] Assignee
	Comment 103 • 8 years ago

(In reply to Tom Tung [:tt] from comment #101)
> I just find that SRIMetadata may modify the original integrity metadata when
> its value is invalid. For example, a integrity metadata with a question mark
> will be truncated when store into class SRIMetadata [1]. Do we need to take
> care about these cases (e.g. [2])? 

I found that I can get the entire integrity attribute for nsIContent object. I'm working on this way and the script part is done. For stylesheet, I still need to find out the current Element for getting nsIContent object.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 104 • 8 years ago

(In reply to Tom Tung [:tt] from comment #103)
> (In reply to Tom Tung [:tt] from comment #101)
> > I just find that SRIMetadata may modify the original integrity metadata when
> > its value is invalid. For example, a integrity metadata with a question mark
> > will be truncated when store into class SRIMetadata [1]. Do we need to take
> > care about these cases (e.g. [2])? 
> 
> I found that I can get the entire integrity attribute for nsIContent object.
> I'm working on this way and the script part is done. For stylesheet, I still
> need to find out the current Element for getting nsIContent object.

I think we only need to set the integrity value on the nsIChannel before it does its AsyncOpen() or AsyncOpen2().  This should happen before the script/stylesheet code checks to see if the integrity matches.  So before OnStartRequest().

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 105 • 8 years ago

Also, Andrew is adding a new console reporting test in bug 1267473.  It seems like maybe you could copy that for a console test in SRI.  Its much simpler than the devtools test.

	François Marier [:francois]
	Comment 106 • 8 years ago

(In reply to Tom Tung [:tt] from comment #101)
> (In reply to Ben Kelly [:bkelly] from comment #99)
> > I think you can just use SRIMetadata::GetHash() and concatenate each hash
> > together separated by spaces.  This is the format specified in the SRI spec
> > and what the SRIMetadata constructor expects:
> > 
> > https://dxr.mozilla.org/mozilla-central/source/dom/security/SRIMetadata.
> > cpp#42
> > 
> > As you said, this could be added as a simple ToString() on SRIMetadata as
> > well.
> 
> I just find that SRIMetadata may modify the original integrity metadata when
> its value is invalid. For example, a integrity metadata with a question mark
> will be truncated when store into class SRIMetadata [1]. Do we need to take
> care about these cases (e.g. [2])? 
> 
> If we need, maybe store them in nsString at class Loader/ScriptLoader is
> better? If it's not necessary, ToString() is enough since we just need to
> store the valid value.  

The invalid parts need to be ignored (as per the spec) so I think you can just leave them out. If these parts ever become valid, we'll change SRIMetadata to stop discarding them.

	François Marier [:francois]
	Comment 107 • 8 years ago

Comment on attachment 8763005 [details] [diff] [review]
Bug 1187335 - P2-part1 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not. r=bkelly.

Review of attachment 8763005 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good, I just pointed out a few minor things.

::: dom/security/SRICheck.cpp
@@ +17,3 @@
>  #include "nsIProtocolHandler.h"
>  #include "nsIScriptError.h"
>  #include "nsIScriptSecurityManager.h"

Do we still need this include now that the Same-Origin check no longers uses the security manager?

@@ +31,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +static void
> +ReportToConsole(uint32_t aErrorFlags, const nsACString& aCategory,

The category is always "Sub-resource integrity", so we may as well remove that parameter and hard-code that string into the function.

@@ +33,5 @@
>  
> +static void
> +ReportToConsole(uint32_t aErrorFlags, const nsACString& aCategory,
> +                nsContentUtils::PropertiesFile aFile, const char *aMessageName,
> +                const nsACString& aParams, nsIConsoleReportCollector* aReporter)

While we don't currently pass in more than one parameter to nsContentUtils::ReportToConsole(), it's possible we'll do that in the future if we want to improve the error messages.

So I would prefer if "aParams" were an array of strings.

@@ +348,5 @@
>    nsresult rv = Finish();
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
> +  LoadTainting tainting = loadInfo->GetTainting();

Please add a NULL check before this line.

NS_ENSURE_TRUE(loadInfo, <some appropriate NS_ERROR_ code>);

::: dom/security/SRICheck.h
@@ +8,3 @@
>  #define mozilla_dom_SRICheck_h
>  
>  #include "mozilla/CORSMode.h"

Do we still need this include?

	Tom Tung [:tt, :ttung] Assignee
	Comment 108 • 8 years ago

(In reply to François Marier [:francois] from comment #107)
Hi  François,

  Thanks for your feedback! I addressed them all and rebased the patch to the newest inbound. Could you help me to review this patch, again? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 109 • 8 years ago

Rebased the patch since there are few changes in ServiceWorkerManager.

	Tom Tung [:tt, :ttung] Assignee
	Comment 110 • 8 years ago

Hi Ben,

This patch is to add the integrity metadata's script/css loader into serviceWorker via HttpChannelInternal. Could you help me to review this patch? Thanks! 

Note:
  1. The integrity metadata is get from SRIMetadata. 
  2. Due to 1, the value of integrity metadata is processed by SRICheck::IntegrityMetadata(...) and the test will show the differences between whole integrity metadata and processed metadata.
  3. Since 1 (which is different from the patch for Fetch API), I will change the implementation in patch for Fetch API if this patch gets r+.

	Tom Tung [:tt, :ttung] Assignee
	Comment 111 • 8 years ago

(In reply to François Marier [:francois] from comment #106)
> The invalid parts need to be ignored (as per the spec) so I think you can
> just leave them out. If these parts ever become valid, we'll change
> SRIMetadata to stop discarding them.

Thanks for the comment and sorry for missing that. 
I decided to utilized the value stored in SRIMetadata and covert it back to nsString in order to store in HTTPChannelInternal. So, I added a ToString() function in that patch(attachment 8769740 [details] [diff] [review]).

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 112 • 8 years ago

Comment on attachment 8769740 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

Review of attachment 8769740 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I think we could refactor the ToString() a bit cleaner.

::: dom/security/SRIMetadata.cpp
@@ +171,5 @@
> +  MOZ_ASSERT(aString.IsEmpty()); // caller should pass empty string
> +  aString.Truncate();
> +
> +  if (!mAlgorithm.IsEmpty()) {
> +    aString.Assign(NS_ConvertUTF8toUTF16(mAlgorithm) + NS_LITERAL_STRING("-"));

If all the internal representations are in nsCString, then I suggest just making ToString() write to an nsACString&.  The caller can convert to UTF16 if they need to.

@@ +174,5 @@
> +  if (!mAlgorithm.IsEmpty()) {
> +    aString.Assign(NS_ConvertUTF8toUTF16(mAlgorithm) + NS_LITERAL_STRING("-"));
> +  }
> +
> +  if (mHashes.Length() == 0) {

So if hashes is zero we might append the "algorithm-" with nothing after it?  That seems wrong.

Can we instead make a helper routine like:

AppendHashString(nsACString& aString)
{
  // append alorithm if necessary
  // append hash
}

Then you can just loop over mHashes and call AppendHashString() on each one.

::: testing/web-platform/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +61,5 @@
>      };
>    return frame.contentWindow.load_font(actual_url);
>  }
>  
> +function script_integrity_test(frame, url, integrity, expexted_integrity) {

s/expexted/expected/g

::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-request-resources-iframe.https.html
@@ +50,5 @@
> +function load_script_with_integrity(url, integrity) {
> +  var script = document.createElement('script');
> +  script.src = url;
> +  if (integrity != '') {
> +    script.integrity = integrity;

Does it matter if you set an empty string integrity value?

	Tom Tung [:tt, :ttung] Assignee
	Comment 113 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #112)
Hi Ben,

Thanks for the feedback! 

> Comment on attachment 8769740 [details] [diff] [review]
> Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to
> serviceWorker.
> 
> Review of attachment 8769740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I think we could refactor the ToString() a bit cleaner.
> 
> ::: dom/security/SRIMetadata.cpp
> @@ +171,5 @@
> > +  MOZ_ASSERT(aString.IsEmpty()); // caller should pass empty string
> > +  aString.Truncate();
> > +
> > +  if (!mAlgorithm.IsEmpty()) {
> > +    aString.Assign(NS_ConvertUTF8toUTF16(mAlgorithm) + NS_LITERAL_STRING("-"));
> 
> If all the internal representations are in nsCString, then I suggest just
> making ToString() write to an nsACString&.  The caller can convert to UTF16
> if they need to.

Sure. It can reduce lots of string conversions.

> @@ +174,5 @@
> > +  if (!mAlgorithm.IsEmpty()) {
> > +    aString.Assign(NS_ConvertUTF8toUTF16(mAlgorithm) + NS_LITERAL_STRING("-"));
> > +  }
> > +
> > +  if (mHashes.Length() == 0) {
> 
> So if hashes is zero we might append the "algorithm-" with nothing after it?
> That seems wrong.

I check the mAlgorithm at first so it will happen only when we have an algorithm without any hash value. Here is existing example for that [1].

If the integrity metadata is an empty string, it will get empty integrity here rather than "algo-".

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/sri/iframe_script_sameorigin.html#195 

> Can we instead make a helper routine like:
> 
> AppendHashString(nsACString& aString)
> {
>   // append alorithm if necessary
>   // append hash
> }
> 
> Then you can just loop over mHashes and call AppendHashString() on each one.

Sure.

> :::
> testing/web-platform/tests/service-workers/service-worker/fetch-request-
> resources.https.html
> @@ +61,5 @@
> >      };
> >    return frame.contentWindow.load_font(actual_url);
> >  }
> >  
> > +function script_integrity_test(frame, url, integrity, expexted_integrity) {
> 
> s/expexted/expected/g

I just find out that we have same mistakes in other function. I'll correct them in next patch.
 
> :::
> testing/web-platform/tests/service-workers/service-worker/resources/fetch-
> request-resources-iframe.https.html
> @@ +50,5 @@
> > +function load_script_with_integrity(url, integrity) {
> > +  var script = document.createElement('script');
> > +  script.src = url;
> > +  if (integrity != '') {
> > +    script.integrity = integrity;
> 
> Does it matter if you set an empty string integrity value?

Nope, I'll remove the if condition to be more clearly.

	Tom Tung [:tt, :ttung] Assignee
	Comment 114 • 8 years ago

I addressed the comment expect few of them and I put my reason in comment #113. Would you mind reviewing this patch, Ben? Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 115 • 8 years ago

Comment on attachment 8770374 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

Review of attachment 8770374 [details] [diff] [review]:
-----------------------------------------------------------------

So, the "algo-" thing made me look at the spec more closely.  I don't think this approach is correct any more.  Sorry for sending you down the wrong path here!

::: dom/security/SRIMetadata.cpp
@@ +179,5 @@
> +  aString.Append(mHashes[aIndex]);
> +}
> +
> +void
> +SRIMetadata::ToString(nsACString& aString) const

Actually, looking at the spec closer I think this is the wrong approach.

The fetch spec does not actually parse the integrity metadata until step 18.2 of Main Fetch.  This is after the network request has been made and a Response is available.

So I believe the service worker FetchEvent.request.integrity should reflect the exact value set in the content integrity attribute.  It should not be the result of the parsing operation which discards invalid values.

So I think we need to save the original integrity attribute token string somewhere and just return it.

Sorry for my earlier confusion and sending you on this wrong path.

	François Marier [:francois]
	Comment 116 • 8 years ago

Comment on attachment 8769725 [details] [diff] [review]
Bug 1187335 - P2-part1 - Modify the way to report to console for worker and use LoadTainting to decide CORS or not. r=bkelly.

Review of attachment 8769725 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/SRICheck.cpp
@@ +32,5 @@
>  
> +static void
> +ReportToConsole(uint32_t aErrorFlags, nsContentUtils::PropertiesFile aFile,
> +                const char *aMessageName, nsTArray<nsString>& aParams,
> +                //const nsACString& aParams,

You should remove this line.

	Tom Tung [:tt, :ttung] Assignee
	Comment 117 • 8 years ago

(In reply to François Marier [:francois] from comment #116)
Hi François,

Thanks for your time and review!

> Comment on attachment 8769725 [details] [diff] [review]
> Bug 1187335 - P2-part1 - Modify the way to report to console for worker and
> use LoadTainting to decide CORS or not. r=bkelly.
> 
> Review of attachment 8769725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/security/SRICheck.cpp
> @@ +32,5 @@
> >  
> > +static void
> > +ReportToConsole(uint32_t aErrorFlags, nsContentUtils::PropertiesFile aFile,
> > +                const char *aMessageName, nsTArray<nsString>& aParams,
> > +                //const nsACString& aParams,
> 
> You should remove this line.
Sorry for this. I forget to remove it.

	Tom Tung [:tt, :ttung] Assignee
	Comment 120 • 8 years ago

(In reply to Tom Tung [:tt] from comment #118)
> Created attachment 8771403 [details] [diff] [review]
> [Final] Bug 1187335 - P2-part1 - Modify the way to report to console for
> worker and use LoadTainting to decide CORS or not. r=bkelly, r=francois.
Addressed #116 comment and rebased the patch

(In reply to Tom Tung [:tt] from comment #119)
> Created attachment 8771406 [details] [diff] [review]
> [Final] Bug 1187335 - P3 - modify SRI test to match current behavior.
> r=bkelly, r=francois.
Modify header since it was finished.

	Tom Tung [:tt, :ttung] Assignee
	Comment 121 • 8 years ago

Rebased the patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 122 • 8 years ago

Hi Ben,

I addressed the comment #115 and your feedback in IRC. Could you help me to review this patch? Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 123 • 8 years ago

Change the header since it have already get r+.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 124 • 8 years ago

Comment on attachment 8771411 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

Review of attachment 8771411 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  r=me with comments addressed.

::: dom/security/SRIMetadata.h
@@ +60,5 @@
>    void GetHash(uint32_t aIndex, nsCString* outHash) const;
>    void GetAlgorithm(nsCString* outAlg) const { *outAlg = mAlgorithm; }
>    void GetHashType(int8_t* outType, uint32_t* outLength) const;
>  
> +  void SetOriginalIntegrityString(const nsAString& aString) { mOriginalIntegrityString.Assign(aString); };

Please don't expose a public setter like this.  I think we should just pass the original string to the constructor alongside the token being parsed.  We should be able to make mOriginalIntegrityString a const nsString.

::: testing/web-platform/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ +168,5 @@
>  
> +        script_integrity_test(f, LOCAL_URL, '     ', '     ');
> +        script_integrity_test(f, LOCAL_URL, 'sha256-foo?123', 'sha256-foo?123');
> +        script_integrity_test(f, LOCAL_URL, 'sha256-foo sha384-abc', 'sha256-foo sha384-abc');
> +        script_integrity_test(f, LOCAL_URL, 'sha256-foo sha256-abc', 'sha256-foo sha256-abc');

Can you add a script and css test case for a completely garbage integrity value.  Something like "this is not a valid integrity because it has no dashes".

	Tom Tung [:tt, :ttung] Assignee
	Comment 125 • 8 years ago

Modify this patch for missing an early return.

	Tom Tung [:tt, :ttung] Assignee
	Comment 126 • 8 years ago

Hi Ben,

I addressed the comment, but I'm afraid of misunderstanding what you meant. Thus, could you help me to review it again? Thanks!

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 127 • 8 years ago

Comment on attachment 8771885 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

Review of attachment 8771885 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/base/nsScriptLoader.cpp
@@ +1362,5 @@
>  
> +      nsAutoString integrity;
> +      scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity,
> +                             integrity);
> +      SRIMetadata sriMetadata = SRIMetadata(integrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

@@ +2608,5 @@
>    if (nsContentUtils::IsChromeDoc(mDocument) && aType.LowerCaseEqualsASCII("module")) {
>      return;
>    }
>  
> +  SRIMetadata sriMetadata = SRIMetadata(aIntegrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

::: dom/security/SRIMetadata.h
@@ +26,5 @@
>  
>    /**
> +   * Create an empty metadata object with an integrity metadata.
> +   */
> +  SRIMetadata(const nsAString& aIntegrity)

Is this constructor really necessary?  Can't we just let mOriginalIntegrityString initialize to the empty string?  When SRICheck::IntegrityMetadata() assigns its own SRIMetadata to the out param then it should overwrite the mOriginalIntegrityString with the correct value.

https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#160

@@ +77,5 @@
>    nsCString mAlgorithm;
>    int8_t mAlgorithmType;
>    bool mEmpty;
> +
> +  nsString mOriginalIntegrityString;

const nsString

::: layout/style/Loader.cpp
@@ +1257,5 @@
>        sheetURI = aURI;
>        originalURI = aURI;
>      }
>  
> +    SRIMetadata sriMetadata = SRIMetadata(aIntegrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 128 • 8 years ago

Comment on attachment 8771885 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker.

Review of attachment 8771885 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/base/nsScriptLoader.cpp
@@ +1362,5 @@
>  
> +      nsAutoString integrity;
> +      scriptContent->GetAttr(kNameSpaceID_None, nsGkAtoms::integrity,
> +                             integrity);
> +      SRIMetadata sriMetadata = SRIMetadata(integrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

@@ +2608,5 @@
>    if (nsContentUtils::IsChromeDoc(mDocument) && aType.LowerCaseEqualsASCII("module")) {
>      return;
>    }
>  
> +  SRIMetadata sriMetadata = SRIMetadata(aIntegrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

::: dom/security/SRIMetadata.h
@@ +26,5 @@
>  
>    /**
> +   * Create an empty metadata object with an integrity metadata.
> +   */
> +  SRIMetadata(const nsAString& aIntegrity)

Is this constructor really necessary?  Can't we just let mOriginalIntegrityString initialize to the empty string?  When SRICheck::IntegrityMetadata() assigns its own SRIMetadata to the out param then it should overwrite the mOriginalIntegrityString with the correct value.

https://dxr.mozilla.org/mozilla-central/source/dom/security/SRICheck.cpp#160

@@ +77,5 @@
>    nsCString mAlgorithm;
>    int8_t mAlgorithmType;
>    bool mEmpty;
> +
> +  nsString mOriginalIntegrityString;

But ignore my suggestion to make mOriginalIntegrityString const.  It needs to be assignable.

::: layout/style/Loader.cpp
@@ +1257,5 @@
>        sheetURI = aURI;
>        originalURI = aURI;
>      }
>  
> +    SRIMetadata sriMetadata = SRIMetadata(aIntegrity);

See my comment in SRIMetadata file.  I don't think this change is necessary.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 129 • 8 years ago

Sorry for the duplicate review output there.  I was trying to add just the additional comment about not making mOriginalIntegrityString const.

	Tom Tung [:tt, :ttung] Assignee
	Comment 130 • 8 years ago

Addressed the comment #128 and assigning the integrity string when the metadata is invalid and empty as we discuss in IRC. 

This patch is mainly to let css and script pass exact integrity metadata to service worker through HttpChannelInternal. To do this, I did few things and I put them below inline.
   1. modify SRIMetadata to let it be able to carry an originalIntegrityString.
   2. modify SRICheck::IntegrityMetadata(..) to make a SRIMetadata carry an originalIntegrityString
   3. set integrity metadata into httpChannelInternal in nsScript.cpp and Loader.cpp
   4. write few simple tests for it.

	Tom Tung [:tt, :ttung] Assignee
	Comment 131 • 8 years ago

Comment on attachment 8772258 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker. r=bkelly.

Hi François,

As I mention in comment #130, I did few changes in nsScriptLoader, Loader, SRIMetadata and SRICheck to pass exact integrity string to service worker. Could you help to review this patch? Thanks!

	François Marier [:francois]
	Comment 132 • 8 years ago

Comment on attachment 8772258 [details] [diff] [review]
Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to serviceWorker. r=bkelly.

Review of attachment 8772258 [details] [diff] [review]:
-----------------------------------------------------------------

I feel like this change introduces too many unnecessary string copies. Every token in the loop of SRICheck::IntegrityMetadata() will create a copy the whole integrity attribute just to (probably) throw it away later.

Correct me if I'm wrong, but what you really need is for IntegrityMetadata() to set mOriginalIntegrity just before it returns (https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/security/SRICheck.cpp#175):

    outMetadata->mOriginalIntegrityString = aMetadataList;
    return NS_OK;

If you make that change, you probably won't need the new constructors.

::: dom/security/SRIMetadata.h
@@ +79,5 @@
>    nsCString mAlgorithm;
>    int8_t mAlgorithmType;
>    bool mEmpty;
> +
> +  nsString mOriginalIntegrityString;

You should maintain the size order here (smallest member variable at the end). mOriginalIntegrityString should go in between mHashes and mAlgorithm.

Also (nit), why not just mIntegrityString?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 133 • 8 years ago

(In reply to François Marier [:francois] from comment #132)
> I feel like this change introduces too many unnecessary string copies. Every
> token in the loop of SRICheck::IntegrityMetadata() will create a copy the
> whole integrity attribute just to (probably) throw it away later.
> 
> Correct me if I'm wrong, but what you really need is for IntegrityMetadata()
> to set mOriginalIntegrity just before it returns
> (https://dxr.mozilla.org/mozilla-central/rev/
> 4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/security/SRICheck.cpp#175):
> 
>     outMetadata->mOriginalIntegrityString = aMetadataList;
>     return NS_OK;

I'm ok with this as long as we don't have a public setter on SRIMetadata.

Keep in mind, though, that we have copy-on-write strings.  We're not actually copying the entire contents of the string here.  Its just copying a reference to the string buffer.

	François Marier [:francois]
	Comment 134 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #133)
> (In reply to François Marier [:francois] from comment #132)
> >     outMetadata->mOriginalIntegrityString = aMetadataList;
> >     return NS_OK;
> 
> I'm ok with this as long as we don't have a public setter on SRIMetadata.

Indeed, we should avoid that. I was thinking it would be reasonable to make SRICheck a friend class of SRIMetadata.

> Keep in mind, though, that we have copy-on-write strings.  We're not
> actually copying the entire contents of the string here.  Its just copying a
> reference to the string buffer.

Ah good point.

	Tom Tung [:tt, :ttung] Assignee
	Comment 135 • 8 years ago

(In reply to François Marier [:francois] from comment #132)
Hi François,

Thanks for your review and your feedback! I learn a lot from them!

> Comment on attachment 8772258 [details] [diff] [review]
> Bug 1187335 - P4-part3 - Support script/css to set integrity metadata to
> serviceWorker. r=bkelly.
> 
> Review of attachment 8772258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel like this change introduces too many unnecessary string copies. Every
> token in the loop of SRICheck::IntegrityMetadata() will create a copy the
> whole integrity attribute just to (probably) throw it away later.

Yeah, I forgot that we could set private class variable via becoming its friend class without a public setter. Making SRICheck a friend class of SRIMetadata is a good idea to solve this problem. 
I'll address that in next patch!

> Correct me if I'm wrong, but what you really need is for IntegrityMetadata()
> to set mOriginalIntegrity just before it returns
> (https://dxr.mozilla.org/mozilla-central/rev/
> 4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/security/SRICheck.cpp#175):
> 
>     outMetadata->mOriginalIntegrityString = aMetadataList;
>     return NS_OK;
> 
> If you make that change, you probably won't need the new constructors.
> 
> ::: dom/security/SRIMetadata.h
> @@ +79,5 @@
> >    nsCString mAlgorithm;
> >    int8_t mAlgorithmType;
> >    bool mEmpty;
> > +
> > +  nsString mOriginalIntegrityString;
> 
> You should maintain the size order here (smallest member variable at the
> end). mOriginalIntegrityString should go in between mHashes and mAlgorithm.

Sure!

> Also (nit), why not just mIntegrityString?

It's okay for me to name it "mIntegrityString" and I'll apply it to next patch. I thought "mOriginalIntegrityString" may be clearer to stand for whole integrity string but it's too long.

	Tom Tung [:tt, :ttung] Assignee
	Comment 136 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #133)
Thanks for reminding me that, Ben. I would like to address comment #134 and we'll not have a public setter in this method.

	Tom Tung [:tt, :ttung] Assignee
	Comment 137 • 8 years ago

Hi François,

I address the comment #132 and #134. Could you help me to review this again? Thanks!

	François Marier [:francois]
	Comment 138 • 8 years ago

Comment on attachment 8771406 [details] [diff] [review]
[Final] Bug 1187335 - P3 - modify SRI test to match current behavior. r=bkelly, r=francois.

Review of attachment 8771406 [details] [diff] [review]:
-----------------------------------------------------------------

I missed two things in my previous review of this patch.

I think we also need to change this test so that it's expected to pass:

https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#15

And wouldn't your changes to the eligibility algorithm also affect these test cases?

https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#61-72

	Tom Tung [:tt, :ttung] Assignee
	Comment 139 • 8 years ago

(In reply to François Marier [:francois] from comment #138)

Sorry for the late reply, I somehow miss this comment. :(

> Comment on attachment 8771406 [details] [diff] [review]
> [Final] Bug 1187335 - P3 - modify SRI test to match current behavior.
> r=bkelly, r=francois.
> 
> Review of attachment 8771406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I missed two things in my previous review of this patch.
> 
> I think we also need to change this test so that it's expected to pass:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_style_crossdomain.html#15

I'm not sure what should I do. I guess I should remove |,.red-text{color:red}| in [1]? Could you give me some hints about that, François? Thanks! 

[1] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#79

> And wouldn't your changes to the eligibility algorithm also affect these
> test cases?
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_script_crossdomain.html#61-72

Oh, they should be changed. I forgot [2] because it is marked as "todo". 
Thanks for reminding me that but I found one thing interesting. I thought [1] should be good_correctDataLoaded(), but I found its LoadTainting is opaque and still try to find out why... 

[1] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#61-66
[2] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#67-72

	François Marier [:francois]
	Comment 140 • 8 years ago

(In reply to Tom Tung [:tt] from comment #139)
> (In reply to François Marier [:francois] from comment #138)
> > I think we also need to change this test so that it's expected to pass:
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> > iframe_style_crossdomain.html#15
> 
> I'm not sure what should I do. I guess I should remove
> |,.red-text{color:red}| in [1]? Could you give me some hints about that,
> François? Thanks! 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_style_crossdomain.html#79

That particular one is already blocked:

  https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#45-50

so the ",.red-text{color:red}" will not be applied.

However, this one is not currently blocked:

  https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#51-56

and it also tries to apply ",.red-text{color:red}".

So I think all we need to do is to change the "todo()" to "ok()" on both of these lines:

  https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#15
  https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_style_crossdomain.html#55

	Tom Tung [:tt, :ttung] Assignee
	Comment 141 • 8 years ago

(In reply to François Marier [:francois] from comment #140)

Thanks for feedback, François! But it's still not clear to me. I might misunderstand something. :(

> (In reply to Tom Tung [:tt] from comment #139)
> That particular one is already blocked: 
>  
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_style_crossdomain.html#45-50
> so the ",.red-text{color:red}" will not be applied.

I think I may misunderstand something. Please correct me if I'm wrong. 
This testcase should be loaded since it is same-origin [1]. Thus, ",.red-text{color:red}" should be applied and so does #51-56. We've discussed that in comment #30 and it is the reason why I wrote this patch. Hence, I don't think only changing from "todo()" to "ok()" will be correct.

[1] https://github.com/whatwg/html/issues/1243

However, it's strange to me that I find out the "LoadTainting" in [2] is opaque since I reckon it should be same-origin and the testcase [3] has proved that with the "LoadTainting" is basic. There may exist a bug for "LoadTainting" because either same-origin or cross-origin cannot have correct "LoadTainting" value?

[2] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#115-119
[3] https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/iframe_script_crossdomain.html#121-126

	François Marier [:francois]
	Comment 142 • 8 years ago

(In reply to Tom Tung [:tt] from comment #141)
> I think I may misunderstand something. Please correct me if I'm wrong. 
> This testcase should be loaded since it is same-origin [1]. Thus,
> ",.red-text{color:red}" should be applied and so does #51-56. We've
> discussed that in comment #30 and it is the reason why I wrote this patch.
> Hence, I don't think only changing from "todo()" to "ok()" will be correct.

You're right. This entire test was written while the spec was wrong.

So I think instead what we need to do is change it to something like this:

      var redText = document.getElementById('red-text');
      var greenText = document.getElementById('green-text');
      var blueText = document.getElementById('blue-text');
      var redTextColor = window.getComputedStyle(redText, null).getPropertyValue('color');
      var greenTextColor = window.getComputedStyle(greenText, null).getPropertyValue('color');
      var blueTextColor = window.getComputedStyle(blueText, null).getPropertyValue('color');

      ok(redTextColor == 'rgb(255, 0, 0)', "The first part should be red.");
      ok(greenTextColor == 'rgb(0, 255, 0)', "The second part should be green.");
      ok(blueTextColor == 'rgb(0, 0, 255)', "The third part should be blue.");


<p><span id="red-text">This should be red</span>, 
  <span id="green-text">this should be green</span> and
  <span id="blue-text">this should be blue</span>.</p>


  <!-- valid non-CORS sha256 hash in a data: URL -->
  <link rel="stylesheet" href="data:text/css,#green-text{color:green}"
        integrity="sha256-<updated hash>"

  <!-- valid CORS sha256 hash in a data: URL -->
  <link rel="stylesheet" href="data:text/css,#blue-text{color:blue}"
        crossorigin="anonymous"
        integrity="sha256-<updated hash>"

to account for the fact that both data: URIs will get loaded properly now.

	François Marier [:francois]
	Comment 143 • 8 years ago

(In reply to Tom Tung [:tt] from comment #141)
> However, it's strange to me that I find out the "LoadTainting" in [2] is
> opaque since I reckon it should be same-origin and the testcase [3] has
> proved that with the "LoadTainting" is basic. There may exist a bug for
> "LoadTainting" because either same-origin or cross-origin cannot have
> correct "LoadTainting" value?
> 
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_script_crossdomain.html#115-119
> [3]
> https://dxr.mozilla.org/mozilla-central/rev/
> db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/security/test/sri/
> iframe_script_crossdomain.html#121-126

Right, I would expect stylesheets and scripts to behave in the same way here. So we should make the same changes to the tests and then fix the underlying bug.