1750812 - Page load for PagerDuty hangs with spinner (due to JS Array GroupBy proposal causing issue in sugar JS library)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Sven Marnach :sven]

In current Nightly, I can't load the page https://mozilla.pagerduty.com/escalation_policies (which requires to log in first). It works fine in the release version of Firefox and in Chrome, but on Nightly it just hangs showing a spinner. After bringing this up in #webcompat on Slack, people recommended that I bisect the problem with mozregression, which I did. Here are the final lines of the output:

11:23.86 INFO: No more integration revisions, bisection finished.
11:23.86 INFO: Last good revision: 3b63167e1ba02d11da697044acfb3e9da79b2041
11:23.86 INFO: First bad revision: e36262888093c8c65a0a3a36e6b584dc117a2b81
11:23.86 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3b63167e1ba02d11da697044acfb3e9da79b2041&tochange=e36262888093c8c65a0a3a36e6b584dc117a2b81

Comment 1

[Daniel Holbert [:dholbert]]

Ticking needinfo to be sure this is on Yulia's radar, given that this seems likely to be a regression from the patch at https://hg.mozilla.org/integration/autoland/rev/e36262888093c8c65a0a3a36e6b584dc117a2b81 which enabled the Array grouping proposal (bug 1739648).

Comment 2

[Daniel Holbert [:dholbert]]

Also: Sven, it would be handy if you could check for errors/exceptions that could hint at what's going on.

Just open your web console (Ctrl+Shift+K, or Cmd+Shift+K on Mac) and then visit the affected site, and see if any errors/exceptions show up there specifically in Nightly (and not in working builds).

Comment 3

[Yulia Startsev [:yulia]]

Hi Sven -- I unfortunately cannot access that page, I am not a registered user for the pager duty -- how would I get added?. Can you also check if -- when you download the page -- if running the downloaded version also fails? then you could upload it. This might avoid me needing to log in. Otherwise as dholbert mentioned, it would be useful to have the console log output.

I suspect one of two reasons here: a web compatibility issue or (somehow) related to the null prototype issue. Without seeing what is going on though I can't know for sure.

Comment 4

[Sven Marnach :sven]

Yulia, I invited you to PagerDuty with view permissions, so you may be able to look at the page yourself now. If this doesn't work, I will try to look at it myself. Unfortunately, there is a lot of output in the console both in release and in Nightly, so it's not obvious at a glance whether there is any difference.

Comment 5

[Yulia Startsev [:yulia]]

It looks like we may have a webcompat issue: there is a conflict between the groupby proposal and a library used by the site:

In the application code on line 38882:

arr.groupBy('name', function(name, group) {
        group = group.map('text');
        if(name === 'day') group = group.concat(set['weekdays']);
        set[name] = group.join('|');
      });
      set['modifiers'] = arr;
    }

the standard groupby does not take a string as it's first method, but a function. The engine throws at this point due to a typeError.

This may be due to the use of an outdated sugar.js library, which attempts to polyfill the array object with a groupby method: https://sugarjs.com/docs/#/Array/groupBy

In 1.1.2 and other affected versions, it incorrectly skips polyfilling the array:

  function extend(klass, instance, override, methods) {
    var extendee = instance ? klass.prototype : klass;
    storeMethods(klass, instance, methods);
    iterateOverObject(methods, function(name, method) {
      if(typeof override === 'function') {
        defineProperty(extendee, name, wrapNative(extendee[name], method, override));
      } else if(override === true || !extendee[name]) {
        defineProperty(extendee, name, method);
      }
      klass.SugarMethods[name] = { instance: instance, method: method };
    });
  }

In particular this line: else if(override === true || !extendee[name]) -- the extendee (the array class) has defined the groupBy method. We end up skipping redefining it, and it causes the sugar library to break.

This appears to be fixed in the most recent version. I don't know when exactly it was fixed, they had a major rewrite into typescript.
In the previous version, it appears to be fixed by 1.5: https://github.com/andrewplummer/Sugar/blob/0c6f63cc5e13f145a61a2211efef6e21f509578b/lib/core.js#L52-L60

Unfortunately, the new native version of this API does not have the same interface as the old sugar.js, which is why this is breaking. One solution here is to update the dependency pagerduty is using.

Attached is a minimal example of the problem for testing.

Unfortunately, this isn't a bug in firefox's implementation, we just happen to be the first to implement the new featue and see the issue. This will require some discussion. It is a web compat/spec issue. For now, what I will do is add a flag to the feature while we try to determine how wide spread this issue is and whether it requires a rename of the method. You will be able to turn the flag off, and continue to use nightly for this page. In the meantime we will see how bad this breakage is.

Comment 6

[Yulia Startsev [:yulia]]

Attachment: example.html

Comment 7

[Yulia Startsev [:yulia]]

Attachment: sugar.js

Comment 8

[Yulia Startsev [:yulia]]

sven: Do we have a way to get in touch with the developers of this site?

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1739648

Comment 10

[Sven Marnach :sven]

We are a paying customer of PagerDuty, so yes, we can get in touch with them through their support. I haven't dealt with the support yet, but I'd expect them to be open to fix bugs in their code. The email address is [e-mail]. I can also ask whether we have any more direct support channel.

Comment 11

[Yulia Startsev [:yulia]]

Another compat failure here: https://github.com/webcompat/web-bugs/issues/98458

Comment 12

[Yulia Startsev [:yulia]]

Once 1750955 lands, you will be able to turn it off in about:config by searching for "array-grouping". I will work with support to see if they can upgrade their library.

Comment 13

[Yulia Startsev [:yulia]]

Quick update here: The support team for PagerDuty is going to update the library, and is checking the rest of their frontends. The flag has landed, so you should be able to pref off groupBy.

Comment 14

[Yulia Startsev [:yulia]]

pagerduty has updated, Sven, can you confirm that it is working for you now?

Comment 15

[Sven Marnach :sven]

I re-enabled javascript.options.experimental.array_grouping, and PagerDuty is still working for me. Thanks a lot!

Yulia, I'll go ahead and remove your PagerDuty account again.

Comment 16

[Yulia Startsev [:yulia]]

Great, this is resolved then.