This page cause 100% CPU load (25% on quad core CPU) and is unusable with Firefox. With IE9 and Opera 12.10 it works fine. http://www.facebookstories.com/stories/1574/interactive-mapping-the-world-s-friendships#color=language-official&story=1&country=US I use the current nightly.

I can reproduce this on Fedora 17 as well.

http://people.mozilla.com/~bgirard/cleopatra/?report=e12b0e27c27276f7d9b77a279ef8266db5ba5ee9 Profiler trace. I get low performance with 5 or so FPS but no hangs, probably because I have a fast CPU.

~15% of time is spent under SetAttribute calls that cause SVG invalidations, and ~33% of time is spent doing SVG reflow.

The CPU usage is different between Aurora17.0a2 and Nightly18.0a1. Aurora17.0a2 : CPU usage is settle down about 6-12%(open the page; and approximately 20 seconds later) Nightly18.0a1 : CPU usage keep about 22-25% *25%=1core Regression window(m-c) CPU usage become lower than 12% http://hg.mozilla.org/mozilla-central/rev/b274e8e3479f Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120928160219 CPU higher than 22% http://hg.mozilla.org/mozilla-central/rev/b62b229a4d41 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120928161018 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b274e8e3479f&tochange=b62b229a4d41 Regression window(m-i) CPU usage become lower than 12% http://hg.mozilla.org/integration/mozilla-inbound/rev/9f476b4ac1e1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120928040137 CPU higher than 22% http://hg.mozilla.org/integration/mozilla-inbound/rev/6b58397ad735 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120928042236 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9f476b4ac1e1&tochange=6b58397ad735

Trying to preserve behaviour as much as possible and keep this simple. Lot more changes I'd like to make here :)

I went for opt-out for the content types since otherwise when adding a new content type you'd need to audit every display item subclass and figure out if they need the flag. And vice versa for adding a new display item type. Seems bug prone, and I can't think of a way to validate correctness. May end up being a bit verbose, but we can probably #define common combinations of flags at least.

Comment on attachment 673085 [details] [diff] [review] Add nsDisplayBackground color and make the bounds of nsDisplayBackground match the image Was passing and thought I'd add my 2c. >+ // a root, other wise keep going in order to let the theme stuff otherwise > case NS_STYLE_BG_CLIP_PADDING: >- haveRadii = mFrame->GetPaddingBoxBorderRadii(radii); >- clipRect = mFrame->GetPaddingRect() - mFrame->GetPosition() + ToReferenceFrame(); >+ haveRadii = aItem->GetUnderlyingFrame()->GetPaddingBoxBorderRadii(radii); >+ clipRect = aItem->GetUnderlyingFrame()->GetPaddingRect() - aItem->GetUnderlyingFrame()->GetPosition() + aItem->ToReferenceFrame(); > break; > case NS_STYLE_BG_CLIP_CONTENT: >- haveRadii = mFrame->GetContentBoxBorderRadii(radii); >- clipRect = mFrame->GetContentRect() - mFrame->GetPosition() + ToReferenceFrame(); >+ haveRadii = aItem->GetUnderlyingFrame()->GetContentBoxBorderRadii(radii); >+ clipRect = aItem->GetUnderlyingFrame()->GetContentRect() - aItem->GetUnderlyingFrame()->GetPosition() + aItem->ToReferenceFrame(); Probably ought to wrap long lines. > > nsRect borderBox = nsRect(ToReferenceFrame(), mFrame->GetSize()); This line can now move inside the if (bg->mBackgroundInlinePolicy... below can't it. >- if (mIsBottommostLayer && NS_GET_A(bg->mBackgroundColor) == 255 && >- !nsCSSRendering::IsCanvasFrame(mFrame)) { >- result = GetInsideClipRegion(presContext, bottomLayer.mClip, borderBox, aSnap); >- } > > // For policies other than EACH_BOX, don't try to optimize here, since > // this could easily lead to O(N^2) behavior inside InlineBackgroundData, > // which expects frames to be sent to it in content order, not reverse > // content order which we'll produce here. > // Of course, if there's only one frame in the flow, it doesn't matter. > if (bg->mBackgroundInlinePolicy == NS_STYLE_BG_INLINE_POLICY_EACH_BOX || > (!mFrame->GetPrevContinuation() && !mFrame->GetNextContinuation())) { > const nsStyleBackground::Layer& layer = bg->mLayers[mLayer]; > if (layer.mImage.IsOpaque()) { > nsRect r = nsCSSRendering::GetBackgroundLayerRect(presContext, mFrame, > borderBox, *bg, layer); >- result.Or(result, GetInsideClipRegion(presContext, layer.mClip, r, aSnap)); >+ result.Or(result, GetInsideClipRegion(this, presContext, layer.mClip, r, aSnap)); > } > } > > return result; > } >+nsDisplayBackgroundColor::IsUniform(nsDisplayListBuilder* aBuilder, nscolor* aColor) >+{ >+ *aColor = mColor; >+ nsStyleContext* bgSC; >+ nsPresContext* presContext = mFrame->PresContext(); >+ if (!nsCSSRendering::FindBackground(presContext, mFrame, &bgSC)) >+ return true; >+ >+ const nsStyleBackground* bg = bgSC->GetStyleBackground(); >+ if (!nsLayoutUtils::HasNonZeroCorner(mFrame->GetStyleBorder()->mBorderRadius) && >+ bg->BottomLayer().mClip == NS_STYLE_BG_CLIP_BORDER) { >+ return true; >+ } >+ return false; return !nsLayoutUtils::HasNonZeroCorner(mFrame->GetStyleBorder()->mBorderRadius) && bg->BottomLayer().mClip == NS_STYLE_BG_CLIP_BORDER;

Comment on attachment 673085 [details] [diff] [review] Add nsDisplayBackground color and make the bounds of nsDisplayBackground match the image Review of attachment 673085 [details] [diff] [review]: ----------------------------------------------------------------- Can't we remove some code from PaintBackgroundWithSC that draws background colors? ::: layout/base/nsCSSRendering.cpp @@ +2611,5 @@ > + const nsRect& aDirtyRect, > + const nsRect& aBorderArea, > + nsStyleContext* aBackgroundSC, > + const nsStyleBorder& aBorder, > + uint32_t aFlags) Fix indent ::: layout/base/nsCSSRendering.h @@ +337,5 @@ > uint32_t aFlags, > nsRect* aBGClipRect = nullptr, > int32_t aLayer = -1); > + > + Remove one blank line, and remove trailing whitespace @@ +343,5 @@ > + nsRenderingContext& aRenderingContext, > + nsIFrame* aForFrame, > + const nsRect& aDirtyRect, > + const nsRect& aBorderArea, > + uint32_t aFlags); Somewhere we need to document that theme backgrounds are drawn in the first layer of background images and nowhere else. @@ +372,5 @@ > + const nsRect& aDirtyRect, > + const nsRect& aBorderArea, > + nsStyleContext *aStyleContext, > + const nsStyleBorder& aBorder, > + uint32_t aFlags); Fix indent ::: layout/base/nsDisplayList.cpp @@ +1537,5 @@ > > + bool drawBackgroundColor = false; > + nscolor color; > + if (!nsCSSRendering::IsCanvasFrame(aFrame) && > + bg) { one line @@ +1544,5 @@ > + nsCSSRendering::DetermineBackgroundColor(presContext, bgSC, aFrame, > + drawBackgroundImage, drawBackgroundColor); > + } > + > + // Even if we don' actually have a background color to paint, we still need don't @@ +1549,5 @@ > + // to create the item because it's used for hit testing. > + aList->AppendNewToTop( > + new (aBuilder) nsDisplayBackgroundColor(aBuilder, aFrame, > + drawBackgroundColor ? color : NS_RGBA(0, 0, 0, 0))); > + lose trailing whitespace @@ +2162,5 @@ > + } > + if (nextItem && nextItem->GetUnderlyingFrame() == mFrame && > + nextItem->GetType() == TYPE_BORDER) { > + flags |= nsCSSRendering::PAINTBG_WILL_PAINT_BORDER; > + } Factor this out into a helper function that scans for TYPE_BORDER, so it can be shared with nsDisplayBackground above @@ +2174,5 @@ > +nsDisplayBackgroundColor::GetOpaqueRegion(nsDisplayListBuilder* aBuilder, > + bool* aSnap) > +{ > + if (mColor == NS_RGBA(0, 0, 0, 0)) { > + return nsRegion(); Just check NS_GET_A(mColor) < 255 and avoid the same check below?

Comment on attachment 673086 [details] [diff] [review] Don't invalidate nsDisplayBackgroundColor if we have a changed image Review of attachment 673086 [details] [diff] [review]: ----------------------------------------------------------------- You're right, I don't like this too much :-) How about this: Make ImageLoader::AssociateRequestToFrame take a display item key as a parameter, and have ImageLoader invalidate that display item key? I think this would mean making ImageLoader::RequestSet an array of (item type, imgRequest) pairs. This would actually make us target invalidations at the right image layer.

I don't think we know which display item key will be painting the image. The callers of AssociateRequestToFrame in nsFrame::DidSetStyleContext in particular, since there are other display item types that could be painting the background. Tables have a few, canvas frames, buttons, fieldset etc.

True, but I've tried again to like your patch and failed :-). Especially because I just noticed mInvalidationFlags being added to nsIFrame.

Can we walk the DisplayItemDatas for the frame whose image is being changed, and simply look at the stored keys, masking off all by the type (via TYPE_BITS), and calling GetIgnoreInvalidateFlagsForType for that type? But instead of that function I'd just have a general "display item type flags" array, and have a flag TYPE_RENDERS_NO_IMAGES that we can use here.

Fixes a test failure with the first patch. This was not fun to find :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a8bf41e0c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/c95958e3b85d

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5374fb480634 for oddly variable Android reftest-4 failures - transform-3d/backface-visibility-2.html was pretty consistent, only passing a couple of times on Armv6, while other transform-3d failures came and went.

Comment on attachment 675436 [details] [diff] [review] Don't invalidate nsDisplayBackgroundColor if we have a changed image v2 Review of attachment 675436 [details] [diff] [review]: ----------------------------------------------------------------- Basically looks fine, but avoid the abstract base class as we discussed ::: layout/base/FrameLayerBuilder.h @@ +300,5 @@ > Layer* GetOldLayerFor(nsDisplayItem* aItem, > nsDisplayItemGeometry** aOldGeometry = nullptr, > Clip** aOldClip = nullptr, > + nsTArray<nsIFrame*>* aChangedFrames = nullptr, > + bool *aIsInvalid = nullptr); Document this parameter ::: layout/base/nsDisplayItemTypes.h @@ +56,5 @@ > +#define DECLARE_DISPLAY_ITEM_TYPE_FLAGS(name,flags) case TYPE_##name: return flags; > +#include "nsDisplayItemTypesList.h" > +#undef DECLARE_DISPLAY_ITEM_TYPE > +#undef DECLARE_DISPLAY_ITEM_TYPE_FLAGS > + default: return 0; How about instead using a static const array of TYPE_MAX uint8_ts?

Now with more qref!

Comment on attachment 675460 [details] [diff] [review] Don't invalidate nsDisplayBackgroundColor if we have a changed image v4 Review of attachment 675460 [details] [diff] [review]: ----------------------------------------------------------------- Very nice.

https://hg.mozilla.org/integration/mozilla-inbound/rev/72a4289d68a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/d83559cf4387 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d793eb6a281 Try: https://tbpl.mozilla.org/?tree=Try&rev=eda9b96d1a58

https://hg.mozilla.org/mozilla-central/rev/72a4289d68a1 https://hg.mozilla.org/mozilla-central/rev/d83559cf4387 https://hg.mozilla.org/mozilla-central/rev/8d793eb6a281

We still need to split nsDisplayCanvasBackground (layout/generic/nsCanvasFrame.{h/cpp}) into two items too. Attached is a testcase that shows this bug happening. This should be fairly similar to the first patch here, where we change the bounds of nsDisplayCanvasBackground to be the size of the image (instead of the size of the viewport), and create a new item (nsDisplayCanvasBackgroundColor?) that is viewport sized and paints on the color portion of the background. We should make sure that bug 810470 lands before doing too much on this.

This is the case that was fixed by the current patches in this bug.

Comment on attachment 674931 [details] [diff] [review] Make mLineContinuationPoint correct when we call Init() on a frame that isn't the first on the line. [Approval Request Comment] Bug caused by (feature/regressing bug #): DLBI User impact if declined: The real need for these patches is that the patches in bug 810470 depend on them. Testing completed (on m-c, etc.): 10 days on m-c with no reported regressions Risk to taking this patch (and alternatives if risky): See bug 810470 String or UUID changes made by this patch: None This request is for the first two patches in this bug (this patch and the previous patch).

Heads up: possible crash regression - bug 812776.

Comment on attachment 674931 [details] [diff] [review] Make mLineContinuationPoint correct when we call Init() on a frame that isn't the first on the line. [Triage Comment] Approving for FF18, as this will help resolve a DLBI regression. If this is landed on mozilla-aurora before ~11AM PT tomorrow, this will make the merge from Aurora 18 to Beta 18. If landed 11AM-5PM PT tomorrow on mozilla-beta, it will make the first FF18 Beta. If landed after that, it will end up in the second FF18 beta.

https://hg.mozilla.org/releases/mozilla-aurora/rev/0121068bd2b8 https://hg.mozilla.org/releases/mozilla-aurora/rev/5c4e46b4958d

We could take the third patch to actually fix this bug on Aurora, if we want to.

The regression is fixed on central.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > We could take the third patch to actually fix this bug on Aurora, if we want > to. If the risk profile is low, it sounds like a good idea to uplift.

This is already fixed on current Aurora. The question is whether we wan to fix it on Beta.

The first two patches here appear to be on beta already. I don't think it's worth trying to uplift the final patch, leaving that on aurora should be fine.

Thanks Matt - given that, untracking for release.

I can reproduce this on FF 19b1 on Windows 7 x64 as well. I am using: Intel(R) Core(TM) i5-3470 CPU @ 3.2GHz 3.6Ghz. Before loading page from Comment 0 my CPU usage is about 6-8 %. After loading that page in FF 19b1 my CPU usage increases at about 35-38%. Looks like this issue in not fixed yet.

(In reply to MarioMi from comment #35) > After loading [facebookstories] in FF 19b1 my CPU usage increases at about 35-38%. > > Looks like this issue in not fixed yet. I'd disagree -- that's not an entirely-unreasonable amount of CPU load, given that we're rendering a pretty complex page. Note that this bug was filed with this description: > This page cause 100% CPU load [...] and is unusable with > Firefox. With IE9 and Opera 12.10 it works fine. So, we've gone from 100% CPU usage & unusable to 35% CPU usage & presumably usable (by your measurement). :) That's a pretty big improvement! However, if you have reason to believe that that's still too high (and/or if you have cross-browser comparisons that show we're doing still doing significantly worse than competitors), please file a separate bug. We try to keep bugs bite-sized as much as possible. (and this one's been closed for a month.)

(In reply to Daniel Holbert [:dholbert] from comment #36) > So, we've gone from 100% CPU usage & unusable to 35% CPU usage & presumably > usable (by your measurement). :) That's a pretty big improvement! However, > if you have reason to believe that that's still too high (and/or if you have > cross-browser comparisons that show we're doing still doing significantly > worse than competitors), please file a separate bug. We try to keep bugs > bite-sized as much as possible. (and this one's been closed for a month.) In this case please someone else verify CPU usage and if it's almost like mine please set as verified fixed. I hadn't looked at page source and how complex it is so after some extra investigations done, I can say it works as expected.

Opening http://www.facebookstories.com/stories/1574/interactive-mapping-the-world-s-friendships#color=language-official&story=1&country=US causes constant 50% CPU usage, so one core is working on 100%. IMO looks not fixed completely. Using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130116 Firefox/21.0 on E6550 2,33GHz@3,5GHz with GTX460v2

CPU usage is now higher than before the patch.

(In reply to MarioMi from comment #37) Looks much better on FF 19b2 on same PC Configuration. It uses about 20% CPU. Based on this and on Comment 32 I consider this Verified Fixed.

Moving back Status as Resolved as long as Tracking for ff18 is still affected.

How can it be fixed when it use now 100% CPU power on 1 core?

(In reply to Virtual_ManPL [:Virtual] from comment #42) > How can it be fixed when it use now 100% CPU power on 1 core? Did you try to reproduce this issue with new profile or in safe mode? http://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode?redirectlocale=en-US&redirectslug=Safe+Mode Here are some information with some options for you to review. http://support.mozilla.org/en-US/kb/firefox-uses-too-many-cpu-resources-how-fix What happens in those cases?

This is my CPU Usage without facebooks tories opened. Please see attached file.

This is my CPU Usage with facebook stories opened. Please see attached file.

I also can confirm same results or almost (+ -) 5% CPU Usage On Mac OS 10.8 and Ubuntu x86 for FF 19b2.

I know how to test Firefox, but thank you for detailed info :) Firefox still uses 100% of 1 core even in safe mode or with new profile without any addons and plugins. For more info - Firefox menus and all interface is very, very laggy. I use latest hourly Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130123 Firefox/21.0 on E6550 2,33GHz@3,5GHz (2 core CPU) with GTX460v2 Attaching as screenshot how it looks on my end when I open and close http://www.facebookstories.com/stories/1574/interactive-mapping-the-world-s-friendships#color=language-official&story=1&country=US

OK, I can confirm that I get ~100% CPU load when I visit that URL. (Not at the front page anymore, because their front page has changed so that it now lacks the expensive bouncy-circles-over-a-map animation.) sysprof says 70% of my time is spent in nsSVGOuterSVGFrame::InvalidateSVG()'s call to nsRegion::Or(). After a little while (5 minutes? presumably after everything 'settles'), my firefox process goes down to 10-20% CPU usage. Matt: I'm assuming we should track this issue in a new bug (even though it matches this bug's description) -- correct?