Closed Bug 1412741 Opened 2 years ago Closed 2 years ago

Hanging when signing into IFTTT via Google

Categories

(Core :: DOM: Core & HTML, defect)

57 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox57 --- verified
firefox58 --- verified
firefox59 --- verified

People

(Reporter: mcomella, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

- Signed into a Google account already
- Visit https://ifttt.com/login
- Click "Continue with Google"
- Select the signed in account

Expected: sign in
Actual: dialog closes and the browser starts to hang

This is 57.0b12 on macOS Sierra. I don't know if this affects other platforms.
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171030103605

I have tested your issue on latest Nightly build (Buid ID: 20171030103605) and managed to reproduce it. I have signed into a google account on Gmail and after that I have opened https://ifttt.com/login page in a new tab and signed in with the Google account. The login pop-up window is dismissed but the browser tab hangs.

I did a regression range for the issue and this is the output:
INFO: Narrowed inbound regression window from [d649fe4b, c99d20ba] (3 builds) to [c91796e4, c99d20ba] (2 builds) (~1 steps left)
INFO: No more inbound revisions, bisection finished.
INFO: Last good revision: c91796e4b8bdb6f60c5ae5fc5559e1243d0e42cb
INFO: First bad revision: c99d20ba408a4fd19bab0bc04ff926e73d838fbf
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c91796e4b8bdb6f60c5ae5fc5559e1243d0e42cb&tochange=c99d20ba408a4fd19bab0bc04ff926e73d838fbf

@Boris, can you take a look at this?
Flags: needinfo?(bzbarsky)
Component: General → DOM
Product: Firefox → Core
This is a regression from bug 1381408.

We're ending up in infinite recursion in this function (defined in <https://web-assets.ifttt.com/assets/application-85c1d11cdbad117974cfda954ed63f17569e1e6c21ab2ef5459d1a39fa72fa28.js>) on the page:

m.extend = m.fn.extend = function() {
                    var e, t, n, r, o, i, a = arguments[0] || {},
                        s = 1,
                        u = arguments.length,
                        l = !1;
                    for ("boolean" == typeof a && (l = a, a = arguments[s] || {}, s++), "object" == typeof a || m.isFunction(a) || (a = {}), s === u && (a = this, s--); u > s; s++)
                        if (null != (e = arguments[s]))
                            for (t in e) n = a[t], r = e[t], a !== r && (l && r && (m.isPlainObject(r) || (o = Array.isArray(r))) ? (o ? (o = !1, i = n && Array.isArray(n) ? n : []) : i = n && m.isPlainObject(n) ? n : {}, a[t] = m.extend(l, i, r)) : void 0 !== r && (a[t] = r));
                    return a
                }

In a debugger I see us keep enumerating a property named "window", which is the first cross-origin-readable thing defined on Window.webidl.  Note that a cross-origin object would test true for isPlainObject in that script, afaict.  The key bits are:

   for (t in e) n = a[t]

and

   i = n && m.isPlainObject(n) ? n : {}

and

   a[t] = m.extend(l, i, r))

So as far as I can tell, once this function gets its hands on a cross-origin window it will think that it's a plain object and it should recursively call "extend" on it, which will just land right back in this function with the exact same object.

Of course this function would also fail on any plain-object object graph with loops in it... but they're not passing those in.

It looks to me like this function is actually jQuery.extend as documented at http://api.jquery.com/jQuery.extend/ (at least the overall structure matches what I see in https://code.jquery.com/jquery-3.2.1.js modulo minification).

Given that, I think we should back out bug 1381408 and push back on the spec change.
Assignee: nobody → bzbarsky
Blocks: 1381408
Flags: needinfo?(bzbarsky)
Comment on attachment 8924054 [details] [diff] [review]
Back out the fix for bug 1381408, because that change doesn't look like it's web-compatible

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

Sigh.
Attachment #8924054 - Flags: review?(peterv) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fdce3a0396c
Back out the fix for bug 1381408, because that change doesn't look like it's web-compatible.  r=peterv
Comment on attachment 8924054 [details] [diff] [review]
Back out the fix for bug 1381408, because that change doesn't look like it's web-compatible

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1381408
[User impact if declined]: Hangs or incorrect functionality on some sites.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not particularly.
[Why is the change risky/not risky?]: This is just reverting to the Firefox 55
   behavior, which also matches other browsers.
[String changes made/needed]: None.
Attachment #8924054 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/1fdce3a0396c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1414292
Comment on attachment 8924054 [details] [diff] [review]
Back out the fix for bug 1381408, because that change doesn't look like it's web-compatible

Backout, reverts to 57 behavior, Beta57+
Attachment #8924054 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Build ID: 20171114100042
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

Verified as fixed on Firefox Nightly 59.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Verified as fixed on Firefox 57.0 and Firefox 58.0b3 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Flags: qe-verify+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.