Closed Bug 1172790 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox39 + fixed
firefox38.0.5 --- unaffected
firefox40 + fixed
firefox41 + affected

People

(Reporter: dholbert, Unassigned)

References

()

Details

(Keywords: regression)

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.
[Tracking Requested - why for this release]: Appears to be a new regression in Firefox 39.
> 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...
Blocks: 1120169
Depends on: 1138325
Summary: https://homescreen.is (iOS homescreen screenshot site) is blank in Firefox 39 & newer → https://homescreen.is (iOS homescreen screenshot site) is blank in Firefox 39 & newer, likely due to RegExp#source change
(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 )
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 ?
Flags: needinfo?(bzbarsky)
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.
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.
Flags: needinfo?(bzbarsky)
(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.
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.
n-i to Karl, see comment 6.
Flags: needinfo?(kdubost)
(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.
Flags: needinfo?(kdubost)
Summary: https://homescreen.is (iOS homescreen screenshot site) is blank in Firefox 39 & newer, likely due to RegExp#source change → https://homescreen.is (iOS homescreen screenshot site) is blank in Firefox 39 & newer, due to RegExp#source change
Marking this fixed based on bug 1150297.
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).
Flags: needinfo?(lmandel)
We disabled this change again in 40 in bug 1150297.
Flags: needinfo?(lmandel)
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.
Flags: needinfo?(arai.unmht)
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.
Flags: needinfo?(arai.unmht)
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.
Site owners fixed it by updating the third-party code they were using, so RESOLVED WORKSFORME
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.