Warn when String generics are used

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

({addon-compat, dev-doc-complete, site-compat})

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
Prerequisite for bug 1222552
Assignee

Updated

3 years ago
Blocks: 1222552
Assignee

Updated

3 years ago
Depends on: 1319934
Assignee

Updated

3 years ago
Depends on: 1319935
Assignee

Updated

3 years ago
Depends on: 1319936
Assignee

Updated

3 years ago
Depends on: 1319937
Assignee

Updated

3 years ago
Depends on: 1319938
Assignee

Updated

3 years ago
Depends on: 1319939
Assignee

Updated

3 years ago
Depends on: 1320143
Assignee

Updated

3 years ago
Depends on: 1320144
Assignee

Comment 1

3 years ago
Attachment #8816289 - Flags: review?(jdemooij)
Assignee

Comment 2

3 years ago
Posted patch bug1319926-part2.patch (obsolete) — Splinter Review
I've left a To-Do note in intrinsic_WarnDeprecatedStringMethod, because I'm not sure which compartment needs to be used for the telemetry callback.
Attachment #8816290 - Flags: review?(jdemooij)
Comment on attachment 8816289 [details] [diff] [review]
bug1319926-part1.patch

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

Nice, thanks!

Calling the intrinsic will definitely add some overhead, but considering the deprecated/proprietary status we don't really care.
Attachment #8816289 - Flags: review?(jdemooij) → review+
Comment on attachment 8816290 [details] [diff] [review]
bug1319926-part2.patch

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

::: js/src/vm/SelfHosting.cpp
@@ +1917,5 @@
>  
> +    NonBuiltinScriptFrameIter iter(cx);
> +    if (!iter.done()) {
> +        const char* filename = iter.filename();
> +        // TODO: Or `iter.compartment()`?

Good question. IMO it doesn't matter too much which compartment we use here, in the vast majority of cases they will be the same anyway. Maybe for consistency with iter.filename() use iter.compartment().
Attachment #8816290 - Flags: review?(jdemooij) → review+
Assignee

Comment 5

3 years ago
Updated part 2 per review comments. Carrying r+ from jandem.
Attachment #8816290 - Attachment is obsolete: true
Attachment #8817662 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 6

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07d6bf74b7a2
Part 1: Warn when deprecated String generics methods are used. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a110ad242ea
Part 2: Collect telemetry about deprecated String generics methods. r=jandem
Keywords: checkin-needed

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/07d6bf74b7a2
https://hg.mozilla.org/mozilla-central/rev/5a110ad242ea
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
We don't currently support changing bucket counts for histograms.
The recommended way is to rename them instead, e.g. JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT_2.
Flags: needinfo?(andrebargull)
Assignee

Comment 10

3 years ago
Ugh, I miscounted the entries. Apparently I shouldn't have touched "n_values" at all. I'll fix this when I'm back at home.
Do we need a warning for the deprecated Array generics (Bug 1222547) as well?
Assignee

Comment 12

3 years ago
(In reply to Kohei Yoshino [:kohei] from comment #11)
> Do we need a warning for the deprecated Array generics (Bug 1222547) as well?

Yes, eventually. But first we need to replace the various uses of Array generics in Firefox code (there are lots of them!).
Assignee

Comment 13

3 years ago
(In reply to André Bargull from comment #10)
> Ugh, I miscounted the entries. Apparently I shouldn't have touched
> "n_values" at all. I'll fix this when I'm back at home.

Or rather I need to request backout for part 2, because we probably first want to discuss on #jsapi and maybe also #telemetry to find out what's the best way forward.
Assignee

Comment 14

2 years ago
Per comment #9, we can't extend JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT and JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_ADDONS to track String generics, which means part 2 needs to be backed out. :-(
Attachment #8821101 - Flags: review?(jdemooij)
Attachment #8821101 - Flags: review?(jdemooij) → review+
Assignee

Updated

2 years ago
Flags: needinfo?(andrebargull)
Keywords: checkin-needed

Comment 15

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5d5babe3d8
Backed out changeset 5a110ad242ea . r=jandem
Keywords: checkin-needed
Merged backout: https://hg.mozilla.org/mozilla-central/rev/1d5d5babe3d8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 17

2 years ago
Part 1 has landed and adding telemetry counters for String generics is now tracked in bug 1339777, so we can close this bug.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → FIXED
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String#String_generic_methods shows a "do not use" banner
https://developer.mozilla.org/en-US/Firefox/Releases/53#JavaScript calls out the deprecation.

To further help deprecation, we could add [Learn more] to the warning and then point to migration best practices (see bug 1293205 for how it was done for |for each|). Basically, a new page needs to be written here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors and "JSMSG_DEPRECATED_STRING_METHOD" and the URL needs to be added to https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/errordocs.js
You need to log in before you can comment on or make changes to this bug.