Closed Bug 1449584 Opened 2 years ago Closed 1 year ago

Turn Off the Lights extension triggers infinite mutation observer loops

Categories

(WebExtensions :: Developer Outreach, defect, P3)

59 Branch
defect

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: stefan.vd01, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [qf-])

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.
Component: Untriaged → DOM
Keywords: perf
Product: Firefox → Core
Whiteboard: [qf]
Flags: needinfo?(bugs)
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.
tentatively to CSS handling, since most of the time is spent in stylo. But perhaps that is because we just call something too often?
Component: DOM → CSS Parsing and Computation
Flags: needinfo?(bugs)
Do you happen to have a profile link around please? Otherwise I can take one, but I suspect you do per comment 2 :)
Flags: needinfo?(bugs)
I don't have anymore, but can create a new one tomorrow.
Flags: needinfo?(bugs)
Hmm, is this style attribute change loop.
yeah, something weird is happening. The addon itself is causing more and more mutation records.
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...
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?
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(bugs)
Well, I guess it was reported before that landed, so nvm.
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(bugs)
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.
Or the spec needs to be changed and then Gecko too.
Xidorn has actually touched the style mutation observer code very recently.
Flags: needinfo?(xidorn+moz)
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.
I'm tempted to file bugs in other browsers and resolve this as INVALID as well, yeah...
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.
Flags: needinfo?(xidorn+moz)
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.
Status: UNCONFIRMED → NEW
Component: CSS Parsing and Computation → Add-ons
Ever confirmed: true
Product: Core → Tech Evangelism
Version: 59 Branch → Firefox 59
(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").
> 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.)
Whiteboard: [qf] → [qf-]
See Also: → 1460295
Component: Add-ons → General
Product: Tech Evangelism → WebExtensions

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

Flags: needinfo?(awagner)
Priority: -- → P3
Summary: observer heavy performance → Turn Off the Lights extension triggers infinite mutation observer loops

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

Flags: needinfo?(awagner)
Flags: needinfo?(stefan.vd01)

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.

Flags: needinfo?(stefan.vd01)
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → WORKSFORME
Component: General → Developer Outreach
Version: Firefox 59 → 59 Branch
You need to log in before you can comment on or make changes to this bug.