1208835 - Add telemetry for the RegExp static property access affected by removing PreserveRegExpStatics.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Separated from bug 887016.

RegExpStatics (RegExp.$1 etc) is not in ES6 spec, and its behavior is different between engines when String.prototype.replace is called with a function as replaceValue argument.

 * V8 and JSC updates RegExp.$1 value before calling replaceValue,
 * Chakra updates RegExp.$1 value after calling replaceValue only if replaceValue didn't throw,
 * SpiderMonkey updates RegExp.$1 before calling replaceValue, and restores the value after calling replaceValue, even if it threw

Also, PreserveRegExpStatics makes selfhosting RegExp.prototype[@@replace] a bit hard and complicated (even with try-catch...).

If we can switch to V8 and JSC's behavior, it gets so simpler and we can decrease fragmentation of the web platform's behavior.

This is breaking change, possible problematic case is following:
  1. String.prototype.replace is called with RegExp and function as its arguments
  2. another RegExp opreation is performed inside the function
  3. RegExp.$1 (or similar RegExp static property) is accessed after String.prototype.replace returns
Currently, RegExp.$1 contains the result of String.prototype.replace.  With the change, it contains the result of the last RegExp operation, in the function.

I'll post try run result shortly (the draft patch in bug 887016 comment #17 was not correct).

Comment 1

[Tooru Fujisawa [:arai]]

As its behavior is different between browsers, this won't be a web-compat issue, but it could be an addon-compat issue.

here's difference:

                                    //  Firefox | Chrome | Safari | Edge | IE
/(a)/.exec("a");                    // ---------+--------+--------+------+----
console.log(RegExp.$1);             //     a    |    a   |    a   |   a  |  a
"b".replace(/(b)/, function() {     //          |        |        |      |
  try {                             //          |        |        |      |
    console.log(RegExp.$1);         //     b    |    b   |    b   |   a  |  a
    "c".replace(/(c)/, function() { //          |        |        |      |
      console.log(RegExp.$1);       //     c    |    c   |    c   |   a  |  a
      /(d)/.exec("d");              //          |        |        |      |
      console.log(RegExp.$1);       //     d    |    d   |    d   |   d  |  d
      throw 1;                      //          |        |        |      |
    });                             //          |        |        |      |
  } catch (e) {}                    //          |        |        |      |
  console.log(RegExp.$1);           //     c    |    d   |    d   |   d  |  d
});                                 //          |        |        |      |
console.log(RegExp.$1);             //     b    |    d   |    d   |   b  |  b

Comment 2

[Tooru Fujisawa [:arai]]

Try run with totally removed PreserveRegExpStatics and related properties/methods from RegExpStatics:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d47a9f75261

Comment 3

[Tooru Fujisawa [:arai]]

Forgot to change StrReplaceRegExp not to use RegExpStatics to get rightContext after replace :P
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=372669e91067

Comment 4

[Tooru Fujisawa [:arai]]

it passed the try run :)

Comment 5

[Tooru Fujisawa [:arai]]

Try with Android and B2G also passed:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=72cea85f80aa

Comment 6

[Tooru Fujisawa [:arai]]

Warning (and telemetry) for this might be a bit complicated.  I'm thinking something like following.

  * add |counter| and |affected| properties to RegExpStatics class
    the initial value of |counter| is 0, and |affected| is false

  * whenever changing RegExpStatics value, increment |counter| and change |affected| to false

  * before calling replaceValue function, remember the value of |counter| as |beforeValue|
  * after calling replaceValue function, compare |counter| and |beforeValue|
    if they are different, set |affected| to true

  * whenever reading RegExpStatics property, check |affected|, and if it's true, print warning

Comment 7

[Tooru Fujisawa [:arai]]

Testing a patch for warning, with crash
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d157a9121491

Comment 8

[Till Schneidereit [:till]]

Perhaps we should just try to land this? The Firefox frontend plus B2G not causing issues is a pretty good indicator that this might just work. It's still relatively early in the cycle, so it'd have plenty of time on Nightly.

The only concern is that any errors caused by this change might be hard to connect to it, so we might not know if it's causing issues that would make us want to back it out. Do you think that that's a serious concern?

Comment 9

[Tooru Fujisawa [:arai]]

(In reply to Till Schneidereit [:till] from comment #8)
> Perhaps we should just try to land this? The Firefox frontend plus B2G not
> causing issues is a pretty good indicator that this might just work. It's
> still relatively early in the cycle, so it'd have plenty of time on Nightly.
> 
> The only concern is that any errors caused by this change might be hard to
> connect to it, so we might not know if it's causing issues that would make
> us want to back it out. Do you think that that's a serious concern?

Yeah, it could be hard to debug, as the actual issue may not happen just after the String.prototype.replace.  For in-tree case, try run with warning and crash should be enough to catch them.  At least in Firefox frontend code, there seems to be no such case (try in comment #7).
Just pushed to try for B2G and Android:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9075a20ee509

If all in-tree tests are passed, remaining concerns are comm-central and Add-ons.
I skimmed comm-central tree, and in almost cases RegExp static properties are accessed directly after match/exec/test, so I think the risk there is very low.
I have no idea how many such cases in Add-ons.  Do you have an access to AMO MXR?  If there are many usage and some of them are not directly after match/search/exec/test, it might be risky, and it would be helpful for Add-on developers to land warning first and wait for several cycles before removing PreserveRegExpStatics.

If all tests passed on all platforms and there's almost no such cases in Add-ons, I think we can just remove PreserveRegExpStatics without adding warning/telemetry.

Comment 10

[Tooru Fujisawa [:arai]]

As discussed in IRC, determining the usage in Add-ons with MXR is too hard, adding telemetry would be easier way.

B2G and Android also passed the try, so this warning won't hit in-tree code.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add telemetry for RegExp static property access after String.prototype.replace with function argument and RegExp static property is changed inside it.

Added matchId and isRestoredFromModifiedMatch properties to RegExpStatics class, matchId corresponds to |counter| and isRestoredFromModifiedMatch corresponds to |affected| in comment #6.

matchId is incremented on each update, and isRestoredFromModifiedMatch is set to true only when restoring from different matchId, and set back to false on next update.  So |isRestoredFromModifiedMatch == true| means the value in RegExpStatics will be changed by removing PreserveRegExpStatics.

The value of isRestoredFromModifiedMatch is checked on all static property access (DEFINE_STATIC_GETTER), and it corrects telemetry for affected case.  In part 1, checkRestoredFromModifiedMatch doesn't return false, but part 2 adds that case.

About the comment for DeprecatedRestoredRegExpStatics in jscompartment.h, I don't know which version adds this restoring feature.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add console warning for RegExp static property access after String.prototype.replace with function argument and RegExp static property is changed inside it.

Added warning for the affected property access.  The warning message might be still unclear.  If you have better idea, please tell me :)

Comment 13

[Till Schneidereit [:till]]

Comment on attachment 8667826 [details] [diff] [review]
Part 1: Add telemetry for RegExp static property access after String.prototype.replace with function argument and RegExp static property is changed inside it.

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

Very nice, thank you.

Requesting data collection review for the new telemetry probe value.

Reason for adding the value: we're trying to remove some non-standard behavior and want to verify our assumption that it's a non-breaking change.

::: js/src/builtin/RegExp.cpp
@@ +600,5 @@
>          CallArgs args = CallArgsFromVp(argc, vp);                               \
>          RegExpStatics* res = cx->global()->getRegExpStatics(cx);                \
>          if (!res)                                                               \
>              return false;                                                       \
> +        if (!res->checkRestoredFromModifiedMatch(cx))                           \

Remove the result check and just call the function here. See next comment.

::: js/src/vm/RegExpStatics.cpp
@@ +128,5 @@
> +            const char* filename = script->filename();
> +            cx->compartment()->addTelemetry(filename, JSCompartment::DeprecatedRestoredRegExpStatics);
> +        }
> +    }
> +    return true;

This function always returns true right now. That's fine: we shouldn't make the failed attempt to report telemetry cause exceptions. Just make it void and remove the check in RegExp.cpp.

::: js/src/vm/RegExpStatics.h
@@ +49,5 @@
> +    /* ID for each match result, incrementing on each update. */
> +    size_t matchId;
> +
> +    /*
> +     * Match result is restored from differnt ID in String.prototype.replace

I would replace this entire comment with something like "Used to report telemetry when RegExp static properties are restored under circumstances where other engines don't. See bug 1208835 for details."

Comment 14

[Till Schneidereit [:till]]

Comment on attachment 8667827 [details] [diff] [review]
Part 2: Add console warning for RegExp static property access after String.prototype.replace with function argument and RegExp static property is changed inside it.

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

I'm not convinced we should warn about this. Ideally the warning shouldn't ever be shown, and it's hard to understand what is being warned about.

I'd favor just waiting for telemetry results and ideally just ripping this all out.

Comment 15

[Tooru Fujisawa [:arai]]

Sorry, I just noticed that current JSCompartment::addTelemetry cannot collect telemetry for Add-ons.
We'll have to add another method (maybe addAddOnTelemetry?).

https://dxr.mozilla.org/mozilla-central/rev/5f16c6c2b969f70e8da10ee34853246d593af412/js/src/jscompartment.cpp#1093
> void
> JSCompartment::addTelemetry(const char* filename, DeprecatedLanguageExtension e)
> {
>    // Only report telemetry for web content, not add-ons or chrome JS.
>    if (addonId || isSystem_ || !filename || strncmp(filename, "http", 4) != 0)

Comment 16

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #15)
> Sorry, I just noticed that current JSCompartment::addTelemetry cannot
> collect telemetry for Add-ons.

Sorry, I forgot about that. For the other features taking telemetry for chrome code, too, swamped what websites reported.
> We'll have to add another method (maybe addAddOnTelemetry?).

That'd be great! We should have that for all of the values in this probe.

Comment 17

[Tooru Fujisawa [:arai]]

(In reply to Till Schneidereit [:till] from comment #16)
> We should have that for all of the values in this probe.

Do you mean that we should take telemetry for all DeprecatedLanguageExtensions (like DeprecatedExpressionClosure, DeprecatedFlagsArgument, etc) for Add-ons separately from web content?  maybe by duplicating all of them and call addAddonTelemetry for each?

Comment 18

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #17)
> Do you mean that we should take telemetry for all
> DeprecatedLanguageExtensions (like DeprecatedExpressionClosure,
> DeprecatedFlagsArgument, etc) for Add-ons separately from web content? 
> maybe by duplicating all of them and call addAddonTelemetry for each?

Yes, that is exactly what I mean. Sorry for not being clear about it. I think it should really just be addons, in fact: we can always test chrome code via automation. (Well, our coverage isn't 100%, but it should be good enough.)

Comment 19

[Benjamin Smedberg]

Comment on attachment 8667826 [details] [diff] [review]
Part 1: Add telemetry for RegExp static property access after String.prototype.replace with function argument and RegExp static property is changed inside it.

What I am reviewing in this patch? Can youdescribe what effect this will have on the data we're collecting? Bonus if that's documented using in-tree docs.

Comment 20

[Tooru Fujisawa [:arai]]

This collects how much web content and Add-ons (with bug 1211164) possibly depend on SpiderMonkey-specific behavior of RegExp's static property (described in comment #0 and #1, specifically PreserveRegExpStatics), to find out if/when it's possible to remove it.  This is collected as a part of JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT (and JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_ADDON in bug 1211164).

There are 2 reason to do this, 1st, we could simplify the implementation and improve the performance of ES6 compliant version of RegExp.prototype[@@replace] (in bug 887016) if we could remove PreserveRegExpStatics.  2nd, RegExp static property is not a part of ES6 spec and it behaves differently across browsers, so it would be better to reduce the difference between them.

If there's almost no usage (we expect this), we could remove PreserveRegExpStatics instantly, and the telemetry will also be removed at that point.
If there's some usage, we will have to figure out how to reduce the usage (maybe by warning message, MDN document, blog post, nightly-only change or something else), and track the number of usage to know when we can remove it.
If there's a lot of usage and/or there's no meaningful way to reduce it, we might have to give up removing it.  In that case, this telemetry will also be removed at the point.
Anyway, the telemetry data is needed for the decision.

btw, where is the best place to document this?

Comment 21

[Benjamin Smedberg]

Comment on attachment 8667826 [details] [diff] [review]
Part 1: Add telemetry for RegExp static property access after String.prototype.replace with function argument and RegExp static property is changed inside it.

You need to update Histograms.json with this new value (9). Fortunately the existing histograms definitions have one extra slot, so you don't need to update the definition, just the comment.

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Add telemetry for RegExp static property access after String.prototype.replace with function argument and RegExp static property is changed inside it.

Thank you for reviewing.

Updated description in Histograms.json, based on bug 1211164's patch.

Comment 23

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/644ac87b6f7472f14b34bc277af219ab9d28f1e3
Bug 1208835 - Add telemetry for RegExp static property access after String.prototype.replace with function argument and RegExp static property is changed inside it. r=till,bsmedberg

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/644ac87b6f74