Closed Bug 1289749 Opened 4 years ago Closed 4 years ago

Current Function.name implementation breaks various jQuery plugins

Categories

(Core :: JavaScript Engine, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- unaffected
firefox49 + fixed
firefox50 + fixed

People

(Reporter: denschub, Assigned: mrrrgn)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

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)
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.
Depends on: 1266255
See Also: 1266255
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.
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.
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.
Flags: needinfo?(winter2718)
This implementation strategy was a dead end.
Attachment #8775346 - Flags: review?(jorendorff)
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+
Pushed by mphillips@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bc93be1406
Backout the existing function.name implementation; r=jorendorff
Thank you, Morgan!
https://hg.mozilla.org/mozilla-central/rev/b6bc93be1406
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
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+
Assignee: nobody → winter2718
You need to log in before you can comment on or make changes to this bug.