761884 - LayoutView is causing an infinite loop by making changes within a MozAfterPaint handler

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Only update the size if it has changed

LayoutView::update is setting textContext every time, which then triggers painting again (and another call to MozAfterPaint).

This is causing test failures and excessive CPU usage with DLBI (bug 539356).

Attached patch fixes the issue, it might be better to cache the width/height values on the object instead of doing string comparison though.

Comment 1

[Paul Rouget [:paul]]

Thank you for spotting this problem.
The loop doesn't happen all the time here. It's hard to reproduce (but I managed to reproduce once).

There's something I am not sure to understand though.

The Layout View (where the text is modified) is in the sidebar (chrome).
We want to track the potential changes in the content.
We don't want to listen to any paint events from the chrome.

I do gBrowser.addEventListener(MozAfterPaint). And after some tests, it seems that the MozAfterPaint event is not fired when paint operations happen in the Layout View (or anywhere in the sidebar).

So why do the loop happen?

However, when we get the MozAfterPaint event, we call getBoundingRect and getComputedStyle (content). And if I am not mistaken, these can cause a reflow, which might cause a paint operation.

What do you think?

Also, do you know why doing gBrowser.contentWindow.addEventListener(MozAfterPaint) doesn't seem to work? Because that should be the right way to do it I guess.

Comment 2

[Matt Woodrow (:mattwoodrow)]

(In reply to Paul Rouget [:paul] from comment #1)
> Thank you for spotting this problem.
> The loop doesn't happen all the time here. It's hard to reproduce (but I
> managed to reproduce once).
> 
> There's something I am not sure to understand though.
> 
> The Layout View (where the text is modified) is in the sidebar (chrome).
> We want to track the potential changes in the content.
> We don't want to listen to any paint events from the chrome.
> 
> I do gBrowser.addEventListener(MozAfterPaint). And after some tests, it
> seems that the MozAfterPaint event is not fired when paint operations happen
> in the Layout View (or anywhere in the sidebar).
> 
> So why do the loop happen?

This might be because of DLBI. My patch queue makes some changes to the way that MozAfterPaint works, and times that it's fired.

> 
> However, when we get the MozAfterPaint event, we call getBoundingRect and
> getComputedStyle (content). And if I am not mistaken, these can cause a
> reflow, which might cause a paint operation.

It's setting the .textContext variable that is causing repainting.

> 
> What do you think?
> 
> Also, do you know why doing
> gBrowser.contentWindow.addEventListener(MozAfterPaint) doesn't seem to work?
> Because that should be the right way to do it I guess.

That should work fine, not sure why it doesn't. Might be worth trying again after DLBI lands.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Since I'm trying to get DLBI landed as soon as possible (for maximum bake time before the next merge), would you mind taking this (or a variant of) in the short term?

We can look at a different solution afterwards if you'd prefer.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 630417 [details] [diff] [review]
Only update the size if it has changed

>+    if (this.doc.querySelector("#element-size").textContent !=  width + "x" + height) {
>+      this.doc.querySelector("#element-size").textContent =  width + "x" + height;

Use a local variable for this.doc.querySelector("#element-size").

Comment 5

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e31a5e6545d3

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 630417 [details] [diff] [review]
> Only update the size if it has changed
> 
> >+    if (this.doc.querySelector("#element-size").textContent !=  width + "x" + height) {
> >+      this.doc.querySelector("#element-size").textContent =  width + "x" + height;
> 
> Use a local variable for this.doc.querySelector("#element-size").

Sorry, completely forgot about this. If the landing sticks, then I'll fix this in a followup.

Comment 6

[Ed Morley [:emorley]]

Ok, so this is pretty sad faces, but I've had to back this out for causing mochitest-4 permaorange (as well as the talos regressions in comment 237):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=61fd66629c4f

eg https://tbpl.mozilla.org/php/getParsedLog.php?id=12544443&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08886a8cf22

It's for times like these that a relanding script would be pretty useful, for automating the qimport of a lange range of changesets (or at least it would make me feel less bad about having to back things like this out). 

Sorry! :-(

Comment 7

[Paul Rouget [:paul]]

Attachment: update

Comment 8

[Paul Rouget [:paul]]

Comment on attachment 634909 [details] [diff] [review]
update

Using variables as suggested in comment 4.
And I think we can re-land that.

Comment 9

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/1b472fa5c8a4

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1b472fa5c8a4