773100 - bubblemark redraws the entire page on invalidation on mobile

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

:(

Comment 1

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

Does DLBI help here?  (Is it better on one of the nightlies that had DLBI landed?)

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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

This was debugged on my single ball bubblemark: http://people.mozilla.org/~jmuizelaar/tests/bubble.html

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Invalidate the frame bounds instead of deleting the frame property

Comment 5

[Jeff Muizelaar [:jrmuizel]]

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

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0be368bb172

Comment 7

[Benoit Girard (:BenWa)]

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 9

[Martijn Wargers (dead)]

This would hopefully fix bug 767070.

Comment 10

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9f2d2b75d4

Comment 11

[Alex Keybl [:akeybl]]

Let's see if this fixes bug 767070 before tracking.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a9f2d2b75d4

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

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-4, no longer working on Mozilla)]

How risky is this?

Comment 15

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

Pretty low I think.

Comment 16

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

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

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/55664fef2487

Comment 20

[JP Rosevear [:jpr]]

Brad, Mark, what do you think of this for 15?

Comment 21

[Benoit Girard (:BenWa)]

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?)]

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

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

This looks fine on recent nightlies, probably fixed by DLBI.