1827132 - physiology.org - ~12 seconds loading page, Chrome is fine

Status: VERIFIED FIXED

(Web Compatibility :: Site Reports, defect, P3)

Description

[Julien Wajsberg [:julienw]]

STR:

1. Open https://journals.physiology.org/doi/full/10.1152/japplphysiol.00315.2006
2. Try to interact, for example with Ctrl + to zoom in => nothing happens until the page finishes loading.
3. When scrolling down, at one point there's just white content until the page finishes loading. So clearly the process' main thread is busy until then.

Here is a profile: https://share.firefox.dev/3KKBJvp
It looks like innerHTML is set several times, and this triggers additional behaviors. So it may be a page programming error, but it's good to mention that Chrome doesn't suffer from the same behavior, so there may be some things to do to alleviate the issue.

As a comparison here is an import from a Chrome profile: https://share.firefox.dev/3mcWRBj

Comment 1

[Bas Schouten (:bas.schouten)]

The Performance Impact Calculator has determined this bug's performance impact to be medium. If you'd like to request re-triage, you can reset the Performance Impact flag to "?" or needinfo the triage sheriff.

Platforms: [x] Windows [x] macOS [x] Linux
Impact on site: Causes noticeable jank
Page load impact: Severe
Websites affected: Rare
[x] Able to reproduce locally

Comment 2

[Sean Feng [:sefeng211]]

Profile shows that we call set InnerHTML and then DOMSubTreeModified and then get InnerHTML. And Chrome obviously doesn't behave the same as they don't seem to have this chained behaviour. Everything starts with a JS function called renderBadge. This could be a Firefox bug I think.

I know Emilio is good at demystifying site behaviours, Emilio, can you help here? I've also got a chrome profile here https://share.firefox.dev/3LTArz3

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Julien, I don't know how to interpret that Chrome profile. Do you see Chrome even dispatching DOMSubtreeModified event?

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Yeah my guess is that Chrome is somehow not firing DOMSubtreeModified for this case, but hard to know for sure. The code in that page is terrible, using the .innerHTML getter to check if a DOM node is empty is amazing :'(

Comment 5

[Julien Wajsberg [:julienw]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #3)

Julien, I don't know how to interpret that Chrome profile. Do you see Chrome even dispatching DOMSubtreeModified event?

I can't say for sure. I think we don't import the event names sadly, so this is a bit useless.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

But do you see them in the Chrome's profiler?

Comment 7

[Julien Wajsberg [:julienw]]

Yeah I see a lot of them. Most of them are very small, one of them only is > 50ms and is 56ms.

Let me see if I can hack something to be able to import them in the profiler.

Comment 8

[Julien Wajsberg [:julienw]]

Here is a profile that shows the dom event name from chrome profiles. I imported a new profile taken today from loading the problematic page.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

So the relevant listener looks like this when unminified:

    var z;
    z = {
        holder: $("#doi_altmetric_drawer_area"),
        init: function init() {
            z.control()
        },
        control: function control() {
            z.holder.find(".altmetric-embed").length && z.holder.find(".altmetric-embed").html().length ? z.holder.show() : z.holder.hide(), z.holder.bind("DOMSubtreeModified", z.control)
        }
    }, UX.altmetricsArea = z

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Attempt at a test-case.

This show a variety of differences on mutation event handling between us and Chromium fwiw.

Their jquery bind() call makes this explode quite heavily (see commented out code), but without it we fire two DOMSubtreeModified events on different targets on the initial innerHTML set, which might be the trigger of this.

Chrome seems to fire two mutation events for subsequent innerHTML calls, one for the clearing, one for the "set the children", I'm not sure that's expected, I think our behavior of batching it is probably better.

But Olli, do you know if the #container event is expected? That one feels wrong.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

In any case this seems like a website bug. The reason this explodes is that they're doing control.bind("DOMSubtreeModified", ..) literally every time the event handler runs, and unlike native event handlers, jquery doesn't deduplicate, so...

If I change the snippet in comment 9 by something like:

function (H, R) {
  var z;
  z = {
    holder: $('#doi_altmetric_drawer_area'),
    init: function init() {
      z.control()
      z.holder.bind('DOMSubtreeModified', z.control)
    },
    control: function control() {
      z.holder.find('.altmetric-embed').length && z.holder.find('.altmetric-embed').html().length ? z.holder.show() : z.holder.hide()
    }
  },
  UX.altmetricsArea = z
},

That is, binding on init() rather than control(), the page loads fast as one would expect.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

I contacted them with the suggested fix above fwiw.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

There is no spec for mutation events (because no one should be using them).

I'm not sure what you're asking. DOMSubtreeModified is supposed to bubble and container has the event listener and one of its descendants is changing, so the event should fire, no?

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Yes, but for some reason on my Nightly on Linux I get an extra event targeted directly at #container (not #badge). Though I can't repro on a local build, which is rather odd.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I don't get events targeted to container. I see 4 events targeted to badge.
And for style changes we have this https://searchfox.org/mozilla-central/rev/5c922d8b93b43c18bf65539bfc72a30f84989003/dom/base/nsStyledElement.cpp#113-115

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Oh, that explains what I was seeing, I had that pref flipped locally and the .style.display setter triggers the event, nvm then.

Comment 17

[Jan Jaeschke [:jjaschke]]

Here's a profile from today: https://share.firefox.dev/3WqkQuN

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Now, bug 769207 is fixed. Could somebody to check how the result is changed? Note that if DevTools observes the DOM mutation, we still need to dispatch a chrome event synchronously only when node removals. That's checked only with a bit field, so, that could still affect to the performance if here is a too hot path.

Comment 19

[Calin Tanase]

The issue no longer reproduces with latest Release (v143) and Nightly (v145). The page loads instantly and I can zoom and scroll right away.

Comment 20

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

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 21

[Raul Bucata]

Verified as FIXED using the RC Build

Tested with:

Browser / Version: Firefox 144.0-candidate build 1
Operating System: Windows 10 PRO x64