1922469 - Fix malfunctioning compatibility issues cache mechanism

Status: REOPENED

(DevTools :: Inspector: Rules, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce

1. Go to https://sanomalearning.design/categories/components/dialog/usage/
2. Open the inspector
3. Click on the "Open dialog" button

-> The inspector freezes

---

Here's a profile I recorded: https://share.firefox.dev/4dCi2RI

It looks like we're spending quite some time in getCSSDeclarationBlockIssues. This could be explained by the high number of CSS variables

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

The issue is that we're sending a lot of declarations to the server and we're spending a lot of time in structuredClone, processing all of those.
Since we're ignoring CSS variables in the server https://searchfox.org/mozilla-release/rev/6e55285b90d6b7aedbcbbd3a9c132552b07c5ba6/devtools/server/actors/compatibility/lib/MDNCompatibility.js#55,57-61

getCSSDeclarationBlockIssues(declarations, browsers) {
...
  for (const { name: property } of declarations) {
    // Ignore CSS custom properties as any name is valid.
    if (property.startsWith("--")) {
      continue;
    }

we can filter the CSS variable declarations from being sent in the first place, and that "fixes" the bug.
This is not fixing the root cause though: while investigating, I was able to see that we had roughly 2000 declarations in the inspector, but we are sending 100 times more (2847088) to the server!

The call to the function is made from each declaration, where we then ask for the rule issues. The calling promise is cached so multiple declarations from the same rule won't trigger multiple calls to the server https://searchfox.org/mozilla-central/rev/f0b3dafb55f1e7904847f5dc889f131040789ba6/devtools/client/inspector/rules/models/rule.js#216-225

async getCompatibilityIssues() {
  if (!this.compatibilityIssues) {
    this.compatibilityIssues =
      this.inspector.commands.inspectorCommand.getCSSDeclarationBlockIssues(
        this.domRule.declarations
      );
  }

  return this.compatibilityIssues;
}

But! the cached promise is cleared from updateEditor https://searchfox.org/mozilla-central/rev/f0b3dafb55f1e7904847f5dc889f131040789ba6/devtools/client/inspector/rules/models/text-property.js#105-113

/**
 * Update the editor associated with this text property,
 * if any.
 */
updateEditor() {
  // When the editor updates, reset the saved
  // compatibility issues list as any updates
  // may alter the compatibility status of declarations
  this.rule.compatibilityIssues = null;

and so here, since we have a lot of editors, it looks like we sometimes call updateEditor on some declaration while for others we called the server, effectively invalidating the cache, and adding duplicated declarations into the array we send.

We should coordinate this a bit better to avoid any issues

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1922469 - [devtools] Avoid sending custom property declarations to getCSSDeclarationBlockIssues. r=#devtools-reviewers.

Those declarations are already ignored on the server, as custom property
declaration can't have compatibility issues.
On pages with lots of CSS variables, we were still paying the price of
iterating over those and sending them through RDP, which could be costly.

Comment 3

[Phabricator Automation]

Comment on attachment 9428798 [details]
Bug 1922469 - [devtools] Avoid sending custom property declarations to getCSSDeclarationBlockIssues. r=#devtools-reviewers.

Revision D224472 was moved to bug 1922511. Setting attachment 9428798 [details] to obsolete.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Bug 1922511 fix is more like a band aid and I wanted to keep this bug open to actually fix the cache mechanism that is malfunctioning (see Comment 1, we might still send more declarations than we need, it's just that you don't usually have cases with tons of non-variable declarations)