Last Comment Bug 773100 - bubblemark redraws the entire page on invalidation on mobile
: bubblemark redraws the entire page on invalidation on mobile
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla17
Assigned To: Matt Woodrow (:mattwoodrow) (PTO until 27 June)
:
Mentors:
http://www.bubblemark.com/dhtml.htm
: 775146 (view as bug list)
Depends on: 776836 778701
Blocks: 774756 767070 check2
  Show dependency treegraph
 
Reported: 2012-07-11 17:10 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-12-05 18:41 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
fixed


Attachments
Invalidate the frame bounds instead of deleting the frame property (1.16 KB, patch)
2012-07-16 19:37 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2012-07-11 17:10:49 PDT
:(
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-11 17:41:00 PDT
Does DLBI help here?  (Is it better on one of the nightlies that had DLBI landed?)
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-07-13 11:26:13 PDT
So here's a rough idea of what's going on.

PresShell::Paint is being called with a sane region of about 56x54. However, by the time we get down to BasicTiledThebesLayer::InvalidateRegion this has expanded to 480x690.

This happens because CreateOrRecycleThebesLayer() is calling layer->InvalidateRegion(Bounds())
because mInvalidateAllThebesContent is true.

mInvalidateAllThebesContent is set by ContainerSet::SetInvalidateAllThebesContent()

Which is caused by InternalInvalidateThebesLayersInSubtree(nsImageFrame*) deleting the ThebesLayerInvalidRegionProperty()
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-07-13 11:27:17 PDT
This was debugged on my single ball bubblemark: http://people.mozilla.org/~jmuizelaar/tests/bubble.html
Comment 4 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-07-16 19:37:31 PDT
Created attachment 642838 [details] [diff] [review]
Invalidate the frame bounds instead of deleting the frame property
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-07-16 21:22:12 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Created attachment 642838 [details] [diff] [review]
> Invalidate the frame bounds instead of deleting the frame property

This fixes the problem.
Comment 7 Benoit Girard (:BenWa) 2012-07-18 09:21:29 PDT
Swift and indiscriminate back-out by ehsan for build failures:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cdef000532ce

tree/layout/base/FrameLayerBuilder.cpp:2253: error: 'InvalidateThebesLayerContents' was not declared in this scope
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-07-18 11:18:40 PDT
*** Bug 775146 has been marked as a duplicate of this bug. ***
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-07-18 11:21:08 PDT
This would hopefully fix bug 767070.
Comment 10 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-07-18 11:57:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9f2d2b75d4
Comment 11 Alex Keybl [:akeybl] 2012-07-18 16:55:04 PDT
Let's see if this fixes bug 767070 before tracking.
Comment 12 Ed Morley [:emorley] 2012-07-19 07:34:08 PDT
https://hg.mozilla.org/mozilla-central/rev/8a9f2d2b75d4
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-23 14:14:55 PDT
bug 767070, comment 24 says this did fix the issue so we'll track this for branches. Please nominate for uplift.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-23 14:21:20 PDT
How risky is this?
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-23 17:16:55 PDT
Pretty low I think.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-23 17:18:29 PDT
Comment on attachment 642838 [details] [diff] [review]
Invalidate the frame bounds instead of deleting the frame property

Review of attachment 642838 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is suitable for Aurora but not Beta.
Comment 17 Alex Keybl [:akeybl] 2012-07-23 17:21:48 PDT
Comment on attachment 642838 [details] [diff] [review]
Invalidate the frame bounds instead of deleting the frame property

[Triage Comment]
Approving for Aurora 16. Do you think we can gain confidence in this patch in time to uplift to Beta, or are you suggesting we wontfix already at this point in the cycle? Thanks!
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-23 18:10:24 PDT
If mobile really has to have it for beta, I think that's OK. Otherwise I wouldn't push for it.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-23 20:18:56 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/55664fef2487
Comment 20 JP Rosevear [:jpr] 2012-07-24 06:08:59 PDT
Brad, Mark, what do you think of this for 15?
Comment 21 Benoit Girard (:BenWa) 2012-07-24 11:45:49 PDT
I'm still seeing full page invalidating with a recent changset:
https://hg.mozilla.org/mozilla-central/rev/cb0b3e605cf1
Comment 22 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-30 05:02:27 PDT
(In reply to JP Rosevear [:jpr] from comment #20)
> Brad, Mark, what do you think of this for 15?

It's a seemingly simple change, but I think we should not uplift for 15. Let it ride.
Comment 23 Alex Keybl [:akeybl] 2012-07-30 08:31:38 PDT
(In reply to Mark Finkle (:mfinkle) from comment #22)
> (In reply to JP Rosevear [:jpr] from comment #20)
> > Brad, Mark, what do you think of this for 15?
> 
> It's a seemingly simple change, but I think we should not uplift for 15. Let
> it ride.

Given mfinkle and roc's risk evaluation, wontfixing for 15.
Comment 24 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-12-05 18:41:19 PST
This looks fine on recent nightlies, probably fixed by DLBI.

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