Current Function.name implementation breaks various jQuery plugins

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: denschub, Assigned: mrrrgn)

Tracking

({regression})

49 Branch
mozilla50
regression
Points:
---

Firefox Tracking Flags

(firefox48 unaffected, firefox49+ fixed, firefox50+ fixed)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Starting with Firefox 49, the Lufthansa website at http://www.lufthansa.com/online/portal/lh/de/homepage is broken since various JS exceptions are thrown.

In `lh_jquery.min.js`, there are multiple uses of `this.dataAttr` as in `this.settings = h({}, v, this.$form.data(this.dataAttr) || {}, e)` as well as a definition for a getter:

> Object.defineProperty(o.prototype, "dataAttr", {
>   get: function() {
>     return this.jQueryPluginDataAttr;
>   }
> })

In Firefox 48 (and, FWIW, in all competing browsers), those getters get called correctly but in Firefox 49 and tonights Nightly, the getters are ignored and `undefined` is provided, which breaks the Lufthansa website and most certainly other jQuery plugins as well.

This is reproducible on the Lufthansa website, but sadly, I was not yet able to build an isolated test case, so something tricky is going on here.
I don't see any suspicious exception by opening the URL.

When are the exceptions thrown?
When opening the webpage, or after clicking some button?
Does it require login?

Also, what's the exception you're seeing?
Flags: needinfo?(mozilla)
(Reporter)

Comment 2

2 years ago
Interesting. In Nightly/DevEdition, the page is missing its header images and all jQuery plugin elements (like the popdown Login button nor the datepickers on the right side are working. This was initially filed as a webcompat bug, see https://github.com/webcompat/web-bugs/issues/2909 for screenshots. Sorry, should have mentioned that!

I had the initial expression that the non-EU version of this site was not affected by this issue (I'm accessing the site from Germany), but since people from Japan were able to reproduce, I discarded that idea. Do you have any chance testing from an EU VPN or something?

The actual exception thrown is "Error: No CBReplace action given." in lh_jquery.min.js:30:28821, but that's a manually thrown exception that gets thrown if reading the data attribute fails.
Flags: needinfo?(mozilla)
Ah, thanks :)

I confirmed the "Error: No CBReplace action given." (sorry, I haven't thought it's that, as it's thrown only once)

will look into this.
bisection result:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=05fef40f3cebb69aa78e405f2f5cb9fdcd301838&tochange=7e8300ca8b7a50d0a47547f143721d653a887c15

so, it's from bug 1266255.

maybe, the semantics change breaks the assumption in the jQuery plugin?
See Also: → bug 1266255
status-firefox48: --- → unaffected
status-firefox49: --- → affected
status-firefox50: --- → affected
Keywords: regression
(Reporter)

Comment 5

2 years ago
Morgan, since this is a regression from bug 1266255, can you provide some details?
Flags: needinfo?(winter2718)
Here's prettified code for lh_jquery.min.js

http://www.lufthansa.com/layout/r166/static/lh/res/js/lh_jquery.min.js
> function i(t, e) {
>     var i = u(e, 1),
>         r = i[0],
>         o = a.a.isEmpty(t.name) ? t._advisor.orig : t,
>         c = void 0,
>         l = void 0,
>         f = void 0,
>         d = void 0;
>     r && (c = r.name, l = r.selector, f = r.dataAttr, d = r.singleton);
>     var h = a.a.camelCase(c || o.jQueryPluginName || o.name),
>         p = f || o.jQueryPluginDataAttr || n.i(s.a)(h, !0),
>         v = l || o.jQueryPluginSelector || n.i(s.b)(p, "d"),
>         g = d || o.jQueryPluginSingleton || !1;
>     return o.prototype.jQueryPluginName = h, o.prototype.jQueryPluginDataAttr = p, o.prototype.jQueryPluginSelector = v, o.prototype.jQueryPluginSingleton = g, Object.defineProperty(o.prototype, "pluginName", {
>         get: function() {
>             return this.jQueryPluginName
>         }
>     }), Object.defineProperty(o.prototype, "dataAttr", {
>         get: function() {
>             return this.jQueryPluginDataAttr
>         }
>     }), Object.defineProperty(o.prototype, "dataSelector", {
>         get: function() {
>             return this.jQueryPluginSelector
>         }
>     }), Object.defineProperty(o.prototype, "singleton", {
>         get: function() {
>             return this.jQueryPluginSingleton
>         }
>     }), t
> }

there, |a.a.isEmpty(t.name)| checks if the name of the function |t| is empty, and it changes the behavior based on the result.
The function |t| is the following anonymous function.

http://www.lufthansa.com/layout/r166/static/lh/res/js/lh_jquery.min.js
> r = this.advised = function() {
>     function t(t) {
>         var e = c(t);
>         return i._callSimpleAdvice("on", o, t), e
>     }
> 
>     function a(t, e) {
>         i._callSimpleAdvice(t, o, e)
>     }
>     var o, s, u, c, f;
>     this instanceof r ? (o = O(n.prototype), c = function(t) {
>         return l(n, o, t)
>     }) : (o = this, c = function(t) {
>         return n.apply(o, t)
>     }), u = S.call(arguments), f = "afterReturning", s = p({
>         target: o,
>         method: e,
>         args: u
>     });
>     try {
>         i._callSimpleAdvice("before", o, u);
>         try {
>             s.result = i._callAroundAdvice(o, e, u, t)
>         } catch (d) {
>             s.result = s.exception = d, f = "afterThrowing"
>         }
>         if (u = [s.result], a(f, u), a("after", u), s.exception) throw s.exception;
>         return s.result
>     } finally {
>         v()
>     }
> }

Before bug 1266255, |t.name| was empty string, but after bug 1266255, |t.name| is "advised",
so |o| becomes different object for |o = a.a.isEmpty(t.name) ? t._advisor.orig : t|,
and "dataAttr" getter is defined on different object.
[Tracking Requested - why for this release]: Breaks webpages.
tracking-firefox49: --- → ?
tracking-firefox50: --- → ?

Comment 8

2 years ago
This is worrying. We will ask other vendors if they've seen other breakages on the web.

If they have, TC39 needs to relitigate.

If they haven't seen/haven't implemented, we need to back out and collect telemetry for uses of Function.name.
Looks like this specific case is our bug.
SetFunctionName should be called only if the LHS is an identifier.

https://tc39.github.io/ecma262/#sec-assignment-operators-runtime-semantics-evaluation
> 12.15.4 Runtime Semantics: Evaluation
> AssignmentExpression:LeftHandSideExpression=AssignmentExpression
> 1. If LeftHandSideExpression is neither an ObjectLiteral nor an ArrayLiteral,
>    then
>     a. Let lref be the result of evaluating LeftHandSideExpression.
>     b. ReturnIfAbrupt(lref).
>     c. Let rref be the result of evaluating AssignmentExpression.
>     d. Let rval be ? GetValue(rref).
>     e. If IsAnonymousFunctionDefinition(AssignmentExpression) and
>        IsIdentifierRef of LeftHandSideExpression are both true, then
>          i. Let hasNameProperty be ? HasOwnProperty(rval, "name").
>         ii. If hasNameProperty is false, perform
>             SetFunctionName(rval, GetReferencedName(lref)).

so |this.advised = function() {...}| shouldn't set the name of the anonymous function to "adviced".
Status: UNCONFIRMED → NEW
Ever confirmed: true
Tracking 49/50+ since this breaks Lufthansa site, and there may be other sites affected as well.
tracking-firefox49: ? → +
tracking-firefox50: ? → +
(Assignee)

Comment 11

2 years ago
In short, I don't think the current function.name implementation can remain. Which is a huge bummer. 

The problem is, the way we're determining names right now makes it very difficult to determine if the left hand side of an assignment is an identifier reference. Submitting a patch to roll it back in a bit.
tracking-firefox49: + → ?
tracking-firefox50: + → ?
Flags: needinfo?(winter2718)
(Assignee)

Comment 12

2 years ago
Created attachment 8775346 [details] [diff] [review]
backout-fname.diff

This implementation strategy was a dead end.
Attachment #8775346 - Flags: review?(jorendorff)
(Reporter)

Updated

2 years ago
Component: JavaScript: Standard Library → JavaScript Engine
Summary: Getter does not get called in various jQuery plugins → Current Function.name implementation breaks various jQuery plugins
Comment on attachment 8775346 [details] [diff] [review]
backout-fname.diff

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

Thanks.
Attachment #8775346 - Flags: review?(jorendorff) → review+

Comment 15

2 years ago
Pushed by mphillips@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bc93be1406
Backout the existing function.name implementation; r=jorendorff
(Reporter)

Comment 16

2 years ago
Thank you, Morgan!

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6bc93be1406
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 18

2 years ago
Comment on attachment 8775346 [details] [diff] [review]
backout-fname.diff

Approval Request Comment
[Feature/regressing bug #]: 1289749
[User impact if declined]: Broken web pages (see bug).
[Describe test coverage new/current, TreeHerder]: This is a rollback. Tests related to the feature have been removed in the patch. Existing tests which were modified during the addition of the feature were re-added.
[Risks and why]: Minimal risks. Simply reverts a feature.
[String/UUID change made/needed]:
Attachment #8775346 - Flags: approval-mozilla-beta?
Attachment #8775346 - Flags: approval-mozilla-aurora?
Track 49/50 as this is regression.
tracking-firefox49: ? → +
tracking-firefox50: ? → +
Comment on attachment 8775346 [details] [diff] [review]
backout-fname.diff

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

This patch fixes a regression. It's too late for 48 beta. We are about to ship 48. Take it in 49 aurora.
Attachment #8775346 - Flags: approval-mozilla-beta?
Attachment #8775346 - Flags: approval-mozilla-beta-
Attachment #8775346 - Flags: approval-mozilla-aurora?
Attachment #8775346 - Flags: approval-mozilla-aurora+
 https://hg.mozilla.org/releases/mozilla-aurora/rev/fcdf4bb70356
status-firefox49: affected → fixed
Assignee: nobody → winter2718
You need to log in before you can comment on or make changes to this bug.