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.

Don't call ClearTextRuns within nsTextFrame::DidSetStyleContext.

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

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

(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.

Filed as 985834.

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

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.)

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 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

(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)

(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.

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

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