Closed Bug 1319926 Opened 4 years ago Closed 4 years ago

Warn when String generics are used

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

(Keywords: addon-compat, dev-doc-complete, site-compat)

Attachments

(3 files, 1 obsolete file)

Prerequisite for bug 1222552
Blocks: 1222552
Depends on: 1319934
Depends on: 1319935
Depends on: 1319936
Depends on: 1319937
Depends on: 1319938
Depends on: 1319939
Depends on: 1320143
Depends on: 1320144
Attachment #8816289 - Flags: review?(jdemooij)
Attached 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+
Updated part 2 per review comments. Carrying r+ from jandem.
Attachment #8816290 - Attachment is obsolete: true
Attachment #8817662 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/07d6bf74b7a2
https://hg.mozilla.org/mozilla-central/rev/5a110ad242ea
Status: ASSIGNED → RESOLVED
Closed: 4 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)
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?
Keywords: addon-compat
(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!).
(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.
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+
Flags: needinfo?(andrebargull)
Keywords: checkin-needed
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5d5babe3d8
Backed out changeset 5a110ad242ea . r=jandem
Keywords: checkin-needed
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
Closed: 4 years ago4 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.