Memory leak with dynamic styles

[qhxin]

Expected behavior:
My APP must render very frequently by our realtime data.

Describe the bug:
My APP has memory leak on Firefox, after using material-ui.
This is the same issue in material-ui mui/material-ui#21528

Codesandbox link:
https://codesandbox.io/s/dazzling-visvesvaraya-eczqe?file=/src/Tab.js

Versions (please complete the following information):

• jss: "^10.0.3"
• Browser [e.g. chrome, safari]: firefox
• OS [e.g. Windows, macOS]: macOS
  Feel free to add any additional versions which you may think are relevant to the bug.

[qhxin]

the leak selector is

          "&:not(:first-child)": {
            borderLeft: "none"
          }

and i have tried &:first-child or &:last-child as the selector, the memory leak is continue.

[kof]

will it happen if you just put this?:

          "&": {
              borderLeft: "none"
          }

[kof]

Can you reproduce it without using react and material-ui? If the assumption is that it's exactly this rule, it means you only need jss core and jss-plugin-nested for reproduction.

If we don't have a bare minimum for reproduction we are unlikely to fix it.

[qhxin]

will it happen if you just put this?:

          "&": {
              borderLeft: "none"
          }

yeah,

 "&": {
}

this selector is the same issue.

[qhxin]

Can you reproduce it without using react and material-ui? If the assumption is that it's exactly this rule, it means you only need jss core and jss-plugin-nested for reproduction.

If we don't have a bare minimum for reproduction we are unlikely to fix it.

i reproduced it in this sandbox: https://codesandbox.io/s/strange-hill-z3gwr?file=/index.html
the sheet leak when the dynamic style function update is processing.

[kof]

@qhxin good catch, I can see the bug is somewhere between these two plugins jss-plugin-rule-value-function and jss-plugin-nested, do you or anyone else want to fix it?

[Jokcy]

@kof It seems jss-plugin-nested add a new Rule to RuleList everytime when update, without delete old rule. It make RuleList.index increase evenytime when update.

I want to help for this. Now I need to know if there is anyway to remove old useless rule when add, or I have to start with it.

[Jokcy]

@kof I try to read the code, I can't quite understand about this code:

let key = name
    if (name in this.raw) {
      key = `${name}-d${this.counter++}`
    }

when addRule to RuleList, even if the same rule name exist, it just create a new name, this make same rule add to rule list every time. Why we need to do like this?

[kof]

@Jokcy right above that line there is the comment:

    // When user uses .createStyleSheet(), duplicate names are not possible, but
    // `sheet.addRule()` opens the door for any duplicate rule name. When this happens
    // we need to make the key unique within this RuleList instance scope.

Is this not explaining it?

[Jokcy]

Yeah, I already read that, my quesstion actually is about it, why we need to keep rules with same name? I just start to read jss code yesterday, so I did not catch a lot of detail now.

[kof]

Mainly because as a user you may call sheet.addRule() from where you are not aware if there is another rule with the same name already there, so the idea was to allow doing so while resolving name conflicts automatically.

I wonder if there is no way in our case here to identify the same rule (without stringification)

[kof]

Also feel free to locally disable this line and run the tests to see what other use cases will be failing.

[Jokcy]

@kof I'm wondering if we can just add another API to fix this. In this API we won't do:

if (name in this.raw) {
    key = `${name}-d${this.counter++}`
}

This API just for plugins and people who know this issue. I tested in the codesandbox demo, it did work, but I got some test cases need to fix.

I need some advice to know if I went into bad direction.

[kof]

@naripok you are refering to #1381 it would make sense to read the PR and the comments

[qhxin]

cssinjs/jss would be eliminated by material-ui...

mui/material-ui#22342

[kof]

#1381 (comment)

[seiyab]

I found that we should take care of following to solve the memory leak.

• Of course, we need to solve memory leak rendered in CSSStyleSheet
• And also, we need to solve memory leak in RuleList.index, RuleList.raw, RuleList.map.
• Additionally, we might need to solve memory leak in DOMRenderer.cssRules
• We should write tests for all the above memory leaks.

[kof]

released in https://github.com/cssinjs/jss/releases/tag/v10.8.1