1172790 - https://homescreen.is (iOS homescreen screenshot site) is blank in Firefox 39 & newer, due to RegExp#source change

Status: RESOLVED WORKSFORME

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Visit https://homescreen.is/

ACTUAL RESULTS: Blank page.

EXPECTED RESULTS: Non-blank page.


Firefox Beta 39, DevEdition 40, and Nightly 41 give ACTUAL RESULTS.

Firefox release and Chrome 45 (dev edition) give EXPECTED RESULTS.

So it looks like this is a regression that we're about to ship, once beta goes to release.


NOTE: In Firefox versions that trigger the bug, DOM inspector shows that we have an essentially-empty DOM a few zero-height divs, with the innermost one having class="wrapper", and it's empty.  Additionally, I see this in error console:
{
uncaught exception: Invalid match arg: /^/ <unknown>

Security wrapper denied access to property _gaUserPrefs on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ is being gradually removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported. analytics.js:4:0
}

In Firefox release, there's a bunch of elements inside of <div class="wrapper">, and I don't see those error console messages.

Comment 1

[Daniel Holbert [:dholbert]]

[Tracking Requested - why for this release]: Appears to be a new regression in Firefox 39.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> uncaught exception: Invalid match arg: /^/ <unknown>

This is likely the same as bug 1149604 and hence bug 1138325.

It looks like we decided to ship this and damn the consequences, so I guess we need to contact this site and get them to update to a newer clojurescript version or something...

Comment 3

[Daniel Holbert [:dholbert]]

Tweeted at them: https://twitter.com/CodingExon/status/608319652666732545

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Not doing reviews right now from comment #2)
> > uncaught exception: Invalid match arg: /^/ <unknown>
> 
> This is likely the same as bug 1149604 and hence bug 1138325.

(Confirmed w/ mozregression -- regression range is
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3436787a82d0&tochange=2cb22c058add )

Comment 5

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

bz: I may have misunderstood this. It was a confusing batch of bugs trying to fix another (batch?) of regressions. I thought this was only affecting a small handful of sites at this point (https://bugzilla.mozilla.org/show_bug.cgi?id=1138325#c35). 

If we should be reconsidering uplifting this batch of fixes to 39, can you or daniel request uplift again? 

If not you, who should own this ?

Comment 6

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

Talking with Sylvestre now and he suggests backing out the feature from 39, like we did from 38:
https://bugzilla.mozilla.org/show_bug.cgi?id=1150297#c2

Maybe that is an option if it isn't just this site or a few others that break.

Sylvestre also suggested that Karl may want to talk with the site owners.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I, personally, think we should just take the patch in bug 1150297 and approve it for 39.  And keep approving it for beta at least until we get to a point where there are no known-broken-if-we-ship-it sites, if we're making progress on evangelism (as opposed to shipping and knowingly breaking even a small number of sites just because they haven't managed to update yet).  If sites start not wanting to update because we haven't shipped it yet, _then_ we can consider shipping even if we knowingly break it.

> If not you, who should own this ?

There's no much to own here other than the approval decision, unless the patch in bug 1150297 doesn't apply to 39.

Comment 8

[Till Schneidereit [:till]]

(In reply to Not doing reviews right now from comment #2)
> It looks like we decided to ship this and damn the consequences, so I guess
> we need to contact this site and get them to update to a newer clojurescript
> version or something...

The telemetry results arai reported in bug 1138325 comment 38 made it seem like we might be able to pull it off this time. With RegExpSourceProp only showing up 228 times compared to other enum values' 100k+, it seemed like the risk would be low.


(In reply to Not doing reviews right now from comment #7)
> I, personally, think we should just take the patch in bug 1150297 and
> approve it for 39.  And keep approving it for beta at least until we get to
> a point where there are no known-broken-if-we-ship-it sites, if we're making
> progress on evangelism (as opposed to shipping and knowingly breaking even a
> small number of sites just because they haven't managed to update yet).  If
> sites start not wanting to update because we haven't shipped it yet, _then_
> we can consider shipping even if we knowingly break it.

I agree with all of this. We should at least try to do evangelism for all sites we encounter, though. Even then, the risk will remain very high that a small fraction but substantial number of our users will experience breakage once this (or other breaking changes like it) reaches release. I think that we should try to minimize that risk as much as we can, but not give up on all changes that cause breakage in the long tail. Nothing new there, of course.

Comment 9

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

OK. I get it now! Thanks bz and Till for the explanations and for renominating. This was hard to understand because several intertwined bugs seem to turn things on and off. I tried to summarize the situation and what we should do for 40, in bug 1150297.

Comment 10

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

n-i to Karl, see comment 6.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Liz Henry (:lizzard) from comment #10)
> n-i to Karl, see comment 6.

Note that I already reached out to the site, and they replied, saying:
"thanks for the heads up. We can update at some point but might be a while. May not want to ship that into production."
https://twitter.com/homescreenapp/status/608651425300647936

Canceling ni=karl.

Comment 12

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

Marking this fixed based on bug 1150297.

Comment 13

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

Are we planning to ship the RegExp#source change to release? If so, we may break some sites (including this one, which doesn't work in 40).

Comment 14

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

We disabled this change again in 40 in bug 1150297.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tooru, I am n-i'ing you on this bug too with the same question. Do we need to revert back the behavior in Beta41 similar to FF40 (comment 14)? Thanks.

Comment 16

[Tooru Fujisawa [:arai]]

Looks like the issue is now fixed on homescreen.is, yay :D

https://homescreen.is/js/main.js
>   if (b instanceof RegExp) {
>     return a.replace(new RegExp(b.source, "g"), c);
>   }
>   throw [A("Invalid match arg: "), A(b)].join("");

That is the updated ClojureScript's code, and I don't hit the error anymore, the page is shown.

Comment 17

[Tooru Fujisawa [:arai]]

posted recent status on bug 1138325 comment #45, I think it's almost safe to ship it at least for those listed websites, but I'll prepare backport just in case.

Comment 18

[Jason Orendorff [:jorendorff]]

Site owners fixed it by updating the third-party code they were using, so RESOLVED WORKSFORME