Last Comment Bug 761884 - LayoutView is causing an infinite loop by making changes within a MozAfterPaint handler
: LayoutView is causing an infinite loop by making changes within a MozAfterPai...
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 16
Assigned To: Matt Woodrow (:mattwoodrow)
:
:
Mentors:
Depends on:
Blocks: dlbi
  Show dependency treegraph
 
Reported: 2012-06-05 19:27 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2012-06-22 08:37 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Only update the size if it has changed (1.25 KB, patch)
2012-06-05 19:27 PDT, Matt Woodrow (:mattwoodrow)
paul: review+
Details | Diff | Splinter Review
update (1.32 KB, patch)
2012-06-20 07:23 PDT, Paul Rouget [:paul]
dcamp: review+
Details | Diff | Splinter Review

Description Matt Woodrow (:mattwoodrow) 2012-06-05 19:27:02 PDT
Created attachment 630417 [details] [diff] [review]
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] 2012-06-06 03:14:26 PDT
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) 2012-06-06 15:34:30 PDT
(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) 2012-06-07 17:48:34 PDT
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] 2012-06-08 05:21:30 PDT
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) 2012-06-10 21:52:23 PDT
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] 2012-06-11 02:21:04 PDT
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] 2012-06-20 07:23:12 PDT
Created attachment 634909 [details] [diff] [review]
update
Comment 8 Paul Rouget [:paul] 2012-06-20 07:25:05 PDT
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] 2012-06-21 03:34:01 PDT
https://hg.mozilla.org/integration/fx-team/rev/1b472fa5c8a4
Comment 10 Tim Taubert [:ttaubert] 2012-06-22 08:37:17 PDT
https://hg.mozilla.org/mozilla-central/rev/1b472fa5c8a4

Note You need to log in before you can comment on or make changes to this bug.