1418860 - Adding telemetry to see how often version param is used in ScriptLoader type

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Masatoshi Kimura [:emk]]

I think this should be done in 59 cycle because people might be using the version parameter to sniff E4X for-each / legacy generators support.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi :jandem, do you think you have bandwidth to take this?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Hi :jandem, do you think you have bandwidth to take this?

No and it's better for someone on the DOM team to take this on IMO. Also we should probably add Telemetry/warnings first before we remove this.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > Hi :jandem, do you think you have bandwidth to take this?
> 
> No and it's better for someone on the DOM team to take this on IMO. Also we
> should probably add Telemetry/warnings first before we remove this.

Thanks. Let me explore an alternative. :)

Comment 4

[Andrew Overholt [:overholt]]

Jon is knowledgeable here as is baku (I think). Catalin is scheduled to work on JS Modules from the DOM side so maybe he is interested? CCing everyone.

Comment 5

[Andrea Marchesini [:baku]]

> No and it's better for someone on the DOM team to take this on IMO. Also we
> should probably add Telemetry/warnings first before we remove this.

Why telemetry? I guess the point of this bug is to ignore the 'version' parameter. Btw, we already ignore the version value. The only thing we care is if the version is valid or not:

https://dxr.mozilla.org/mozilla-central/source/dom/script/ScriptLoader.cpp#1176-1187

Comment 6

[Andrea Marchesini [:baku]]

Attachment: version.patch

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8931421 [details] [diff] [review]
version.patch

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

::: dom/script/ScriptLoader.cpp
@@ +1178,5 @@
>    nsAutoString mimeType;
>    nsresult rv = parser.GetType(mimeType);
>    NS_ENSURE_SUCCESS(rv, false);
>  
>    if (!nsContentUtils::IsJavascriptMIMEType(mimeType)) {

The new code should use aType directly instead of parsing it to align with the spec and other browsers.

Comment 8

[Masatoshi Kimura [:emk]]

That is, "application/javascript;version=1.8" should not be considered as a JavaScript MIME type.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

We should still be backwards compatible. Sounds rather scary to not support the old stuff there.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Andrea Marchesini [:baku] from comment #5)
> > No and it's better for someone on the DOM team to take this on IMO. Also we
> > should probably add Telemetry/warnings first before we remove this.
> 
> Why telemetry? I guess the point of this bug is to ignore the 'version'
> parameter. Btw, we already ignore the version value. The only thing we care
> is if the version is valid or not:

Yeah I did that in bug 1417895. But as emk said, the "right" behavior is probably to ignore (not execute) versioned script tags (like other browsers do) and that change could break things (also for Thunderbird etc).

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Andrea Marchesini [:baku] from comment #6)
This patch removes the validity check on the version number and makes it so we allow any version to be specified with the mime type.  Is that what we want to do here?  It seems the end goal is to not allow any version parameter at all (as comment 7 suggests), but we also don't want to break anything that relies on this.  This change makes us more permissive rather than less.

Comment 12

[Masatoshi Kimura [:emk]]

Note that something that rely on versioned scripts are already broken by bug 1083482 and bug 1388317 because versioned scripts are basically used to enable those removed features. Even if we continue accept versions, the code will throw SyntaxError anyway.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> Yeah I did that in bug 1417895. But as emk said, the "right" behavior is
> probably to ignore (not execute) versioned script tags (like other browsers
> do) and that change could break things (also for Thunderbird etc).

Thunderbird would not be relevant to this bug because I already disable version scripts in XUL (bug 1390106). Only web-exposed content would be affected.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: version2.patch

Here I'm adding a telemetry ID to store how often a script is used with version parameter.

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 8931682 [details] [diff] [review]
version2.patch

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

LGTM.

Comment 16

[François Marier [:francois]]

Comment on attachment 8931682 [details] [diff] [review]
version2.patch

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

That looks fine to me.

We have a new process for data collection (including new telemetry probes): https://wiki.mozilla.org/Firefox/Data_Collection#Step_1:_Submit_Request

All that's needed is for you to download the request form from Github, answer the questions and attach it as a text file to this bug.

You can r? me on that file and I will take care of Step 2.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: request.txt

Comment 18

[François Marier [:francois]]

(In reply to Andrea Marchesini [:baku] from comment #17)
> Created attachment 8932260 [details]
> request.txt

By the way, this is NOT Category 3 data (browsing history), but rather Category 1:

    "This also includes compatibility information about features and APIs used by websites"

which is good because Category 3 is generally opt-out only and has to be stored outside of telemetry :)

Comment 19

[François Marier [:francois]]

Comment on attachment 8932260 [details]
request.txt

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

Yes, in Histograms.json

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.

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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0a56e8287da
Adding telemetry to see how often version param is used in ScriptLoader type, r=jonco, data-r=francois

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 22

[Andrea Marchesini [:baku]]

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2018-01-07&keys=__none__!__none__!__none__&max_channel_version=nightly%252F59&measure=SCRIPT_LOADED_WITH_VERSION&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-11-30&table=0&trim=1&use_submission_date=0

We have some data. It seems that in nightly 59, the use is 0.02% I would like to proceed with removing versioning support in ScriptLoader. Feedback?

Comment 23

[Masatoshi Kimura [:emk]]

I concur with removing support.

Comment 24

[David Bruant]

This bugs and bug 1428745 have the same name.
I feel this bug is only about adding telemtry though. I'm taking the initiative to change the summary accordingly (based on the commit message)