642153 - Validator: Flag DOM mutation events as warnings for their extreme inefficiency

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P3)

Description

[Kris Maglione [:kmag]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b13pre) Gecko/20110310 Firefox/4.0b13pre
Build Identifier: 

I think it would be a good idea to warn unwary developers that using DOM Mutation events are extremely inefficient. They have some uses, but some developers see no issue with inserting them into every page for trivial purposes, which is an extremely bad idea.

Reproducible: Always

Comment 1

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

Agreed. Kris, can you list the events that should be flagged?

Comment 2

[Kris Maglione [:kmag]]

Sure, I just didn't want to go to the trouble if we weren't going to do this.

Any of the events marked deprecated here would be a candidate: http://www.w3.org/TR/DOM-Level-3-Events/

But in particular: DOMAttrModified, DOMAttributeNameChanged, DOMCharacterDataModified, DOMElementNameChanged, DOMNodeInserted, DOMNodeInsertedIntoDocument, DOMNodeRemoved, DOMNodeRemovedFromDocument, DOMSubtreeModified

I'm not sure whether we should flag calls to addEventListener or just the presence of those strings.

Comment 3

[Matt Basta [:basta]]

I can mark these as flagged identifiers. If these are pretty uncommon anyway (or shouldn't be used), then there likely won't be too much noise.

Comment 4

[Matt Basta [:basta]]

Check it out:

https://github.com/mattbasta/amo-validator/commit/4e2839d861bcbf8d5b7f8aadfb2dbcdce0cbcbf6

Will create a pull request as soon as the innerHTML stuff goes through.

Comment 5

[Kris Maglione [:kmag]]

Sorry, I missed comment 3. I don't think adding these to the banned identifier list because they're strings passed to addEventListener rather than strings. They also apparently work as on* attributes (but not on* properties, as far as I can tell).

Comment 6

[Matt Basta [:basta]]

> strings passed to addEventListener rather than strings.

Come again?

Comment 7

[Kris Maglione [:kmag]]

Heh...

elem.addEventListener("DOMSubtreeModified", onDOMSubtreeModified, false);

elem.setAttribute("onDOMSubtreeModified", "doStuff(event)")

<elem onDOMSubtreeModified="doStuff(event)"/>

Sorry for the typo. I meant "rather than identifiers".

Comment 8

[Matt Basta [:basta]]

Ah, then this is going to end up as a lovely regex test. Will do soon.

Comment 9

[Matt Basta [:basta]]

Posted:

https://github.com/mozilla/amo-validator/pull/14

Comment 10

[Matt Basta [:basta]]

Pushed to mozilla/amo-validator

Comment 11

[krupa raj[:krupa]]

Kris/ Jorgev, can you verify? Thanks!

Comment 12

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

It doesn't work on preview.

https://addons.allizom.org/en-US/developers/addon/the-official-diego-torres-add-/file/114505/validation

In overlay.js:

cl.addEventListener('DOMSubtreeModified', function (e) {

Comment 13

[Kris Maglione [:kmag]]

Attachment: Failing XPI

Agreed, this doesn't seem to work on allizom or from git. overlay.js and overlay.xul should trigger warnings in the attached XPI.

Comment 14

[Matt Basta [:basta]]

I'm seeing the errors being reported:

Warning: DOM Mutation Events Prohibited

DOM mutation events are flagged because of their deprecated status, as well as their extreme inefficiency. Consider using a different event.
        Tier:   3
        File:   content/overlay.js
        Line:   10
        Context:
        >       document.addEventListener('DOMNodeRemovedFromDocument', function () {}, false);
        >       document.addEventListener('DOMSubtreeModified', function () {}, false);
        >

Can you confirm that you have the latest version from git? Also, make sure you're using the --determined flag.

Comment 15

[Kris Maglione [:kmag]]

The JS errors are showing up for me with the latest pull now (they weren't at the time of my last comment), but still not the on* attributes.

Comment 16

[Matt Basta [:basta]]

Ah, didn't realize that those existed. They're added now, if someone could look over the code:

https://github.com/mozilla/amo-validator/pull/23

Comment 17

[Matt Basta [:basta]]

Merged:

https://github.com/mozilla/amo-validator/commit/4bce5bfb2421c5a895bffecc85b5ed07378082ab

Comment 18

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

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...