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)
Tracking
()
People
(Reporter: alice0775, Assigned: sinker)
References
Details
(Keywords: regression)
Attachments
(3 files, 2 obsolete files)
549.10 KB,
video/mp4
|
Details | |
1.07 KB,
patch
|
Details | Diff | Splinter Review | |
2.93 KB,
patch
|
mattwoodrow
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
Attachment #8726214 -
Attachment is obsolete: true
Assignee | ||
Comment 3•8 years ago
|
||
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)
Reporter | ||
Comment 4•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
OS: Unspecified → Windows 7
Assignee | ||
Comment 5•8 years ago
|
||
AFAIK, it happens only with Windows and HW acceleration enabled.
Comment 6•8 years ago
|
||
[Tracking Requested - why for this release]: regression from recent landing with e10s
Updated•8 years ago
|
Comment 7•8 years ago
|
||
(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)
Comment 8•8 years ago
|
||
(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.)
Looks like a terrible regression, tracked for fx47.
Tracked for 46 as well. Liz, fyi.
Flags: needinfo?(lhenry)
Assignee | ||
Comment 14•8 years ago
|
||
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.
Comment 15•8 years ago
|
||
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.
Comment 16•8 years ago
|
||
(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.
Updated•8 years ago
|
Assignee | ||
Comment 17•8 years ago
|
||
Assignee | ||
Comment 18•8 years ago
|
||
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 19•8 years ago
|
||
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().
Assignee | ||
Comment 21•8 years ago
|
||
(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)
Assignee | ||
Comment 22•8 years ago
|
||
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.
Assignee | ||
Comment 23•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8734277 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 25•8 years ago
|
||
I am being off from the work until next Monday. I will upload a patch for review by next Tuesday.
Flags: needinfo?(tlee)
Comment 27•8 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #26) > Tlee, how's the work here going? sorry, Thinker, not Tlee. My apologies.
Assignee | ||
Comment 28•8 years ago
|
||
Assignee | ||
Comment 29•8 years ago
|
||
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)
Assignee | ||
Comment 30•8 years ago
|
||
(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)
Comment 31•8 years ago
|
||
(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?
Assignee | ||
Comment 32•8 years ago
|
||
(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.
Assignee | ||
Comment 33•8 years ago
|
||
Hi Alice, could you confirm if this patch work?
Flags: needinfo?(alice0775)
Reporter | ||
Comment 34•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8744295 -
Flags: feedback?(matt.woodrow) → feedback+
Comment 35•8 years ago
|
||
Looks like all we need is an r+ from matt and we're good to go here?
Flags: needinfo?(tlee)
Assignee | ||
Comment 36•8 years ago
|
||
--
Attachment #8744295 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8744295 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(tlee)
Attachment #8748044 -
Flags: review?(matt.woodrow)
Updated•8 years ago
|
Attachment #8748044 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 38•8 years ago
|
||
(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
Comment 39•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f94661bb15af
Keywords: checkin-needed
Comment 40•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f94661bb15af
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Hi Jim, I am leaning towards wontfix'ing this one for Fx47. What do you think?
Flags: needinfo?(jmathies)
Comment 42•8 years ago
|
||
sure. we definitely want it uplifted to 48.
Flags: needinfo?(jmathies) → needinfo?(tlee)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(tlee)
Comment 43•8 years ago
|
||
Thinker, could you fill the request to uplift to 48. Thanks.
Flags: needinfo?(tlee)
Assignee | ||
Comment 44•8 years ago
|
||
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+
Comment 47•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/b519876e15d4
Hi Alice0775 White, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(alice0775)
Reporter | ||
Comment 49•8 years ago
|
||
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.
Description
•