1449584 - Turn Off the Lights extension triggers infinite mutation observer loops

Status: RESOLVED WORKSFORME

(WebExtensions :: Developer Outreach, defect, P3)

Description

[Stefan vd]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

1. Install the Turn Off the Lights Firefox extension version 4.0.2.0
https://addons.mozilla.org/en-US/firefox/addon/turn-off-the-lights/
2. Enable the "AutoStop" feature in the Options page
3. Go to a YouTube video
4. The Firefox web browser show this yellow message:
Warning Unresponsive script

5. There is a high use of performance with using this code.
File in the Turn Off the Lights extension -> "autostop.js" source:

	observer.observe(videolist, {
		subtree: true,       // observe the subtree rooted at ...videolist...
		childList: true,     // include childNode insertion/removals
		characterData: false, // include textContent changes
		attributes: true     // include changes to attributes within the subtree
	});

However, there is no issue in other web browsers such as Google Chrome, Chromium (Yandex), Opera.


Actual results:

Show high-performance use and this yellow warning message.


Expected results:

It blocks the video player and shows you a red banner on top that video content.

Comment 1

[Stefan vd]

The 'Autostop' feature that is in the Turn Off the Lights Firefox extension stop all the HTML5 videos that are playing. However, the performance is well in the other modern web browser such as Google Chrome. However, when you open a video website (such as YouTube) Firefox show suddenly this "yellow banner" and it said that it possible slow down the page.

Google Chrome = Opera = Chromium = Microsoft Edge -> no yellow banner.

Comment 2

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

tentatively to CSS handling, since most of the time is spent in stylo. But perhaps that is because we just call something too often?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Do you happen to have a profile link around please? Otherwise I can take one, but I suspect you do per comment 2 :)

Comment 4

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

I don't have anymore, but can create a new one tomorrow.

Comment 5

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

https://perfht.ml/2v4IhQP

Comment 6

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

Hmm, is this style attribute change loop.

Comment 7

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

yeah, something weird is happening. The addon itself is causing more and more mutation records.

Comment 8

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

Ok, so we get tons of style attribute changes where
old value [background: rgba(165, 8, 0, 0.88) none repeat scroll 0% 0% !important; height: 360px; display: none; top: 0px; left: 0px; width: 640px;]
new value [background: rgba(165, 8, 0, 0.88) none repeat scroll 0% 0% !important; display: none; top: 0px; left: 0px; width: 640px; height: 360px;]

My guess is that it isn't too relevant the values serialized tiny bit differently, since there should be MutationRecord even if attribute value didn't change.
But do other browsers not try to change attribute...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Looks like what's happening is that we're restyling because we consider it changed even though it really didn't. This could be a regression from bug 1415330, maybe?

Does this repro with https://hg.mozilla.org/mozilla-central/rev/62908a56c59f34e4dd5175717228cd514ac65c60 backed out?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Well, I guess it was reported before that landed, so nvm.

Comment 11

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

https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty says the attribute should be always updated.

data:text/html,<script>document.documentElement.style.top = "0px";  var m = new MutationObserver(function(r) { for (var i = 0; i < r.length; ++i) { console.log(r[i]); }  } ); m.observe(document.documentElement,  { attributes: true}); document.documentElement.style.top = "0px"; </script>

Looks like Chrome is buggy here.

Comment 12

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

Or the spec needs to be changed and then Gecko too.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Xidorn has actually touched the style mutation observer code very recently.

Comment 14

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

FWIW, I'd prefer if other implementations would be fixed, since right now MutationObserver works nicely consistent, and this would add a weird special case.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

I'm tempted to file bugs in other browsers and resolve this as INVALID as well, yeah...

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

So, this is basically the worst case in my mind when I was writing the spec as well as the implementation in bug 1415330.

This is the case that, the script listens to style attribute change, and it set multiple attributes to potentially identical value from the observer callback:
> item.style.top = visposition.y+"px";
> item.style.left = visposition.x+"px";
> item.style.width = w;
> item.style.height = h;

This will inevitably trigger mutations after the change in bug 1415330, and I think all browsers should agree on that if they are going to follow us on that change (which CSSWG resolved to a while ago, and I believe that's the right decision given various issues around ordering).

I call it the worst case because even optimizing out setting the last declaration repeatedly with identical is not going to help on this case.

Before that change, though, we could avoid this issue, but we didn't because of bug 1197705, that it is hard to trigger mutation observer after we decide whether to commit change (see bug 1197705 comment 5). There are still cases affected by that issue we should fix, but that's now unrelated to this bug anyway.

I think we should probably change this to a Tech Evangelism bug and ask the addon author to fix it. The most straightforward way as far as I can see, is to only listen to style attribute change on <video> target (move the `if(mutation.attributeName == "style")` block into the `if(mutation.target.tagName == "VIDEO")` block), since that code reads information of <video> elements and set styles on other (overlay?) elements.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

It seems their GitHub repo is quite out-of-date, so we cannot file PR for them... The only thing we can do is to reach out to the author.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #16)
> I think we should probably change this to a Tech Evangelism bug and ask the
> addon author to fix it. The most straightforward way as far as I can see, is
> to only listen to style attribute change on <video> target (move the
> `if(mutation.attributeName == "style")` block into the
> `if(mutation.target.tagName == "VIDEO")` block), since that code reads
> information of <video> elements and set styles on other (overlay?) elements.

It might be intended that they listen on all elements because changes on ancestors could change the position of <video> as well. In that case, I guess the way to fix would be to explicitly skip calling if the mutation is triggered on elements they mutate (via checking the className, which should include "stefanvdautostop").

Comment 19

[Daniel Holbert [:dholbert]]

> I think we should probably change this to a Tech Evangelism bug and ask the addon author to fix it. 

Let's say [qf-] then. (Though, if the expensive thing is something we can/should make less expensive somehow, please spin off a helper bug as-needed and that one might wanna be qf.)

Comment 20

[David Durst [:ddurst]]

Probably worth reaching out to the developer based on #c16-18.

Comment 21

[Andreas Wagner [:TheOne] [use NI]]

Before I contact the developer, could someone check whether it's still reproducible in the latest version of the add-on? Thanks.

Comment 22

[Stefan vd]

With the current Turn Off the Lights Firefox extension v4.0.29.0, I see not this yellow bar anymore on my Firefox v65.0.1. And that is good for my Firefox users.
However, it sometimes doesn't stop this HTML5 video(s) anymore.