950526 - need to be smarter about when textruns are dumped due to restyling

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[John Daggett (:jtd)]

Steps: load the URL
Result: browser becomes unresponsive and sluggish

The testcase is one that simply changes the body element color at a fixed interval for a page containing a large chunk of text.  The sluggishness is in part due to the fact that large text runs make the browser unresponsive (bug 934770) but the root cause is that even for simply style changes such as flipping the color we completely dump and rebuild all textruns on the page.

After initial page load, all the unloading and rebuilding of textruns happens within paint code:

#0  nsTextFrame::ClearTextRun (this=0x1267faf10, aStartContinuation=0x0, aWhichTextRun=nsTextFrame::eInflated) at nsTextFrame.cpp:4339
#1  0x000000010430cc58 in nsTextFrame::ClearTextRuns (this=0x1267faf10) at nsTextFrame.h:507
#2  0x00000001042f2da0 in nsTextFrame::DidSetStyleContext (this=0x1267faf10, aOldStyleContext=0x12558b9f8) at nsTextFrame.cpp:4443
#3  0x00000001040cc2c2 in nsIFrame::SetStyleContext (this=0x1267faf10, aContext=0x126bb6080) at nsIFrame.h:801
#4  0x000000010414d61b in mozilla::ElementRestyler::RestyleSelf (this=0x7fff5fbfa6e8, aSelf=0x1267faf10, aRestyleHint=0) at RestyleManager.cpp:2443
#5  0x000000010414c834 in mozilla::ElementRestyler::Restyle (this=0x7fff5fbfa6e8, aRestyleHint=0) at RestyleManager.cpp:2251
#6  0x000000010414ea3c in mozilla::ElementRestyler::RestyleContentChildren (this=0x7fff5fbfaa90, aParent=0x1267fac50, aChildRestyleHint=0) at RestyleManager.cpp:2787
#7  0x000000010414dc42 in mozilla::ElementRestyler::RestyleChildren (this=0x7fff5fbfaa90, aChildRestyleHint=0) at RestyleManager.cpp:2522
#8  0x000000010414c86f in mozilla::ElementRestyler::Restyle (this=0x7fff5fbfaa90, aRestyleHint=eRestyle_Self) at RestyleManager.cpp:2255
#9  0x000000010414912a in mozilla::RestyleManager::ComputeStyleChangeFor (this=0x1243f8800, aFrame=0x1267fac50, aChangeList=0x7fff5fbfad90, aMinChange=0, aRestyleTracker=@0x1243f8848, aRestyleDescendants=false) at RestyleManager.cpp:2875
#10 0x0000000104148d2f in mozilla::RestyleManager::RestyleElement (this=0x1243f8800, aElement=0x12437e660, aPrimaryFrame=0x1267fac50, aMinHint=0, aRestyleTracker=@0x1243f8848, aRestyleDescendants=false) at RestyleManager.cpp:818
#11 0x0000000104181152 in mozilla::RestyleTracker::ProcessOneRestyle (this=0x1243f8848, aElement=0x12437e660, aRestyleHint=eRestyle_Self, aChangeHint=0) at RestyleTracker.cpp:121
#12 0x000000010414f2c1 in mozilla::RestyleTracker::DoProcessRestyles (this=0x1243f8848) at RestyleTracker.cpp:205
#13 0x0000000104180579 in mozilla::RestyleTracker::ProcessRestyles (this=0x1243f8848) at RestyleTracker.h:230
#14 0x000000010414ae78 in mozilla::RestyleManager::ProcessPendingRestyles (this=0x1243f8800) at RestyleManager.cpp:1391
#15 0x0000000104111dcd in PresShell::FlushPendingNotifications (this=0x11bdc6000, aFlush={mFlushType = Flush_InterruptibleLayout, mFlushAnimations = false}) at /builds/mozcentral/layout/base/nsPresShell.cpp:3972
#16 0x000000010411fa8e in PresShell::WillPaint (this=0x11bdc6000) at /builds/mozcentral/layout/base/nsPresShell.cpp:7596

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, we need a new style change hint for things that require textrun reconstruction.

Comment 2

[John Daggett (:jtd)]

Attachment: patch, avoid dumping textruns

Don't call ClearTextRuns within nsTextFrame::DidSetStyleContext.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8393354 [details] [diff] [review]
patch, avoid dumping textruns

>+/* static */ void
>+nsLayoutUtils::MarkDescendantsDirty(nsIFrame *aSubtreeRoot)
>+{
>+  nsAutoTArray<nsIFrame*, 4> subtrees;
>+  subtrees.AppendElement(aSubtreeRoot);
>+
>+  // dirty the frame itself
>+  aSubtreeRoot->MarkIntrinsicWidthsDirty();

You succeed in doing this for aSubtreeRoot, but not for the other subtrees created by out-of-flows.

I think the easy fix for this is to remove both MarkIntrinsicWidthsDirty calls you have, and replace them with a single call on the |f| variable right after you remove it from the stack.

>diff --git a/layout/svg/SVGTextFrame.cpp b/layout/svg/SVGTextFrame.cpp

>   PresContext()->PresShell()->FrameNeedsReflow(
>-    f, nsIPresShell::eResize, NS_FRAME_HAS_DIRTY_CHILDREN);
>+    f, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);
> }

This change seems less than ideal.  I'm curious why it was needed -- could we at least get a followup bug on fixing it, if not find another way to fix the problem this is fixing?

r=dbaron

Comment 4

[John Daggett (:jtd)]

Attachment: patch, simple reftest of font styles in elements affect by first-line

Reftest to assure the patch code hit all the right points in the subframe tree.

Comment 5

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #3)
> >   PresContext()->PresShell()->FrameNeedsReflow(
> >-    f, nsIPresShell::eResize, NS_FRAME_HAS_DIRTY_CHILDREN);
> >+    f, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);
> > }
> 
> This change seems less than ideal.  I'm curious why it was needed --
> could we at least get a followup bug on fixing it, if not find
> another way to fix the problem this is fixing?

This is due to the assertion in FrameNeedsReflow which requires
eStyleChange to be used only with NS_FRAME_IS_DIRTY.

I'll file a separate bug.

Comment 6

[John Daggett (:jtd)]

Filed as 985834.

Comment 7

[John Daggett (:jtd)]

Pushed to trunk, with changes based on review comments
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d12ab8bf5fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5f4ed799fa3

Comment 8

[Carsten Book [:Tomcat]]

sorry, had to backout this changes for android reftests failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36430576&tree=Mozilla-Inbound

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think you may also need to do this after the ReparentFrames call in nsCSSFrameConstructor::WrapFramesInFirstLineFrame.  I still haven't found one for first-letter, though.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Other places that may need fixing, which call ResolveStyleForNonElement directly (since there's no subtree):

nsCSSFrameConstructor::CreateFloatingLetterFrame, although it's *right* after frame creation so I'd hope we don't have text runs yet

nsCSSFrameConstructor::CreateLetterFrame, ditto

nsCSSFrameConstructor::RemoveFloatingFirstLetterFrames

nsCSSFrameConstructor::RemoveFirstLetterFrames

nsFirstLetterFrame::CreateContinuationForFloatingParent

nsFirstLetterFrame::DrainOverflowFrames

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Also, the failure appears to be Android only because Android doesn't have the second scrollbar reflow.)

Comment 12

[John Daggett (:jtd)]

Attachment: patch, more reftests to test first-line/first-letter scenarios

Comment 13

[John Daggett (:jtd)]

Attachment: patch, dirty subtrees in a couple more places

These fix the reftest failures on Android, in particular the call to xxx within nsFirstLetterFrame::DrainOverflowFrames.  

I couldn't figure out what I needed to hit the callpoint within nsFirstLetterFrame::CreateContinuationForFloatingParent.  If you can suggest a way of testing this, I'll add it to the set of reftests.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8401700 [details] [diff] [review]
patch, more reftests to test first-line/first-letter scenarios

Did you mean for the floater test to test floating first-letter rather than first-letter on a float?

Maybe clean up the newlines in the tests (there are a few "No newline at end of file") and definitely the double-newline in the reftest.list.

r=dbaron with that

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to John Daggett (:jtd) from comment #13)
> I couldn't figure out what I needed to hit the callpoint within
> nsFirstLetterFrame::CreateContinuationForFloatingParent.  If you can suggest
> a way of testing this, I'll add it to the set of reftests.

You need ::first-letter { float: left }, not float:left on the block that has the ::first-letter.  See previous comment.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8401703 [details] [diff] [review]
patch, dirty subtrees in a couple more places

>+      nsLayoutUtils::MarkDescendantsDirty(this);

Seems like you should be able to pass |kid| rather than |this|, since it's |kid| whose style context is changing.

r=dbaron with that (assuming it still works)

Comment 17

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #15)

> You need ::first-letter { float: left }, not float:left on the block that
> has the ::first-letter.  See previous comment.

*sigh* I put in a test for this but it's actually tricky to test due to a separate existing bug which I logged (bug 992846).  In a nutshell, floating the first-letter affects the baseline for the remaining text on the line on platforms sensitive to integer pixel rounding errors (Win GDI/Linux).  This is also probably related to the odd way we lay out floating first-letter pseudo elements. See bug 415506 and bug 729352.

So I'm going to pull out a reftest just for the floating first-letter case and mark it failing/random on Windows/Linux.  The fix for bug 992846 will clear that.

Comment 18

[John Daggett (:jtd)]

Pushed to inbound, with changes based on review comments
https://hg.mozilla.org/integration/mozilla-inbound/rev/353dde65b242
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1a8a3f1a69
https://hg.mozilla.org/integration/mozilla-inbound/rev/88f30798bfd0

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/353dde65b242
https://hg.mozilla.org/mozilla-central/rev/9e1a8a3f1a69
https://hg.mozilla.org/mozilla-central/rev/88f30798bfd0