Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com Reporter
	Description • 7 years ago

[Affected versions]:
 Fx 53.0b1
 Fx 54.0a2
 Fx 55.0a1

[Affected platforms]:
 Windows 10 x64
 Ubuntu 14.04 LTS
 Mac OS X 10.11.6

[Steps to reproduce]:
 1. Launch Firefox.
 2. Go to https://threejs.org/examples/#css3d_panorama .

[Expected result]:
 The panoramic view goes smoothly.

[Actual result]:
 The panoramic view freezes often.

[Regression range]:
 I will write the regression range as soon as possible.

	Sotaro Ikeda [:sotaro]
	Comment 1 • 7 years ago

:CristiComo, could you get the regression range? Thanks!

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com Reporter
	Comment 2 • 7 years ago

Sorry for the long pause!

Last good: 20160605030215

First bad: 20160606131341


Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f75d7afd686e2af034d60e321c032b313e3ba440&tochange=58137ecd12edb7ae1a57c80da811676bc66fd03c


If you have trouble reproducing this issue make sure the browser is maximized or in full screen.

Cheers!

	Ryan VanderMeulen [:RyanVM]
	Comment 3 • 7 years ago

Bug 1274962 looks highly likely in that range.

	Ryan VanderMeulen [:RyanVM]
	Comment 4 • 7 years ago

This definitely reproduces for me on Fx52 as well (as would be expected given the regression range).

	Sotaro Ikeda [:sotaro]
	Comment 5 • 7 years ago

:sinker might also know about the problem.

	Thinker Li [:sinker]
	Comment 6 • 7 years ago

I can't reproduce it with m-c and Linux.  Does it have been fixed?

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 7 years ago

Very janky for me on Win10 still.

	Miko Mynttinen
	Comment 8 • 7 years ago

The problem here is at least partially caused by the invalidation in LayerManagerComposite::UpdateAndRender() not working properly. That is, the invalid area in http://searchfox.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#481 is empty when it should not be.

One source of the problem is the usage of Union() with IntRects instead of SafeUnion() in area calculations in LayerTreeInvalidation.cpp, where integer overflow causes the width or height of the rectangle to be negative, and therefore empty.

The test page here seems to also trigger another nsRegion related assertion: "Shouldn't return empty rect: '!mTmp.IsEmpty()', file /mozilla-unified/obj-ff-dbg/dist/include/nsRegion.h, line 389", which might be related.

I think (and hope) that this bug might be related to poor performance on Windows when running this CSS 3D transforms demo: http://www.keithclark.co.uk/labs/css-fps/nojs/

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 9 • 7 years ago

This also affects http://keithclark.co.uk/labs/css-fps/nojs/ which is a great benchmark for Advanced Layers. I'd like to make sure this gets fixed in the same release AL ships.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 10 • 7 years ago

This patch changes ComputeChangeInternal's signature so that we can return false on overflow. This is just a refactoring, no functionality changes.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 7 years ago

This changes LayerTreeInvalidation::ComputeDifferences to return false if overflow occurs. Compositors now handle this scenario by invaliding the entire window. Client-side callers treat this as invalidating the entire frame or treating it like no previous layer tree was available to compare.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 12 • 7 years ago

Advanced Layers tracks a per-container invalid region, which the old compositor does not. This patch changes the per-container callback so that we can specify the entire container should be invalidated, if for example overflow occurred.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 13 • 7 years ago

This patch adds overflow checks to LayerTreeInvalidation, by changing all Rect::Union calls to be Rect::SafeUnion. Failure is propagated up. Maybe there is a way to clamp the failure to within leaf ContainerLayers, but I don't know if it's worth it.

Initially I changed the signature of Old/NewTransformedBounds() and fixed up all their callers. However, most callers are statically operating on leaf layer types (i.e., non-ContainerLayers). So I copied the base definitions of those functions as non-virtuals to avoid refactoring all the derived layer cases.

(The MOZ_RELEASE_ASSERTS are for a try push, I'll change them to MOZ_ASSERTs before landing.)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 14 • 7 years ago

This changes the NotifySubDocInvalidation callback to accept a pointer instead of a reference. If it's null, the entire layer should be invalidated.

	Matt Woodrow (:mattwoodrow)
	Comment 15 • 7 years ago

Comment on attachment 8892325 [details] [diff] [review]
part 2, change ComputeDifferences

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +390,5 @@
>                MOZ_CRASH("Out of bounds");
>              }
>              // Invalidate any regions of the child that have changed:
> +            nsIntRegion region;
> +            mChildren[childsOldIndex]->ComputeChange(LTI_DEEPER(aPrefix), region, aCallback);

Don't we want to handle the case where this returns false?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 16 • 7 years ago

(Yes, it's handled in the 3rd+4th patches - in retrospect should have handled it here.)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 17 • 7 years ago

Sorry, part 4 had the wrong patch attached. This is the correct one.

	Pulsebot
	Comment 18 • 7 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4e23b12403
Refactor LayerTreeInvalidation::ComputeChangeInternal's signature to handle overflow. (bug 1345891 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c752b3b2520
Handle bounds overflow in consumers of LayerTreeInvalidation. (bug 1345891 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7badf38b24
Handle invalidation bounds overflow in ContainerLayerMLGPU. (bug 1345891 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ce1cb14303
Handle 3d context bounds overflow in LayerTreeInvalidation. (bug 1345891 part 4, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f886c403b88a
Change the subdoc invalidation callback to handle overflow. (bug 1345891 part 5, r=mattwoodrow)

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 19 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8d4e23b12403
https://hg.mozilla.org/mozilla-central/rev/1c752b3b2520
https://hg.mozilla.org/mozilla-central/rev/5d7badf38b24
https://hg.mozilla.org/mozilla-central/rev/a8ce1cb14303
https://hg.mozilla.org/mozilla-central/rev/f886c403b88a

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com Reporter
	Comment 20 • 7 years ago

I reproduced this issue using Fx 55.0a1, build ID: 20170309030216, on Windows 10 x64.
I can confirm this issue is fixed using build ID: 20170806100257, on Widows 10 x64.

However this issue is still reproducible using the same build on macOS X 10.12.5 and Ubuntu 14.04LTS.

	Aki Sasaki (not active)
	Comment 21 • 7 years ago

https://hg.mozilla.org/projects/date/rev/8d4e23b1240386699839fa16cccfa67a261e94b3
Refactor LayerTreeInvalidation::ComputeChangeInternal's signature to handle overflow. (bug 1345891 part 1, r=mattwoodrow)

https://hg.mozilla.org/projects/date/rev/1c752b3b2520b0f3fe93c2c1c26a6e39d4e0219c
Handle bounds overflow in consumers of LayerTreeInvalidation. (bug 1345891 part 2, r=mattwoodrow)

https://hg.mozilla.org/projects/date/rev/5d7badf38b240fce314b8466e6d6842d30145753
Handle invalidation bounds overflow in ContainerLayerMLGPU. (bug 1345891 part 3, r=mattwoodrow)

https://hg.mozilla.org/projects/date/rev/a8ce1cb143034ea116c19bc36a9d899c74767885
Handle 3d context bounds overflow in LayerTreeInvalidation. (bug 1345891 part 4, r=mattwoodrow)

https://hg.mozilla.org/projects/date/rev/f886c403b88a78035ae0e2f7fe71c830e40bcb3f
Change the subdoc invalidation callback to handle overflow. (bug 1345891 part 5, r=mattwoodrow)

	Mohammad Maruf Rahman
	Comment 22 • 7 years ago

I have reproduced this bug with Nightly 55.0a1 (2017-03-09) (64-bit) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Beta!

Build   ID    20171009192146
User Agent    Mozilla/5.0 (X11; Linuxx86_64; rv:57.0) Gecko/20100101 Firefox/57.0