1339822 - [Extension bug] EmojiOne Keyboard causes severe page load regression (multiple times longer + unresponsiveness until documentready)

Status: RESOLVED WONTFIX

(WebExtensions :: General, defect)

Description

[Ronan Jouchet [:ronjouch]]

Using the "Emoji Keyboard 2016 by EmojiOne" extension (both version 0.15 and pre-release 0.24) with its enabled-by-default option "Auto-Replace (Replace all emoji in Firefox to EmojiOne)",

- Load time regresses enormously (example: loading SO page [2] takes 14s to load, instead of 2s without the option)

- Page becomes unresponsive until documentready:
  * Cannot close loading tab.
  * Cannot focus or type in input fields.

Also, it seems to me this option is useless since Firefox now bundles EmojiOne and *efficiently* does identical replacement.

I am alerting EmojiOne via the email posted on the extension page ([e-mail]), pointing them here, and am posting this bug in case Mozilla can proactively do something about it (no-op / force-disable the option?)

Configuration: Firefox Developer Edition 53.0a2 (2017-02-15) (64-bit), Ubuntu Linux 16.04.2

[1] https://addons.mozilla.org/en-US/firefox/addon/emoji-keyboard-by-emojione/

[2] http://stackoverflow.com/questions/990904/remove-accents-diacritics-in-a-string-in-javascript

Comment 1

[Alexandre Folle de Menezes]

Same happens here, Firefox 51 on Win10.

Comment 2

[Ronan Jouchet [:ronjouch]]

(In reply to Alexandre Folle de Menezes from comment #1)
> Same happens here, Firefox 51 on Win10.

I got email feedback from Casey at EmojiOne; they are aware of the problem and he mentioned one path they seem to be exploring to work around the issue. It's been some time, though; meanwhile new users of this extension are still getting this option ON by default, suffer nasty performance regression, and blame Firefox :-/

Alexandre, I encourage you to directly contact them by email at [e-mail] and point them to this bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Jorge, is there something we can do about this on our end?

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

I tried reproducing this on my computer (Mac OS X) and couldn't. This is with version 0.24, the latest public version. Granted, there is bound to be *some* additional delay for this add-on, and similar ones that try to make substitutions in all of the text of a page.

I don't see anything that warrants taking action at the policy level on AMO. As for things that could be done in Firefox, I suppose that providing a more efficient web or WebExtensions API to do this particular action would be useful for these add-ons, which are numerous.

Comment 5

[Ronan Jouchet [:ronjouch]]

(In reply to Jorge Villalobos [:jorgev] from comment #4)
> There is bound to be *some* additional delay for this add-on, and
> similar ones that try to make substitutions in all of the text of a page.

Agreed.

> I don't see anything that warrants taking action at the policy level on AMO.

Sad, this is exactly the kind of performance killer happening out of
Mozilla's view, remaining unchecked and contributing to the user perception
that "Firefox is slow", because users have no idea it's due to an addon 

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

Moving to WebExtensions. I doubt there's much we can do here, but ni Mike Conca for ideas or closing this.

Comment 7

[Mike Conca [:mconca]]

> I suppose that providing a more
> efficient web or WebExtensions API to do this particular action would be
> useful for these add-ons, which are numerous.

Since this has to do with content manipulation, a standard WebAPI (not WebExtensions API) makes more sense, although that seems unlikely to happen. Obviously, the better the JavaScript engine, the better the perceived performance.

Interestingly, the developers of this add-on have a much more recent version of it on the Chrome store. It gets terrible reviews and appears it may be a click-bot or ad-injection engine. I wonder if the performance issues a year ago had more to do with that, and they haven't resubmitted to Firefox because of the human-review process on AMO.

Comment 8

[David Durst [:ddurst]]

There's not much we can do. The authors need to address this first.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> There's not much we can do. The authors need to address this first.

What is "this"?  Is there an API they should be using but aren't?  Is there an API they need but are lacking?

We really don't want to have extensions that increase page load times by a factor of 7 (!) for no good reason, as here.

Comment 10

[Kev Needham [:kev]]

Andreas, could you review and get in touch with the developer?

Comment 11

[Andreas Wagner [:TheOne] [use NI]]

All recent submissions have been rejected already for review issues. The remaining public versions, including the latest from 2016, contain review issues and should have been rejected too, also our policies have changed in the meantime. I can reject all versions and let the developer know about this bug.

Should we take additional steps beyond rejecting for the users that have it installed already?

Comment 12

[Andreas Wagner [:TheOne] [use NI]]

I forgot to add live-testing results:

Using the SO test page mentioned in comment 0, with a fresh profile and cleared cache:

Without the add-on:
Load to finish: 2-3s

With the add-on:
Load to finish: 8-9s

(tried three times each)

Comment 13

[David Durst [:ddurst]]

After looking at the code, it seems that the slowdown related to the option being enabled by default is due to the fact that the extension is essentially observing the entire DOM and every change to it. There is no slowdown in the WebExtensions internals, mostly it's in the extension code and platform (DOM and mutation observer).

For existing users, they could set auto-replace to off (in the extension's settings). Whether we take any action beyond that is up to Product.

[Academically, when I was looking at the code, I thought the observer was wasteful and that it could just observe user input controls. But rpl pointed out that not all editable areas are necessarily input controls. Whether there's a better methodology for what they're trying to do is a question, but what they are doing is resulting in a negative performance impact, which is expected in that situation.]

Comment 14

[Boris Zbarsky [:bzbarsky]]

> the extension is essentially observing the entire DOM and every change to it.

Just observing the entire DOM shouldn't have this sort of impact on pageload, I would think.  Are they doing a bunch of expensive work on every mutation?

If it's possible to create a non-extension testcase using mutationobserver in a similar way and that also shows the slowdown, that would be really useful...

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> > the extension is essentially observing the entire DOM and every change to it.
> 
> Just observing the entire DOM shouldn't have this sort of impact on
> pageload, I would think.  Are they doing a bunch of expensive work on every
> mutation?
> 
> If it's possible to create a non-extension testcase using mutationobserver
> in a similar way and that also shows the slowdown, that would be really
> useful...

Hi Boris,
You are definitely right, the extension is not just observing the mutations, but it is synchronously going through the added DOM nodes and doing some work on all of them (which seems to be pretty expensive, at least when it is done from an extension content script).

Here is some profile data collected with the GeckoProfiler on the extension running on the stackoverflow page linked in Comment 0: https://perfht.ml/2LOpNrD

I may be also worth to mention that:

- for arbitrary domain (like this one), the content scripts looks for every added DOM nodes and recursively process the added nodes and their descendant nodes (for other websites the extension seems to have more specific code that doesn't process all the added DOM node)

- near the start of the page loading a div is added by this webpage, which contains basically most of the content of the page (and processing this node and its descendants seems to be triggering the first amount of expensive work)

- this stackoverflow page contains some code snippets and, after the load event has been fired, a stackoverflow script rewrites the DOM elements related to these code snippets, to apply the syntax highlighting (which creates a pretty high number of span and text nodes, that are then detected by the mutation observer and processed as described above).

My guess is that if the same code would be running inside the webpage as non-extension code, it would not be so expensive
e.g. because Gecko wouldn't need to proxy all the calls coming from the content script sandbox on all the DOM objects accessed by the mutation observer.

If it may be helpful, I can attach to this issue a simplified test extension which does most of what the full extension seems to be doing, but with just the minimum amount of that content script code (and a performance impact which seems to be more or less the same one introduced by the original extension).

(another option could be to create another simplified test extension that adds similar code to the page as an additional script element, instead of executing it from the content script sandbox, which should show us if there would be the same kind of impact when doing the same work from a mutation observer that run on that webpage as a non-extension script)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Luca, thank you, that profile is really helpful!  For one thing, it points to exactly which code is slow.

The most relevant function looks like this:

      replaceEmojisInNode: function (node) {
        if (location.hostname.indexOf('twitter') > -1) {
          this.replaceTwitterEmojisInNode(node);
        } else if (location.hostname.indexOf('google') > -1) {
          /* bunch of logic I'll omit */
        } else if (location.hostname.indexOf('youtube.com') > -1) {
          this.replaceYoutubeEmojis(node);
        } else if (location.hostname.indexOf('unicode.org') > -1) {
          this.replaceUnicodeEmojis(node);
        } else if (location.hostname.indexOf('facebook.com') > -1) {
          this.replaceFacebookEmojis(node);
        } else if (location.href.indexOf('web.whatsapp') > -1) {
          this.replaceWhatsAppEmojis(node);
        } else if (location.href.indexOf('instagram.com') > -1) {
          this.replaceInstagramEmojis(node);
        } else if (location.href.indexOf('twitch.tv') > -1) {
          this.replaceTwitchEmojis(node);
        }
        else {
          if (node.nodeType == 3) {
            this.replaceEmojiInTextNode(node);
          } else {
            for (var child in node.childNodes) {
              this.replaceEmojisInNode(child);
            }
          }
        }
      }

The gets of "location" and "location.href" and "location.hostname" pretty much entirely dominate the profile time here.

Some of that is in fact due to running in an extension (e.g. getting the bareword "location" needs to proxy it from the sandbox to the window and then go through the Xray).   But a lot is just because getting things off Location is not very fast (and required to be not-fast by the spec, due to the security checks Location does).

There's a similar problem in the processMutationQueue entry point:

          while (queue.length > 0) {
            var node = queue.pop();
            if (node) {
              if (location.href.indexOf('www.google.com') > -1) {
                this.targetGoogleSearchNodes(node);
              } else {
                this.replaceEmojisInNode(node);
              }
            }

that accounts for most of what's in the profile outside replaceEmojisInNode.

Doing all that "what site am I on?" work on a per-node basis makes no sense, especially when in all the cases except the fallback you only ever do that work on the root node.  So ideally this extension would just get fixed to do those checks once up front, not for every node....

Comment 17

[David Durst [:ddurst]]

Providing NI for Kev from conversation on email.

The consensus is that we should pull this from AMO. It's a demonstrably bad performer, and we've reached out to the developer already. They are active on Chrome, but not on this, probably due to the difference in user counts on the two platforms, so at this point it's developer inaction on developer code that is responsible for the performance. Users shouldn't have to be subject to this, and there are alternatives. [Perhaps removing it will get the developer's attention, even?]

There's another question about blocking this, but I defer that to Andreas and Jorge as to what's appropriate here. It's their case to make if they want to.

NI Andreas for next steps (if there are any) to notify the developer, but recommending WONTFIX.

Comment 18

[Andreas Wagner [:TheOne] [use NI]]

All versions have been rejected and the developer has been notified.

I'd be in favor of blocking the extension due to the significant performance impact (see comment 11), which is explicitly covered in the current blocklisting policies.