1138325 - Turning RegExp#source from an instance property into an accessor breaks ClojureScript apps

Status: RESOLVED WORKSFORME

(Web Compatibility :: Site Reports, defect)

Description

[Josh Mize [:jgmize]]

Steps to reproduce:

1. Visit http://jamesmacaulay.github.io/zelkova-todomvc/ in developer edition or Nightly
2. Observe app is broken
3. Open developer console
4. "uncaught exception: Invalid match arg: /\./" with an "<unknown>" source
5. Open same website in Beta and Release, note that app is working and no errors in console

1. Visit https://getprismatic.com in developer edition or nightly
2. Log in
3. Observe app is broken
4. Note similar error in console: "uncaught exception: Invalid match arg: /^\/\//"
5. Same website works in Beta and Release with no such error

Comment 1

[Till Schneidereit [:till]]

Sigh. That's a bug in ClojureScript, which was fixed an hour ago:
https://github.com/clojure/clojurescript/commit/d717b4edea074fcfd3e718a6134238ba26f76f82

In short, they detected RegExp instances by testing for `hasOwnProperty('source')`, which isn't true since we moved the flags to accessors.

This will probably affect all slightly more complex ClojureScript projects, so not sure what to do about it.

Jason, do you think we have to back this out for now?

Comment 2

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

Comment 3

[Jason Orendorff [:jorendorff]]

Let's try advocacy first. Keeping it may help get people to upgrade to the version with the bugfix. I don't know how widely deployed ClojureScript is. It's been kicking around for a few years, but maybe it's recent enough that the most important sites using it are still actively developed. Maybe they'll upgrade.

Comment 4

[David Nolen]

So it appears bug reports are rolling in particularly from the dev customers that leverage products like CircleCI.

https://twitter.com/circleci/status/572863636110102530
https://twitter.com/circleci/status/572873470616330240
https://twitter.com/circleci/status/572873715974733824
https://twitter.com/circleci/status/572874187494187010

Here's a list of companies who are using it production:

https://github.com/clojure/clojurescript/wiki/Companies-Using-ClojureScript

Comment 5

[Josh Mize [:jgmize]]

Correct me if I'm wrong, but isn't this backwards-incompatible change a bigger problem than "just" ClojureScript? Won't this break any website that relies on the current behavior? Shouldn't maintaining backwards compatibility with existing sites be a pretty high priority, and justify backing out this change?

Comment 6

[Till Schneidereit [:till]]

(In reply to Josh Mize [:jgmize] from comment #5)
> Correct me if I'm wrong, but isn't this backwards-incompatible change a
> bigger problem than "just" ClojureScript? Won't this break any website that
> relies on the current behavior? Shouldn't maintaining backwards
> compatibility with existing sites be a pretty high priority, and justify
> backing out this change?

Language changes are almost always a balancing act. You don't want to be completely hamstrung by not being able to change the slightest details of how existing parts of the language works, but you also don't want to break lots of content.

In this case yes, this is a backwards-incompatible change, but one that the EcmaScript designers feel/hope we can get away with because the number of sites it breaks is thought to be small enough.

ClojureScript breaking is a pretty strong indicator of trouble, but depending on how that community works, it might well be possible that all or most deployments are updated quickly enough for things to work out.

Note that there are lots of precedents for things like this, so it's not like we're doing something outrageous here.

Comment 7

[Till Schneidereit [:till]]

Moving to evangelism for now. We should reach out to ClojureScript folks to see if they can do a quick release and try to get people to roll that out quickly.

Comment 8

[Hallvord R. M. Steen [:hallvors]]

This is being fixed, right? 
https://github.com/clojure/clojurescript/commit/d717b4edea074fcfd3e718a6134238ba26f76f82

Comment 9

[Till Schneidereit [:till]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #8)
> This is being fixed, right? 

It is, yes. The question is how quickly they will do a new release of ClojureScript and then how quickly that will be deployed in the field. If one of those doesn't happen quickly enough, we have to assess the impact this has on our users and decide whether we need to back out the change that caused the regression. The change is on Developer Edition now, so there is some amount of urgency here.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this will break the web for a likely non-trivial number of users, and we can guesstimate that updates to Clojure will not be as fast across the web as our 6 weeks release cycles are, we'll track this and do a backout in Beta when 38 gets to Beta.

What would be useful would be to test against top websites to see how many are impacted now and then we'd have a line to compare against at a later date when we want to know if we can re-enable it yet.

Kairo is this something we can look to QA to prepare a baseline for?

Comment 11

[Robert Kaiser]

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #10)
> What would be useful would be to test against top websites to see how many
> are impacted now and then we'd have a line to compare against at a later
> date when we want to know if we can re-enable it yet.
> 
> Kairo is this something we can look to QA to prepare a baseline for?

That should really be done in some automated fashion and not through manual QA, I think - at least the step of finding whatever you consider "top websites" that use Clojure at all. I have no knowledge on who would be able to do that automation or look into it nowadays, I'd guess that someone on the A-Team side might?

Comment 12

[Tooru Fujisawa [:arai]]

Do we have to backout entire patch for bug 1120169?
I guess only RegExp.prototype.source part should be enough for this issue, and we can leave other changes on flag accessors untouched, but backing out entire patch might be better for simplicity.
  https://hg.mozilla.org/mozilla-central/rev/9ddd307bb5d1

even in minimum case, bug 1130798, and bug 1130860 should also be backed out, since they touched mainly on RegExp.prototype.source.
  https://hg.mozilla.org/mozilla-central/rev/7b89d8a612eb
  https://hg.mozilla.org/mozilla-central/rev/18240dad751f
  https://hg.mozilla.org/mozilla-central/rev/87d812111396

Comment 13

[Hallvord R. M. Steen [:hallvors]]

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #10)
> Since this will break the web for a likely non-trivial number of users, and
> we can guesstimate that updates to Clojure will not be as fast across the
> web as our 6 weeks release cycles are, we'll track this and do a backout in
> Beta when 38 gets to Beta.

Good idea.

> What would be useful would be to test against top websites to see how many
> are impacted now and then we'd have a line to compare against at a later
> date when we want to know if we can re-enable it yet.

Personally, I don't think I've noticed any site using ClojureScript. Given that I think it's relatively rare, even though I could do a test run of the 4000+ sites we track on arewecompatibleyet.com looking for this error message, I don't think it would tell us much. I suggest you implement a telemetry check instead..

> Kairo is this something we can look to QA to prepare a baseline for?

The Web Compatibility team has infrastructure to answer such questions (run and maintained by me). Next time we may even be able to answer such things based on saved test run data - but we're not that far yet.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Personally, I don't think I've noticed any site using ClojureScript.

We've had an indepedent report of one in bug 1149604 (by way of bug 1149339).

Comment 15

[Tooru Fujisawa [:arai]]

Tested following 2 cases.

back out bug 1120169 (all accessors), bug 1132295, bug 1130798, and bug 1130860
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8cdba5c8acf

back out bug 1120169 (Only RegExp.prototype.source), bug 1130798, and bug 1130860
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ef45d2dc4d1

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> Let's try advocacy first. Keeping it may help get people to upgrade to the
> version with the bugfix. I don't know how widely deployed ClojureScript is.
> It's been kicking around for a few years, but maybe it's recent enough that
> the most important sites using it are still actively developed. Maybe
> they'll upgrade.

Advocacy for libraries that require client updates is generally a slow process. Not only do you need to get a fix in the library (already done) but you generally need to speak with all clients to get them to update too.

(In reply to Till Schneidereit [:till] from comment #6)
> In this case yes, this is a backwards-incompatible change, but one that the
> EcmaScript designers feel/hope we can get away with because the number of
> sites it breaks is thought to be small enough.

Do we have any data to support this thought?
 
> ClojureScript breaking is a pretty strong indicator of trouble, but
> depending on how that community works, it might well be possible that all or
> most deployments are updated quickly enough for things to work out.

I'd much rather have an idea of what we're going to break rather than just release and find out. We've done that with other features and it doesn't always end well.

> Note that there are lots of precedents for things like this, so it's not
> like we're doing something outrageous here.

We haven't always made the best decisions in the past. The fact that people reported the site bustage on a channel before Beta is a pretty big red flag to me.

(In reply to Hallvord R. M. Steen [:hallvors] from comment #13)
> (In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #10)
> > Since this will break the web for a likely non-trivial number of users, and
> > we can guesstimate that updates to Clojure will not be as fast across the
> > web as our 6 weeks release cycles are, we'll track this and do a backout in
> > Beta when 38 gets to Beta.
> 
> Good idea.

I agree. Looks like this may have been backed out as per comment 15.

arai - Can you please confirm?
 
> The Web Compatibility team has infrastructure to answer such questions (run
> and maintained by me). Next time we may even be able to answer such things
> based on saved test run data - but we're not that far yet.

Assuming that we have or are going to back this out for 38 I think it is still worth trying to get data on this change so that we can go into this with our eyes open. 

Hallvord - Do you have time to set this up?

Comment 18

[Tooru Fujisawa [:arai]]

Not yet backed out, comment #15 is try run.

What steps are needed to backout changesets in beta branch?
review and approval?

Comment 19

[Till Schneidereit [:till]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #17)
> (In reply to Till Schneidereit [:till] from comment #6)
> > In this case yes, this is a backwards-incompatible change, but one that the
> > EcmaScript designers feel/hope we can get away with because the number of
> > sites it breaks is thought to be small enough.
> 
> Do we have any data to support this thought?

No. I probably didn't make that clear enough, but I do in fact think that this case needs at least quite a bit of delay before it can be shipped on release.
>  
> > ClojureScript breaking is a pretty strong indicator of trouble, but
> > depending on how that community works, it might well be possible that all or
> > most deployments are updated quickly enough for things to work out.
> 
> I'd much rather have an idea of what we're going to break rather than just
> release and find out. We've done that with other features and it doesn't
> always end well.

I fully agree.

> 
> > Note that there are lots of precedents for things like this, so it's not
> > like we're doing something outrageous here.
> 
> We haven't always made the best decisions in the past. The fact that people
> reported the site bustage on a channel before Beta is a pretty big red flag
> to me.

Yes, certainly. For me the only question is whether we should back it out on Beta only, in the hopes of web devs using dev edition and fixing things, or if it should be backed out there, too.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Move source property to RegExp instance again.

As discussed on IRC, backed out only source property, keeping bug 1130798 and bug 1130860 applied.

EscapeRegExpPattern is now called in CompileRegExpObject, there previously EscapeNakedForwardSlashes was called.
And empty strings are replaced by emptyRegExp "(?:)" in some places, to avoid storing empty string to SOURCE_SLOT bypassing EscapeRegExpPattern (RegExp.prototype and `new RegExp()` with no arguments).
Other flag accessors are not touched.

Seems no regression related to this patch happened in try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f03b14d9380f

Comment 21

[Hallvord R. M. Steen [:hallvors]]

> Assuming that we have or are going to back this out for 38 I think it is
> still worth trying to get data on this change so that we can go into this
> with our eyes open. 
> 
> Hallvord - Do you have time to set this up?

Sort of..
I scanned 63 sites listed as users of ClojureScript on https://github.com/clojure/clojurescript/wiki/Companies-Using-ClojureScript, and the URL in this bug. Here's an excerpt of this log:

Presumably ClojureScript http://jamesmacaulay.github.io/zelkova-todomvc/js/app.js
Broken ClojureScript seen! http://jamesmacaulay.github.io/zelkova-todomvc/js/app.js

Presumably ClojureScript http://www.8thlight.com/
Broken ClojureScript seen! http://www.8thlight.com/assets/eighthlight-200eb63b72445bdaca9c38e9e19f7b86.js

Now opening https://www.cognician.com/
Broken ClojureScript seen! https://s3.amazonaws.com/cognician-static/js/elf/elf.js

Now opening http://www.mttmarket.com/
Broken ClojureScript seen! https://mttmarket.com/static/js/main.1dd1b413b2c4d45ca702dc844feca5d6d061908d.js

Now opening http://getprismatic.com/home
Broken ClojureScript seen! http://cdn.getprismatic.com/assets/target/js/webapp-196c067d4a21d0528aec38e91d37a1d9.js

Now opening https://racehubhq.com/
Broken ClojureScript seen! https://racehubhq.com/bundles/698ea59dde23/app.js

Now opening http://www.smartchecker.de/
Broken ClojureScript seen! http://www.smartchecker.de/assets/smartchecker/js/1424864333569-common.js

Now opening http://www.sparrho.com/
Broken ClojureScript seen! http://s3-eu-west-1.amazonaws.com/sparrho-static/js/app/cljs.compiled.js

Now opening http://www.nemcv.com/
Broken ClojureScript seen! http://www.nemcv.com/ojobs2.js

Now opening https://opensensors.io/
Broken ClojureScript seen! https://opensensors.io/cljs//src/clojure/string.js

Now opening https://paddleguru.com/
Broken ClojureScript seen! https://d25d8zmroho46r.cloudfront.net/bundles/5e55a8d895a1/guru.js

So 11 sites with old ClojureScript. There are 7 sites with presumably updated versions (the word 'clojure' in source but not hasOwnProperty("source").. The remaining 45 sites had no 'clojure' in source. They might use ClojureScript on a non-front page part of the site, or internally.

Comment 22

[Till Schneidereit [:till]]

Comment on attachment 8586702 [details] [diff] [review]
Move source property to RegExp instance again.

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

Thanks! Does this apply to Beta as-is? If so, just set the approval flag for beta and sheriffs will land it after approval.

Comment 23

[Tooru Fujisawa [:arai]]

Moved the patch to bug 1150297
(this bug seems not to be appropriate place to fix the code)

Comment 24

[Hallvord R. M. Steen [:hallvors]]

My gut feeling at this point is that the ClojureScript problem is quite limited and can be handled with evangelism. *If* we find similar problems in other libraries we'll have more issues of course. It's great it was discovered so early, but the reason may just be that people who are interested in cutting-edge stuff are more likely to both check out Clojure-based developments and run nightly builds. 

It would be interesting to see the change in a Beta..

Comment 25

[Hallvord R. M. Steen [:hallvors]]

(I've opened issues on webcompat.com for contacting these sites, it seems all of them are pretty approachable)

Comment 26

[Sylvestre Ledru [:Sylvestre]]

As bug 1150297 is fixed, I guess this bug no longer affects 38.

Comment 27

[Julien Wajsberg [:julienw]]

Just a notice that getprismatic.com is not fixed yet. It's interesting to note that I got one report today that blamed Firefox instead of the website.

The webcompat link is https://webcompat.com/issues/847, it seems nobody contacted them yet?

Comment 28

[Julien Wajsberg [:julienw]]

Just contacted them over Twitter.
https://twitter.com/jwajsberg/status/591169944798240770

Comment 29

[Mike Taylor [:miketaylr]]

> The webcompat link is https://webcompat.com/issues/847, it seems nobody contacted them yet?

Thanks for reaching out! Our team is a small operation (2.5 staff)--so your help doing outreach is greatly appreciated.

Comment 30

[Mike Taylor [:miketaylr]]

Prismatic is now aware of the issue, so hopefully that will be fixed soon: https://github.com/webcompat/web-bugs/issues/847#issuecomment-95669112

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Hallvord, do you have a sense of how widespread this problem is?  Would it be worth checking against a list of top websites? 

Is there any sort of workaround that sites can deploy if they aren't in a position to quickly update ClojureScript?

Can you suggest a release note for 39 if we know that this isn't fixable on our side?

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Till, it looks like your patch never landed in 39.  Do you think it will help? Can we land this on 40, and test it there?

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:

Comment 34

[Kohei Yoshino]

I have documented this on the 39 compat doc so far.
https://developer.mozilla.org/en-US/Firefox/Releases/39/Site_Compatibility#JavaScript

Comment 35

[Hallvord R. M. Steen [:hallvors]]

(In reply to Liz Henry (:lizzard) from comment #31)
> Hallvord, do you have a sense of how widespread this problem is?  Would it
> be worth checking against a list of top websites? 

My gut feeling is that ClojureScript isn't *that* widely used - so rather than checking a list of top sites, I did some more targeted testing based on the ClojureScript project's list of sites that use their library. 

This turned up 11 sites ( https://github.com/webcompat/web-bugs/issues?utf8=%E2%9C%93&q=is%3Aissue+clojure )

> Is there any sort of workaround that sites can deploy if they aren't in a
> position to quickly update ClojureScript?

The short answer is "nothing elegant". The longer answer is that you *could* hack Object.prototype.hasOwnProperty() to return true for 'source' when its this object is instanceof regexp - but it is a pretty hackish workaround. It would likely look something like this:

(function(hasownprop){
    Object.prototype.hasOwnProperty = function(str){
        if(this instanceof RegExp && str === ¨'source'){
            return true;
        }
        return hasownprop.call(this,str);
    }
})(Object.prototype.hasOwnProperty)

Caveat: untested..

> Can you suggest a release note for 39 if we know that this isn't fixable on
> our side?

Kohei wrote this already :)

Comment 36

[jen strickland :jenstrickland]

Enabled tracking for 40.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

I think at this point, we will wontfix this for 39.  Can you aim for 40 or 41?  If someone ends up wanting to land it on 39 just let me know and I can reconsider.

Comment 38

[Tooru Fujisawa [:arai]]

Checked the site listed in comment #21 again.
So far, only one site is fixed, I don't see ClosureScript in two, and others are still using old ClosureScript, especially one hits the bug :(

Fixed
  * page:   http://www.smartchecker.de/
    script: http://www.smartchecker.de/assets/smartchecker/js/1424864333569-common.js

No ClosureScript found (might be wrong)
  * page:   http://www.8thlight.com/
  * page:   http://getprismatic.com/home

Not Fixed, but doesn't hit the bug when opening the page (not exhaustive test)
  * page:   http://jamesmacaulay.github.io/zelkova-todomvc/js/app.js
    script: http://jamesmacaulay.github.io/zelkova-todomvc/js/app.js
  * page:   https://www.cognician.com/
    script: https://s3.amazonaws.com/cognician-static/js/elf/elf.js
  * page:   http://www.mttmarket.com/
    script: https://mttmarket.com/static/js/main.d7f3d9c94d29fd87947e6d28c1ca270178e073c7.js
  * page:   http://www.sparrho.com/
    script: http://s3-eu-west-1.amazonaws.com/sparrho-static/js/app/cljs.compiled.js
  * page:   http://www.nemcv.com/
    script: http://www.nemcv.com/ojobs2.js
  * page:   https://opensensors.io/
    script: https://opensensors.io/cljs//src/clojure/string.js
  * page:   https://paddleguru.com/
    script: https://d25d8zmroho46r.cloudfront.net/bundles/9b2e6af8fcb7/guru.js

Not Fixed, hits the bug when opening the page, and the page contents aren't shown correctly
  * page:   https://racehubhq.com/
    script: https://d3vv3l5q1bcq9w.cloudfront.net/bundles/99ed9726f13c/app.js
    exceptions:
      uncaught exception: Invalid match arg: /^\/\// <unknown>
      uncaught exception: Invalid match arg: /[!'()]/

Also, from the telemetry added by bug 1153963, Using RegExp#source with hasOwnProperty or getOwnPropertyDescriptor (RegExpSourceProp=8) hits 45 times on Nightly 40, 142 times on Aurora 40, and 54 times on Nightly 41.
  40 Nightly: http://telemetry.mozilla.org/#filter=nightly%2F40%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=multiselect-all!Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table
  40 Aurora: http://telemetry.mozilla.org/#filter=aurora%2F40%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=multiselect-all!Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table
  41 Nightly: http://telemetry.mozilla.org/#filter=nightly%2F41%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=multiselect-all!Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table

Comment 39

[Julien Wajsberg [:julienw]]

For prismatic, the new URL is http://getprismatic.com/reader.

Comment 40

[Tooru Fujisawa [:arai]]

(In reply to Julien Wajsberg [:julienw] from comment #39)
> For prismatic, the new URL is http://getprismatic.com/reader.

Thanks, so, it's not fixed, and the page contents aren't shown correctly.
  page: http://getprismatic.com/reader
  script: http://cdn.getprismatic.com/assets/target/js/webapp-3f147de28e6ae31f093014697fedcc82.js
  exception:
    uncaught exception: Invalid match arg: /^\/\//

Comment 41

[Sylvestre Ledru [:Sylvestre]]

I am going untrack it as it doesn't seem to be that critical and we are still tracking bug 1172790

Comment 42

[Mike Taylor [:miketaylr]]

FWIW, getprismatic updated their site and is working. cf., https://github.com/webcompat/web-bugs/issues/847#issuecomment-116830781

Comment 43

[Mike Taylor [:miketaylr]]

mttmarket.com updated their version of Clojurescript: https://github.com/webcompat/web-bugs/issues/848#issuecomment-124158589

Comment 44

[Tooru Fujisawa [:arai]]

Checked websites listed in comment #21 and bug 1172790 again.
so, now 5 sites are fixed, I don't see ClosureScript in one, and others still using old one, and bug 1172790 hits the bug.

Already fixed (including comment #42 and comment #43)
  * page:   http://www.smartchecker.de/
    script: http://www.smartchecker.de/assets/smartchecker/js/1424864333569-common.js
  * page:   http://getprismatic.com/reader
    script: http://cdn.getprismatic.com/assets/target/js/webapp-afdb930e330b5572140a8b73a18d7b3b.js
  * page:   http://www.mttmarket.com/
    script: https://mttmarket.com/static/js/main.8a55584e64c9ed598f651f0d7b0f189c.js

Newly fixed
  * page:   http://www.sparrho.com/
    script: http://s3-eu-west-1.amazonaws.com/sparrho-static/js/app/supper.compiled.js
    (http://s3-eu-west-1.amazonaws.com/sparrho-static/js/app/cljs.compiled.js still exists with not-fixed code, but seems not used any more)
  * page:   https://racehubhq.com/
    script: https://d3vv3l5q1bcq9w.cloudfront.net/bundles/758f31e5ecef/app.js

No ClosureScript found (might be wrong)
  * page:   http://www.8thlight.com/

Not Fixed, but doesn't hit the bug when opening the page (not exhaustive test)
  * page:   http://jamesmacaulay.github.io/zelkova-todomvc/js/app.js
    script: http://jamesmacaulay.github.io/zelkova-todomvc/js/app.js
  * page:   https://www.cognician.com/
    script: https://s3.amazonaws.com/cognician-static/js/elf/elf.js
  * page:   http://www.nemcv.com/
    script: http://www.nemcv.com/ojobs2.js
  * page:   https://opensensors.io/
    script: https://opensensors.io/cljs//src/clojure/string.js
  * page:   https://paddleguru.com/
    script: https://d25d8zmroho46r.cloudfront.net/bundles/301b5f26ce26/guru.js

Not Fixed, hits the bug when opening the page, and the page contents aren't shown correctly
  * page:   https://homescreen.is/
    script: https://homescreen.is/js/main.js
    (bug 1172790)

from the telemetry added by bug 1153963, Using RegExp#source with hasOwnProperty or getOwnPropertyDescriptor (RegExpSourceProp=8) hits 64 times on 41 Aurora, 75 times on 41 Nightly, 10 times on 42 Nightly.
  41 Aurora: http://telemetry.mozilla.org/advanced/#filter=aurora%2F41%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table
  41 Nightly: http://telemetry.mozilla.org/advanced/#filter=nightly%2F41%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table
  42 Nightly: http://telemetry.mozilla.org/advanced/#filter=nightly%2F42%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table

I'm going to prepare backport for 40 Beta, in bug 1150297.

Comment 45

[Tooru Fujisawa [:arai]]

Checked websites listed again.
Now 6 sites are fixed, I don't see ClosureScript in one, and others still using old one, but at least I don't see the page hitting the error.

Already fixed
  * page:   http://www.smartchecker.de/
    script: http://www.smartchecker.de/assets/smartchecker/js/1439804031329-common.js
  * page:   http://getprismatic.com/reader
    script: http://cdn.getprismatic.com/assets/target/js/webapp-7d02c7690735074fd7d440edac0dd8fd.js
  * page:   http://www.mttmarket.com/
    script: https://mttmarket.com/static/js/main.859958b1a89a6bf7dab12dd0e026fd00.js
  * page:   http://www.sparrho.com/
    script: http://s3-eu-west-1.amazonaws.com/sparrho-static/js/app/supper.compiled.js
    (http://s3-eu-west-1.amazonaws.com/sparrho-static/js/app/cljs.compiled.js still exists with not-fixed code, but seems not used any more)
  * page:   https://racehubhq.com/
    script: https://d3vv3l5q1bcq9w.cloudfront.net/bundles/f460d0a8da56/app.js

Newly fixed
  * page:   https://homescreen.is/
    script: https://homescreen.is/js/main.js
    (bug 1172790)

No ClosureScript found (might be wrong)
  * page:   http://www.8thlight.com/

Not Fixed, but doesn't hit the bug when opening the page (not exhaustive test)
  * page:   http://jamesmacaulay.github.io/zelkova-todomvc/js/app.js
    script: http://jamesmacaulay.github.io/zelkova-todomvc/js/app.js
  * page:   https://www.cognician.com/
    script: https://s3.amazonaws.com/cognician-static/js/elf/elf.js
  * page:   http://www.nemcv.com/
    script: http://www.nemcv.com/ojobs2.js
  * page:   https://opensensors.io/
    script: https://opensensors.io/cljs//src/clojure/string.js
  * page:   https://paddleguru.com/
    script: https://d25d8zmroho46r.cloudfront.net/bundles/278e8a33d48c/guru.js

from the telemetry added by bug 1153963, Using RegExp#source with hasOwnProperty or getOwnPropertyDescriptor (RegExpSourceProp=8) hits 52 times on 42 Aurora, 21 times on 42 Nightly, 8 times on 43 Nightly.
(data for 42 Aurora and 43 Nightly is not yet comparable with previous ones tho)
  42 Aurora: https://telemetry.mozilla.org/advanced/#filter=aurora%2F42%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table
  42 Nightly: https://telemetry.mozilla.org/advanced/#filter=nightly%2F42%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table
  43 Nigthly: https://telemetry.mozilla.org/advanced/#filter=nightly%2F43%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT%2Fsaved_session%2FFirefox&aggregates=Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Table

I'll prepare backport for 41 Beta just in case, but I think there's no more "known-broken-if-we-ship-it" site, is it? (bug 1172790 comment #7)

Comment 46

[Till Schneidereit [:till]]

Jason, based on the stats in comment 45, do you agree that we should probably just ship this, or at least give it another try on beta?

Comment 47

[Jason Orendorff [:jorendorff]]

(In reply to Till Schneidereit [:till] from comment #46)
> Jason, based on the stats in comment 45, do you agree that we should
> probably just ship this, or at least give it another try on beta?

Let it ship to beta.

(If "just ship this" means backporting a patch enabling the change to beta, let me just say, that sounds like a horrible idea! Features do not jump trains, and compatibility concerns are a reason for more caution, not less.)

Comment 48

[Jason Orendorff [:jorendorff]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #17)
> (In reply to Jason Orendorff [:jorendorff] from comment #3)
> > Let's try advocacy first. Keeping it may help get people to upgrade to the
> > version with the bugfix. I don't know how widely deployed ClojureScript is.
> > It's been kicking around for a few years, but maybe it's recent enough that
> > the most important sites using it are still actively developed. Maybe
> > they'll upgrade.
> 
> Advocacy for libraries that require client updates is generally a slow
> process.

Understatement--generally it's impossible. But in this case, Till, Hallvord, and I read the signs and concluded it could be done. It only took 6 months!

> I'd much rather have an idea of what we're going to break rather than just
> release and find out.

Yeah, we should invest to make it cheaper, in the future, to obtain the kind of data we wish we could get today. I think we are actually doing this. But there will always be uncertainty around this kind of change, because even the capabilities we *wish* we had still have issues:

-   have a testing framework for spidering the web in a custom build of Firefox,
    with telemetry to detect when a web page triggers the difference in behavior
    (writing the custom telemetry is still not really cheap; much web content
    is behind a login, so auth is a concern; false negatives and false positives
    still happen)

-   like the above, but instead of spidering, have a large corpus of web content
    so we can do it all offline (same issues)

-   have a very large corpus of saved web content and just search it statically
    for signs of trouble (extremely imprecise, both false negatives and false
    positives galore)

Obtaining the caliber of data that would make you feel really confident is costly and is going to stay costly. :-|

Comment 49

[Till Schneidereit [:till]]

(In reply to Jason Orendorff [:jorendorff] from comment #47)
> Let it ship to beta.
> 
> (If "just ship this" means backporting a patch enabling the change to beta,
> let me just say, that sounds like a horrible idea! Features do not jump
> trains, and compatibility concerns are a reason for more caution, not less.)

To be clear, this is on beta right now. The question is whether we should land the patches arai prepared in bug 1150297 for beta 41 to disable the change again, like we did for the last two trains. If we do nothing, this ships in release without jumping of trains. Because that would indeed be a horrible idea, yes. See bug 1150297 comment 36 where arai explains the current situation excellently.

Comment 50

[Jason Orendorff [:jorendorff]]

Ship it.

Comment 51

[Till Schneidereit [:till]]

It seems like all or most of the affected sites have updated ClosureScript to something that's not affected. In any case, there's nothing actionable left here.

Comment 52

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/source should be up-to-date, clearing ddn.