1416629 - Add a telemetry to see how many sites making a CORS response for a same-origin mode request

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Tom Tung [:tt, :ttung]]

Please refer to bug 1222008 comment 55.

Ideally, measure how many requests for loading a document do that on both normal script and worker script.

The fallback solution is to measure how many requests (for all kinds of request) do that.

I'll try to implement this and send it to review soonly.

Comment 1

[Tom Tung [:tt, :ttung]]

Hi Ben,

Currently, I only have a naive idea to implement this and few things not sure about. Would you mind giving me some feedback? Thanks!

1. Have a counter on nsIDocument to record how many CORS responses are generated by same-origin mode requests.

2. On the place where we get the response (OnStopRequest() [1]) tries to get the channel (in order to get corsMode attribute) and the document object (in order to increase the counter).

[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN12_GLOBAL__N_114LoaderListener13OnStopRequestEP10nsIRequestP11nsISupports8nsresult%2C_ZN18nsIRequestObserver13OnStopRequestEP10nsIRequestP11nsISupports8nsresult&redirect=false

Few questions:
- You said "use counter" on bug 1222008 comment 55. Did you mean this API[2] or just a counter to count the frequency of use by maybe Histograms API? (although, it seems to often be used on WEBIDL)
[2] https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/use-counters.html

Might be stupid questions:
- Could this happen without a service worker redirect the request? If not, maybe I could try to implement this within the code for respondWith() in ServiceWorkerEvent.
- You mentioned that "get that out of the LoadInfo or nsIChannel because I need to get a nsIDocument to reference". Why? Is the reason that we cannot get a nsIDocument object from nsIChannel because it has not been created yet?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #1)
> Hi Ben,
> 
> Currently, I only have a naive idea to implement this and few things not
> sure about. Would you mind giving me some feedback? Thanks!
> 
> 1. Have a counter on nsIDocument to record how many CORS responses are
> generated by same-origin mode requests.
> 
> 2. On the place where we get the response (OnStopRequest() [1]) tries to get
> the channel (in order to get corsMode attribute) and the document object (in
> order to increase the counter).
> 
> [1]
> https://searchfox.org/mozilla-central/search?q=symbol:
> _ZN12_GLOBAL__N_114LoaderListener13OnStopRequestEP10nsIRequestP11nsISupports8
> nsresult%2C_ZN18nsIRequestObserver13OnStopRequestEP10nsIRequestP11nsISupports
> 8nsresult&redirect=false
> 
> Few questions:
> - You said "use counter" on bug 1222008 comment 55. Did you mean this API[2]
> or just a counter to count the frequency of use by maybe Histograms API?
> (although, it seems to often be used on WEBIDL)
> [2]
> https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/
> telemetry/collection/use-counters.html

I meant this API:

https://searchfox.org/mozilla-central/source/dom/base/UseCounters.conf#54

So you could do something like this to increase the count:

https://searchfox.org/mozilla-central/source/dom/base/DOMError.cpp#75

> Might be stupid questions:
> - Could this happen without a service worker redirect the request? If not,
> maybe I could try to implement this within the code for respondWith() in
> ServiceWorkerEvent.

Yea, I think we should put this in the respondWith() code in ServiceWorkerEvents.cpp.

> - You mentioned that "get that out of the LoadInfo or nsIChannel because I
> need to get a nsIDocument to reference". Why? Is the reason that we cannot
> get a nsIDocument object from nsIChannel because it has not been created yet?

I just couldn't remember how to get the nsIDocument out of an nsIChannel.  Some channels won't have an nsIDocument associated with it, though.

For example, in worker script loading we sometimes don't have an nsIDocument (nested worker or SharedWorker) or sometimes we just don't want to use the document:

https://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#138

So we end up creating the channel without the document here:

https://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#198

I'm just not sure which case we follow for normal dedicated workers like `new Worker('foo.js')`.  If we don't have an nsIDocument on the channel, then we may not be easily able to do use counters.  Instead we would just have to do normal telemetry probes like Telemetry::Accumulate().

Does that help?

Comment 3

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #2)
Thanks for the feedback and clearing up my confusion! And, yes, it does help a lot!

Comment 4

[Tom Tung [:tt, :ttung]]

Hi Ben,

For now, I can only get the document from the intercepted channel if the request is comming from the document. But, for the request sending from a worker, I have no idea how to get the document which create the worker from the channel.

So, I'm considering to use a use counter to track the request sent from a doucment and other telemtery like Histogram to track the request sent from a worker. 

Do you any suggestions for that? Is it a good idea to tracking them separately? Or, we should use a telemtery to tracking both cases just like the fall back option you mentioned in bug 1222008 comment 55?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I think we should just do normal telemetry and not use counters for now.  If we can't include all the data in the use counter then we can't reasonably use it to make a decision anyway.

I was hoping we could use FetchEvent.clientId to track back to the document, but we don't currently set it for worker script requests.

Comment 6

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request.

Hi Ben,

This patch add two counter to record cors type response for same-origin mode request and overall synthesized repsonse by Scalars. So that we can get how many precentage of response is that kind of response. 

I mark the expire release to be 61 because I think we may need to make the decision soon. But I'm not so sure about that.

Would you mind helping me to review it? Thanks!

I'll ask for a data review after getting r+.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928878 [details] [diff] [review]
Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request.

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

Looks great to me.  Thanks!

Rebecca, can you review these two telemetry probes?  We want to prevent sites from doing the conditional part here, but are unsure of how many sites do that today.  We are hoping the probes will give us some insight.

Note, we would have liked to normalize these to the number of pages that trigger the condition, but unfortunately there is not a good way to get the nsIDocument in this code.  We are settling for a percentage of total number of synthesized responses.

If possible, we would like to uplift these probes to FF58 so we can make our decision sooner.

Comment 8

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bcf5ca626aebed3c96af4febca0c7bb3b1d0d1c

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8928878 [details] [diff] [review]
Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request.

Francois, can you do a data review of this telemetry probe?  I've been told Rebecca might be out on PTO.

Comment 10

[François Marier [:francois]]

Comment on attachment 8928878 [details] [diff] [review]
Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request.

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

Looks good to me.

By the way, the data review process has changed and we now ask people to answer a few questions: https://wiki.mozilla.org/Firefox/Data_Collection#Step_1:_Submit_Request

Essentially, all you have to do is download the text file from Github (https://github.com/mozilla/data-review/blob/master/request.md), answer the questions in it and attach the completed file on Bugzilla. You can flag me as r? on it and I will complete the rest of the review process.

::: toolkit/components/telemetry/Scalars.yaml
@@ +1315,5 @@
> +  cors_res_for_so_req_count:
> +    bug_numbers:
> +      - 1416629
> +    description: >
> +      The count of number of synthesize response which is for a same-origin mode

It might be good to specify that it's tracking "synthesize responses made by service workers" (i.e. including the words "service worker").

@@ +1316,5 @@
> +    bug_numbers:
> +      - 1416629
> +    description: >
> +      The count of number of synthesize response which is for a same-origin mode
> +      reqeust and its mode is cors.

typo: "request"

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request. r=bkelly, data-r=francois

Address the comment

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: request.md

Hi Francois,

I try to fill the document that you mentioned. Could you help me to review it? Thanks!

Comment 13

[François Marier [:francois]]

Comment on attachment 8931200 [details]
request.md

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Scalars.yaml.

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default on on pre-release channels only.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, telemetry alerts are sufficient.

Comment 14

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #13)
> 5) Is the data collection request for default-on or default-off?
> 
> Default on on pre-release channels only.

I got this wrong. This is opt-out on all channels, including release.

Comment 15

[Tom Tung [:tt, :ttung]]

(In reply to François Marier [:francois] from comment #13)
Thanks for the review!

I'm going to push this to inbound and maybe uplift this to 58 next week.

Comment 16

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #15)
> I'm going to push this to inbound and maybe uplift this to 58 next week.

Hmm the mozilla-inbound is closed, I'll try again later.

Comment 17

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request. r=bkelly, data-r=francois

Update the patch.

Note: I do a slight change to this patch. I move the counter for overall synthesized response earlier than the counter for cors type response on same-origin request.

Comment 18

[Tom Tung [:tt, :ttung]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f9187b0b2e9c42f5eeca898bf81640174573fe
Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request. r=bkelly, data-r=francois

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c6f9187b0b2e

Comment 20

[Tom Tung [:tt, :ttung]]

Sorry, I forgot to mark it as "leave-open". I still need to uplift this to FF58

Comment 21

[Tom Tung [:tt, :ttung]]

Comment on attachment 8931553 [details] [diff] [review]
[Final] Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request. r=bkelly, data-r=francois

Approval Request Comment
[Feature/Bug causing the regression]:

This is not a regression.

We are considering to decide whether to reject the cors type response for same-origin mode request in service worker on bug 1222008. And, we want to make the decision soon, so send an uplift request to Firefox 58.

[User impact if declined]:

No, but it would be nice if we can collect the data and make the final decision as soon as possible.

[Is this code covered by automated tests?]:

No, but this code only add two telemetries, and it has been landed in Nightly (FF59)

[Has the fix been verified in Nightly?]:

It's not a regression, so it doesn't need to be verified. But, it's fixed in Nightly.

[Needs manual test from QE? If yes, steps to reproduce]: 

No

[List of other uplifts needed for the feature/fix]:

No, only this patch.

[Is the change risky?]:

No, I don't think so.

[Why is the change risky/not risky?]:

The patch doesn't change any current service worker code patch and logic. It only adds two probs.

[String changes made/needed]:

No

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8931553 [details] [diff] [review]
[Final] Bug 1416629: Add a telemetry to get how many precentage of synthesized cors response for same-origin mode request. r=bkelly, data-r=francois

Take this to help gather telemetry data. Should be low risk. Beta58+.

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b9d29c50dff5