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

RESOLVED FIXED in mozilla31

Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

(Depends on 2 bugs)

unspecified
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
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
Yes, we need a new style change hint for things that require textrun reconstruction.
(Assignee)

Comment 2

5 years ago
Don't call ClearTextRuns within nsTextFrame::DidSetStyleContext.
Attachment #8393354 - Flags: review?(dbaron)
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
Attachment #8393354 - Flags: review?(dbaron) → review+
(Assignee)

Comment 4

5 years ago
Reftest to assure the patch code hit all the right points in the subframe tree.
Attachment #8393939 - Flags: review?(cam)
Attachment #8393939 - Flags: review?(cam) → review+
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Filed as 985834.
Depends on: 985834
sorry, had to backout this changes for android reftests failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36430576&tree=Mozilla-Inbound
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.
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
(Also, the failure appears to be Android only because Android doesn't have the second scrollbar reflow.)
(Assignee)

Comment 13

5 years ago
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.
Attachment #8401703 - Flags: review?(dbaron)
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
Attachment #8401700 - Flags: review?(dbaron) → review+
(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 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)
Attachment #8401703 - Flags: review?(dbaron) → review+
(Assignee)

Comment 17

5 years ago
(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.
Depends on: 1076918
No longer depends on: 1076918
You need to log in before you can comment on or make changes to this bug.