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.

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?

(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?

(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!

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?

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.

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 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 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 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"

Address the comment

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

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.

(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.

(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.

(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.

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.

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

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

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 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+.