Closed Bug 1253241 Opened 8 years ago Closed 8 years ago

[e10s] "Scroll of Lorem Ipsum" testcase causes graphic glitches when hide menupopup after open Inspector

Categories

(Core :: Layout, defect)

Unspecified
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s m9+ ---
firefox45 --- wontfix
firefox46 - wontfix
firefox47 + wontfix
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: alice0775, Assigned: sinker)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Attached video screencast (obsolete) —
Browser Identifier:
https://hg.mozilla.org/mozilla-central/rev/eb25b90a05c194bfd4f498ff3ffee7440f85f1cd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0 ID:20160302030209

I cannot reproduce without e10s.

Reproducible: always with e10s. 

Steps To Reproduce:
1. Reduce browser height to approx. 800px
2. Enable Menubar/Bookmark Toolbar
3. Open attachment 8710335 [details] (from bug 1241394)
4. Open(Ctrl+Shift+C)
   --- observe graphic glitches
5. Close  Inspector
6. Right click on the text or Open menu from menubar and hide it
   --- observe graphic glitches

Actual Results:
 It turns black.
 It turns black the under of the pop-up menu.
 See attached screencast


Actual Results:
 No glitches

Last Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7641104770a80015e63597b58cb312fefcbd9ab4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150917233551

First Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/621ab19e86db28c38bbbf9119fbf6831ea344c54
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150917234652

Regression Window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7641104770a80015e63597b58cb312fefcbd9ab4&tochange=621ab19e86db28c38bbbf9119fbf6831ea344c54
Attached video screencast
Attachment #8726214 - Attachment is obsolete: true
Regression from bug 1097464.
Flags: needinfo?(tlee)
I can not reproduce it on Linux with e10s.
Could you give me the information about graphics?  Like what is on the about:support page.
Flags: needinfo?(tlee) → needinfo?(alice0775)
Graphics
--------

Adapter Description: AMD Radeon HD 6450
Adapter Drivers: aticfx64 aticfx64 aticfx64 aticfx32 aticfx32 aticfx32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64
Adapter RAM: 1024
Asynchronous Pan/Zoom: wheel input enabled; touch input enabled
ClearType Parameters: Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 300
Device ID: 0x6779
Direct2D Enabled: true
DirectWrite Enabled: true (6.2.9200.17568)
Driver Date: 7-28-2015
Driver Version: 15.200.1062.1003
GPU #2 Active: false
GPU Accelerated Windows: 1/1 Direct3D 11 (OMTC)
Subsys ID: 23111787
Supports Hardware H264 Decoding: Yes
Vendor ID: 0x1002
WebGL Renderer: Google Inc. -- ANGLE (AMD Radeon HD 6450 Direct3D11 vs_5_0 ps_5_0)
windowLayerManagerRemote: true
AzureCanvasAccelerated: 0
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo
Flags: needinfo?(alice0775)
OS: Unspecified → Windows 7
AFAIK, it happens only with Windows and HW acceleration enabled.
[Tracking Requested - why for this release]:
regression from recent landing with e10s
Blocks: e10s-gfx
Flags: needinfo?(milan)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Regression from bug 1097464.

That implies that Firefox 46 is affected as well. (Does not affect Firefox 45, since bug 1097464 was backed out from there in bug 1240783.)

--> Setting status-firefox46 to affected, and requesting tracking for that release, as a visual regression. (see attached screencast)
(Though since this is e10s-only [per bug summary and comment 0], most Beta46 users won't be affected -- only those who end up in the e10s-enabled experimental group. I'm not sure how that factors into tracking-firefox46 decisions.)
sinker, are you going to have time to look at this?
Flags: needinfo?(tlee)
Looks like a terrible regression, tracked for fx47.
Tracked for 46 as well. Liz, fyi.
Flags: needinfo?(lhenry)
No longer blocks: e10s-gfx
Flags: needinfo?(milan)
I am working on.
Flags: needinfo?(tlee)
QA Contact: tlee
Thanks!
Assignee: nobody → tlee
Just an update!  Layers are drawn,  the intermediate surface of disappearing content was drawn too.  I guess somehow the content of intermediate surface was cleared.  I am looking for a way to dump the content of the surface on windows.
I'm not sure we will need to uplift this to beta 46 since the experiment is only running another week or so. It would be potentially affecting around 20% of beta users. But, if you come up with something that you think is safe you can request uplift.
Flags: needinfo?(lhenry)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> I'm not sure we will need to uplift this to beta 46 since the experiment is
> only running another week or so. It would be potentially affecting around
> 20% of beta users. But, if you come up with something that you think is safe
> you can request uplift.

This doesn't need to get uplifted to the beta experiment. We would like it in aurora if we're rolling out e10s+apz.
Comment on attachment 8734277 [details] [diff] [review]
Check mCurrentClip only for default rendering target

To render intermediate surface, even mCurrentClip is empty, it still should be drawn.  If the compositor doesn't draw for intermediate surfaces, the surface will be empty, and the content of the surface would not be shown on the screen later when the screen is updated.
Attachment #8734277 - Flags: review?(matt.woodrow)
Comment on attachment 8734277 [details] [diff] [review]
Check mCurrentClip only for default rendering target

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

Why are we rendering intermediate surfaces if the destination is entirely clipped out?

Shouldn't we make the caller of BeginFrame do the intersection between the invalid rect and the clip rect, and not do compositing if the result is empty?

We could then make BeginFrame return false if mCurrentClip.IsEmpty().
Think see Matt's question
Flags: needinfo?(tlee)
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> We could then make BeginFrame return false if mCurrentClip.IsEmpty().
If mCurrentClip is not empty and not covering the area that the intermediate surface should be drawn to, it still creates a wrong result.  For this situation, we need to do some changes to make sure next time LayrPropertiesBase::ComputeDifference() would return a right invalid region or it would not invalidate the area that was skipped here.

The line [1] in CompositorD3D11::DrawQuad() checks mCurrentRT against mDefaultRT before applying mCurrentClip.   mCurrentClip is only applied for default rendering target.  The callers also expect the rendering target be updated as its request.  Checking the current target at the early return line is reasonable to avoid unexpected result.

[1] https://dxr.mozilla.org/mozilla-central/rev/494289c72ba3997183e7b5beaca3e0447ecaf96d/gfx/layers/d3d11/CompositorD3D11.cpp#930
Flags: needinfo?(tlee)
For forget the first paragraph of th last comment, since it is based on a wrong assumption, the code is checking IsEmpty() not intersection with mCurrentClip.
Update for the discussion with Mat on IRC.
 - Check if the clip is empty at BeginFrame() and return false if it is empty.
 - add an assertion at DrawQuad() to make sure mCurrentClip will be never empty.
Attachment #8734277 - Flags: review?(matt.woodrow)
How's the follow up patch work going tlee?
Flags: needinfo?(tlee)
I am being off from the work until next Monday.  I will upload a patch for review by next Tuesday.
Flags: needinfo?(tlee)
Tlee, how's the work here going?
Flags: needinfo?(tlee)
(In reply to Jim Mathies [:jimm] from comment #26)
> Tlee, how's the work here going?

sorry, Thinker, not Tlee. My apologies.
Comment on attachment 8744295 [details] [diff] [review]
Return empty render bound for empty clip

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +1158,5 @@
> +    clipRect.IntersectRect(clipRect, IntRect(aClipRectIn->x, aClipRectIn->y, aClipRectIn->width, aClipRectIn->height));
> +  }
> +
> +  if (clipRect.IsEmpty()) {
> +    *aRenderBoundsOut = Rect();

This line would cause LayerManagerComposite stop rendering.  See |LayerManagerComposite::Render()|
Attachment #8744295 - Flags: feedback?(matt.woodrow)
(In reply to Jim Mathies [:jimm] from comment #26)
> Tlee, how's the work here going?

Hi Jim, I was busy on other tasks.  But, yeah, I am back to this bug.
Flags: needinfo?(tlee)
(In reply to Thinker Li [:sinker] from comment #29)
> Comment on attachment 8744295 [details] [diff] [review]
> Return empty render bound for empty clip
> 
> Review of attachment 8744295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +1158,5 @@
> > +    clipRect.IntersectRect(clipRect, IntRect(aClipRectIn->x, aClipRectIn->y, aClipRectIn->width, aClipRectIn->height));
> > +  }
> > +
> > +  if (clipRect.IsEmpty()) {
> > +    *aRenderBoundsOut = Rect();
> 
> This line would cause LayerManagerComposite stop rendering.  See
> |LayerManagerComposite::Render()|

What's the question here?

Stopping rendering is what we want isn't it?
(In reply to Matt Woodrow (:mattwoodrow) from comment #31)
> 
> What's the question here?
> 
> Stopping rendering is what we want isn't it?
I just want to make sure if I miss anything, or we can start review it.
Hi Alice, could you confirm if this patch work?
Flags: needinfo?(alice0775)
(In reply to Thinker Li [:sinker] from comment #33)
> Hi Alice, could you confirm if this patch work?

In local win32 build,
[0] m-c cfc7ebe59293 (latest m-c without patch)
[1] apply attachment 8734277 [details] [diff] [review] to m-c cfc7ebe59293
[2] apply attachment 8744295 [details] [diff] [review] to m-c cfc7ebe59293
[3] apply attachment 8734277 [details] [diff] [review] and attachment 8744295 [details] [diff] [review] both to m-c cfc7ebe59293

I can reproduced on [0](latest m-c without patch).
And, I cannot reproduce the problem on local build[1], [2] and [3] :).
(Though Bug 1266161 is existing)
Flags: needinfo?(alice0775)
Attachment #8744295 - Flags: feedback?(matt.woodrow) → feedback+
Looks like all we need is an r+ from matt and we're good to go here?
Flags: needinfo?(tlee)
--
Attachment #8744295 [details] [diff] - Attachment is obsolete: true
Attachment #8744295 - Attachment is obsolete: true
Flags: needinfo?(tlee)
Attachment #8748044 - Flags: review?(matt.woodrow)
Attachment #8748044 - Flags: review?(matt.woodrow) → review+
thinker, is this ready to land?
Flags: needinfo?(tlee)
(In reply to Thinker Li [:sinker] from comment #36)
> Created attachment 8748044 [details] [diff] [review]
> Return empty render bound for empty clip, v2

please checkin this patch.  Thank you!
Flags: needinfo?(tlee)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f94661bb15af
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Hi Jim, I am leaning towards wontfix'ing this one for Fx47. What do you think?
Flags: needinfo?(jmathies)
sure. we definitely want it uplifted to 48.
Flags: needinfo?(jmathies) → needinfo?(tlee)
Flags: needinfo?(tlee)
Thinker, could you fill the request to uplift to 48. Thanks.
Flags: needinfo?(tlee)
Comment on attachment 8748044 [details] [diff] [review]
Return empty render bound for empty clip, v2

Approval Request Comment
[Feature/regressing bug #]: bug 1253241
[User impact if declined]: Content glitch for pages with perserve-3d elements.
[Describe test coverage new/current, TreeHerder]: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26847bce8078
and testcase https://bugzilla.mozilla.org/attachment.cgi?id=8710335
[Risks and why]: 
Medium, change no much in logic, but changes are on a path that every frame would run through.
[String/UUID change made/needed]:
none
Flags: needinfo?(tlee)
Attachment #8748044 - Flags: approval-mozilla-beta?
Comment on attachment 8748044 [details] [diff] [review]
Return empty render bound for empty clip, v2

48 is in Aurora, flipping flags from Beta to Aurora.
Attachment #8748044 - Flags: approval-mozilla-beta? → approval-mozilla-aurora?
Comment on attachment 8748044 [details] [diff] [review]
Return empty render bound for empty clip, v2

E10s related, Aurora48+
Attachment #8748044 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Alice0775 White, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(alice0775)
Reproduced on Bad build of comment #0.
And I can confirm that the problem is fixed on latest Nightly[1].

[1]
https://hg.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 ID:20160527030220
Flags: needinfo?(alice0775)
You need to log in before you can comment on or make changes to this bug.