Open Bug 1393664 Opened 7 years ago Updated 2 years ago

Add an eslint rule to check if users use functions returning live node lists

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement, P3)

enhancement

Tracking

(Not tracked)

REOPENED

People

(Reporter: lchang, Unassigned)

References

Details

(Whiteboard: [fxperf:p3:responsiveness])

Functions like "getElementsByTagName" return a live node list, which registers a mutation observer that slows down all of the DOM operations happening inside the document from that point on. Users should be notified to use an alternative implementation strategy if possible. Not sure if it's feasible but it would be great to have an eslint rule to ensure that.
There seems to be a lot of existing locations in the tree that do this, including lots on the main windows. We can do a rule to prevent usage of getElementsByTagName, I'm concerned about the amount of whitelist we'd currently need though due to the existing locations. Do we have ongoing efforts to reduce these? (I'm also a bit surprised this hasn't been notified to the mailing lists at least for the short term). What are the alternative options that developers can use?
Priority: -- → P3
Product: Testing → Firefox Build System

I think as this stands, we don't have a good replacement/suggestion for alternatives. Nor do we have any suggestions that we need to work on these/clean these up at this time.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX

(In reply to Luke Chang [:lchang] (inactive) from comment #0)

Functions like "getElementsByTagName" return a live node list, which
registers a mutation observer that slows down all of the DOM operations
happening inside the document from that point on.

This sounds terrible for perf, if the observers don't get removed when the collection object gets GC'd (which is what I'd expect, meaning I wouldn't expect such observers to live very long, pretty much "ever", in Firefox front-end use). Olli, can you confirm if there is a lasting impact to perf as soon as anything creates a live DOM node list in a doc?

If the mutation observer does get removed, I wonder if there's any kind of runtime analysis we could do to check if we leave any lists (and therefore observers) around "long-term" so that mistakes like that get picked up by automated tests... I guess with async functions, it's hard to know what is "long-term", one can easily imagine something like:

let labels = doc.getElementsByTagName("label");
let promises;
for (let l of labels) {
  promises.push(someAsyncOperation(l));
}
await Promise.all(promises);

but perhaps we should outlaw even that with runtime analysis (as I assume the mutation observer will stick around until after labels goes out of scope, and because of the await any code that runs while at least one of the promises is pending is going to be sad...).

(In reply to Mark Banner (:standard8) from comment #2)

I think as this stands, we don't have a good replacement/suggestion for alternatives. Nor do we have any suggestions that we need to work on these/clean these up at this time.

querySelectorAll does not return a live node list, and is pretty much a drop in replacement (though also, some of the calls in frontend use getElementsByTagName("foo")[0] so could just use querySelector in that case!)

Gonna reopen because there's a replacement we can use, and this seems like a valid concern; just curious how severe an issue it is...

Status: RESOLVED → REOPENED
Flags: needinfo?(bugs)
Resolution: WONTFIX → ---
Whiteboard: [fxperf]
Whiteboard: [fxperf] → [fxperf:p3:responsiveness]

When the live lists go away, they are unregistered as being mutation observers.
https://searchfox.org/mozilla-central/source/dom/base/nsContentList.cpp#405-406,539-540

NodeLists' wrappers are also allocated from the nursery, so that the objects are deleted as soon as possible.
https://searchfox.org/mozilla-central/rev/235982cdb78d0dc3ab4e48326b084ce087302cab/dom/webidl/NodeList.webidl#13

But yes, if the list if kept alive, it has overhead, similar to Range object.

Flags: needinfo?(bugs)
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.