1289749 - Current Function.name implementation breaks various jQuery plugins

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dennis Schubert [:denschub]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

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?

Comment 2

[Dennis Schubert [:denschub]]

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.

Comment 3

[Tooru Fujisawa [:arai]]

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.

Comment 4

[Tooru Fujisawa [:arai]]

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?

Comment 5

[Dennis Schubert [:denschub]]

Morgan, since this is a regression from bug 1266255, can you provide some details?

Comment 6

[Tooru Fujisawa [:arai]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Breaks webpages.

Comment 8

[Shu-yu Guo [:shu]]

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.

Comment 9

[Tooru Fujisawa [:arai]]

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".

Comment 10

[Marcia Knous [:marcia]]

Tracking 49/50+ since this breaks Lufthansa site, and there may be other sites affected as well.

Comment 11

[Morgan Phillips [:mrrrgn]]

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.

Comment 12

[Morgan Phillips [:mrrrgn]]

Attachment: backout-fname.diff

This implementation strategy was a dead end.

Comment 13

[Jason Orendorff [:jorendorff]]

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

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

Thanks.

Comment 14

[Morgan Phillips [:mrrrgn]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=263a0deb93dd

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bc93be1406
Backout the existing function.name implementation; r=jorendorff

Comment 16

[Dennis Schubert [:denschub]]

Thank you, Morgan!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6bc93be1406

Comment 18

[Morgan Phillips [:mrrrgn]]

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]:

Comment 19

[Gerry Chang [:gchang]]

Track 49/50 as this is regression.

Comment 20

[Gerry Chang [:gchang]]

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.

Comment 21

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-aurora/rev/fcdf4bb70356