New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 783087 link

Starred by 9 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression


Show other hotlists

Hotlists containing this issue:
Hotlist-1


Sign in to add a comment

Regression: Browser gets crashed after entering on Sign in overlay.

Reported by db...@etouch.net, Nov 9 2017

Issue description

Chrome Version: 64.0.3263.0 Revision fd1e89557fa78a467061f732a8473fd863a89b1a-refs/heads/master@{#515052} 
OS: Mac(10.12.6,10.13.2)

What steps will reproduce the problem?
(1) Launch chrome, open dev tools window and press Esc key to open Condole drawer.
(2) Navigate to Sensors section, change drop down option from 'Touch' and reload the page.
(3) Observe.

Actual: Browser gets crashed.

CrashId: Uploaded Crash Report ID 3aa3e808c3369b6c (Local Crash ID: d0430ffd-f52b-42e2-b11f-2a22445797a3)

Expected: Browser should not get crash.

This is a regression issue, broken in 'M64'will soon update the other info:

Good Build: 64.0.3262.0
Bad Build: 64.0.3263.0

Note: issue is not seen on Windows and Linux OS.

 
Actual_Crash.mov
13.1 MB Download

Comment 1 by db...@etouch.net, Nov 9 2017

Labels: hasbisect-per-revision
Owner: danakj@chromium.org
Status: Assigned (was: Unconfirmed)
You are probably looking for a change made after 515042 (known good), but no later than 515043 (first known bad).
CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.
  https://chromium.googlesource.com/chromium/src/+log/e90c1f7c2699453a150a46b34c90c905185cd008..43d70d5dd490d5bf146cbb35c79a53edcbdf3a25

Suspect: https://chromium.googlesource.com/chromium/src/+/43d70d5dd490d5bf146cbb35c79a53edcbdf3a25

Cc: gov...@chromium.org ligim...@chromium.org
Labels: ReleaseBlock-Beta
Stack Trace for the Crash ID provided:
--------------------------------------
Thread 0 (id: 84878) CRASHED [EXC_BAD_ACCESS / KERN_INVALID_ADDRESS @ 0x000000a0 ] MAGIC SIGNATURE THREAD
Stack Quality70%Show frame trust levels
0x000000010453915a	(Google Chrome Framework -color_space.cc:168 )	gfx::ColorSpace::operator!=(gfx::ColorSpace const&) const
0x000000010499077c	(Google Chrome Framework -resource_pool.cc:172 )	cc::ResourcePool::ReuseResource(gfx::Size const&, viz::ResourceFormat, gfx::ColorSpace const&)
0x0000000104990bab	(Google Chrome Framework -resource_pool.cc:220 )	cc::ResourcePool::AcquireResource(gfx::Size const&, viz::ResourceFormat, gfx::ColorSpace const&)
0x0000000104e999ce	(Google Chrome Framework -gl_renderer.cc:3328 )	viz::GLRenderer::CopyRenderPassDrawQuadToOverlayResource(viz::CALayerOverlay const*, cc::Resource**, gfx::RectF*)
0x0000000104e990e4	(Google Chrome Framework -gl_renderer.cc:3418 )	viz::GLRenderer::ScheduleRenderPassDrawQuad(viz::CALayerOverlay const*)
0x0000000104e97141	(Google Chrome Framework -gl_renderer.cc:3124 )	viz::GLRenderer::ScheduleCALayers()
0x0000000104e969f1	(Google Chrome Framework -gl_renderer.cc:2603 )	viz::GLRenderer::FinishDrawingFrame()
0x0000000104e82195	(Google Chrome Framework -direct_renderer.cc:365 )	viz::DirectRenderer::DrawFrame(std::__1::vector<std::__1::unique_ptr<viz::RenderPass, std::__1::default_delete<viz::RenderPass> >, std::__1::allocator<std::__1::unique_ptr<viz::RenderPass, std::__1::default_delete<viz::RenderPass> > > >*, float, gfx::Size const&)
0x0000000104e865dd	(Google Chrome Framework -display.cc:349 )	viz::Display::DrawAndSwap()
0x0000000104e893b4	(Google Chrome Framework -display_scheduler.cc:202 )	viz::DisplayScheduler::DrawAndSwap()
0x0000000104e885ab	(Google Chrome Framework -display_scheduler.cc:489 )	viz::DisplayScheduler::OnBeginFrameDeadline()
0x0000000104c18a45	(Google Chrome Framework -callback.h:65 )	ui::(anonymous namespace)::WrappedTask::Run()
0x0000000103b0985b	(Google Chrome Framework -callback.h:65 )	base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)
0x0000000103b2eac3	(Google Chrome Framework -message_loop.cc:394 )	base::MessageLoop::RunTask(base::PendingTask*)
0x0000000103b2eff8	(Google Chrome Framework -message_loop.cc:406 )	base::MessageLoop::DoWork()
0x0000000103b30e29	(Google Chrome Framework -message_pump_mac.mm:452 )	base::MessagePumpCFRunLoopBase::RunWork()
0x0000000103b21ce9	(Google Chrome Framework + 0x01cface9 )	base::mac::CallWithEHFrame(void () block_pointer)
0x0000000103b3074e	(Google Chrome Framework -message_pump_mac.mm:428 )	base::MessagePumpCFRunLoopBase::RunWorkSource(void*)
0x00007fff8a0e57e0	(CoreFoundation + 0x000aa7e0 )	
0x00007fff8a0c4f0b	(CoreFoundation + 0x00089f0b )	
0x00007fff8a0c442e	(CoreFoundation + 0x0008942e )	
0x00007fff8a0c3e27	(CoreFoundation + 0x00088e27 )	
0x00007fff980ba934	(HIToolbox + 0x00030934 )	RunCurrentEventLoopInMode
0x00007fff980ba76e	(HIToolbox + 0x0003076e )	ReceiveNextEventCommon
0x00007fff980ba5ae	(HIToolbox + 0x000305ae )	_BlockUntilNextEventMatchingListInModeWithFilter
0x00007fff93e98df5	(AppKit + 0x00048df5 )	_DPSNextEvent
0x00007fff93e98225	(AppKit + 0x00048225 )	-[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
0x0000000103772c5f	(Google Chrome Framework -chrome_browser_application_mac.mm:175 )	__71-[BrowserCrApplication nextEventMatchingMask:untilDate:inMode:dequeue:]_block_invoke
0x0000000103b21ce9	(Google Chrome Framework + 0x01cface9 )	base::mac::CallWithEHFrame(void () block_pointer)
0x0000000103772ba3	(Google Chrome Framework -chrome_browser_application_mac.mm:174 )	-[BrowserCrApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
0x00007fff93e8cd7f	(AppKit + 0x0003cd7f )	-[NSApplication run]
0x0000000103b316e6	(Google Chrome Framework -message_pump_mac.mm:804 )	base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*)
0x0000000103b3026d	(Google Chrome Framework -message_pump_mac.mm:179 )	base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*)
0x0000000103b533c3	(Google Chrome Framework -run_loop.cc:114 )	<name omitted>
0x00000001037784c7	(Google Chrome Framework -chrome_browser_main.cc:1919 )	ChromeBrowserMainParts::MainMessageLoopRun(int*)
0x00000001025843f3	(Google Chrome Framework -browser_main_loop.cc:1206 )	content::BrowserMainLoop::RunMainMessageLoopParts()
0x00000001025874b1	(Google Chrome Framework -browser_main_runner.cc:140 )	content::BrowserMainRunnerImpl::Run()
0x000000010258092b	(Google Chrome Framework -browser_main.cc:46 )	content::BrowserMain(content::MainFunctionParams const&)
0x000000010372d9ee	(Google Chrome Framework -content_main_runner.cc:705 )	content::ContentMainRunnerImpl::Run()
0x0000000104f97dfa	(Google Chrome Framework -main.cc:456 )	service_manager::Main(service_manager::MainParams const&)
0x000000010372cfa3	(Google Chrome Framework -content_main.cc:19 )	content::ContentMain(content::ContentMainParams const&)
0x0000000101e2ae6e	(Google Chrome Framework -chrome_main.cc:125 )	ChromeMain
0x0000000101bc2dd3	(Google Chrome -chrome_exe_main_mac.cc:165 )	main
0x00007fff9b6df5ac	(libdyld.dylib + 0x000035ac )	
0x00007fff9b6df5ac	(libdyld.dylib + 0x000035ac )	

Adding release blocker for this issue.Please undo if not the case.

Thank You!

Comment 3 by db...@etouch.net, Nov 9 2017

Summary: Regression: Browser gets crashed after entering on Sign in overlay. (was: Regression: Browser gets crashed after opening devtools and relaoding page.)
Other steps to reproduce this issue:

1. Launch chrome, navigate to NTP and click on Avatar icon.
2. Now click on 'Sign in to chrome', enter email id and press Enter key.
3. Observe

Actual: Browser gets crashed.
Labels: -ReleaseBlock-Beta ReleaseBlock-Dev
Updating blocker priority for this issue as per comment #3.Please decrease the priority if not the case.

Thank You! 

Comment 5 by db...@etouch.net, Nov 9 2017

Components: Services>SignIn
Cc: abdulsyed@chromium.org
Cc: danakj@chromium.org dpranke@chromium.org perezju@chromium.org
 Issue 783198  has been merged into this issue.
Cc: spang@chromium.org enne@chromium.org
 Issue 783088  has been merged into this issue.
Cc: -perezju@chromium.org -danakj@chromium.org -abdulsyed@chromium.org -gov...@chromium.org -nyerramilli@chromium.org -rbasuvula@chromium.org -ligim...@chromium.org -dpranke@chromium.org -msrchandra@chromium.org -ranjitkan@chromium.org -spang@chromium.org xing...@intel.com ccameron@chromium.org wutao@chromium.org
I think what happens here is:

1. Theres 2 renderpasses, B=child, A=root.
2. ProcessForOverlays() turns a RPDQ into an overlay (CALayer).
3. B is skipped for drawing, for whatever reason (cached?).
4. A is skipped because skip_drawing_root_render_pass is true.
5. So UseRenderPass is never called, and current_render_pass is null.
6. ScheduleCALayers calls ScheduleRenderPassDrawQuad for the scheduled RPDQ overlay.
7. ScheduleRenderPassDrawQuad makes a resource pool, then calls CopyRenderPassDrawQuadToOverlayResource
8. CopyRenderPassDrawQuadToOverlayResource wants to know the color space of the current_render_pass.

current_render_pass is still null in 8, so we crash.

However, before the patch, current_render_pass would have been the child which is the incorrect renderpass, so the color space would be wrong.

If ProcessForOverlays() is indeed only making overlays from the root renderpass, then this should be asking the root renderpass. Or we need to store the renderpass with the rpdq for the CALayer code.
Welp. I put a NOTREACHED() in GLRenderer::CopyRenderPassDrawQuadToOverlayResource() and not a single unit test fails. :(
Issue 783275 has been merged into this issue.
I have seen the last error msg in Issue 783275:

cc::ResourceProvider::LockForRead without having first written to the resource will result in this error.


Status: Started (was: Assigned)
Ok I have a unit test that reproduces the CALayer mac crash. I think the LockForRead one is a bit different, and need to investigate that next.
GLRenderer::SetUseProgram() also uses current_frame()->current_render_pass->color_space, which was the wrong render pass before, and null with my patch.
#0  gfx::ColorSpace::IsValid (this=0xa0) at ../../ui/gfx/color_space.cc:71
#1  0x00007ffff39a51ee in viz::GLRenderer::SetUseProgram (this=0x7fffffffc478, program_key_no_color=..., src_color_space=..., dst_color_space=...)
    at ../../components/viz/service/display/gl_renderer.cc:2986
#2  0x00007ffff399b6db in viz::GLRenderer::SetUseProgram (this=0x7fffffffc478, program_key=..., src_color_space=...) at ../../components/viz/service/display/gl_renderer.cc:2979
#3  0x00007ffff39a02d1 in viz::GLRenderer::ChooseRPDQProgram (this=0x7fffffffc478, params=0x7fffffff9748) at ../../components/viz/service/display/gl_renderer.cc:1408
#4  0x00007ffff39ad895 in viz::GLRenderer::CopyRenderPassDrawQuadToOverlayResource (this=0x7fffffffc478, ca_layer_overlay=0x25c1304e9720, resource=0x7fffffff9f68, 
    new_bounds=0x7fffffff9f58) at ../../components/viz/service/display/gl_renderer.cc:3390

Issue 783443 has been merged into this issue.
Cc: erikc...@chromium.org
So a horrible fix would be to say "well, if we don't have a current render pass, then you must have meant the root render pass for the current frame"
I wonder if the ReadLock one is the DCHECK in ComputeScissorRectForRenderPass() being violated.

It says:
  DCHECK(
      current_render_pass->copy_requests.empty() ||
      (current_render_pass->damage_rect == current_render_pass->output_rect));

That is, the pass damage rect is the output rect if there's a copy. Presumably the output rect is not empty.. then the damage rect is not empty and we won't skip the pass due to ComputeScissorRectForRenderPass().IsEmpty().

If we did skip it then we won't have a texture anymore, as wutao pointed out on the CL.

GLRenderer::CopyRenderPassDrawQuadToOverlayResource() does:

  cc::ScopedResource* contents_texture =
      render_pass_textures_[ca_layer_overlay->rpdq->render_pass_id].get();
  ...
  params.contents_texture = contents_texture;

So it's grabbing the draw quad's texture. If we skipped and didn't draw it, then it would crash when UpdateRPDQTexturesForSampling() does:

    params->contents_resource_lock =
        base::MakeUnique<cc::DisplayResourceProvider::ScopedSamplerGL>(
            resource_provider_, params->contents_texture->id(), GL_LINEAR);

The id there will be 0, which will crash when ResourceProvider tries to use a thing not in the resource map.

The stack is coming from a copy request which is what made me think this
> So a horrible fix would be to say "well, if we don't have a current render pass, then you must have meant the root render pass for the current frame"

I did something better for the color space stuff in https://chromium-review.googlesource.com/c/chromium/src/+/760558 PS4.
Issue 783271 has been merged into this issue.
Oh so crash/ has some of the source indexed but not all on the other crash, but what it is is:

1. DirectRenderer::DrawFrame() calls DrawRenderPassAndExecuteCopyRequests(root_render_pass);
2. DirectRenderer::DrawRenderPassAndExecuteCopyRequests() calls DrawRenderPass()
3. ... we end up in DoDrawQuad->DrawRenderPassDrawQuad .. etc

So this isn't a copy request my above theory is wrong. It's that the root RenderPass has a RPDQ to a RenderPass that got skipped. Maybe cuz it had no damage. The damage needs to consider the bigger picture in order to early out like that.
The key thot about damage I have is that damage is aggregated up from children to the root. So just cuz the child has no damage doesn't mean that the root has no damage that overlaps it (or if we're not using partial swap) that promises the child won't be drawn.

I need to revisit the early out when it was added to see its intentions again.
I was thinking due to partial swap, the ScissorRect could be empty (the damage_rect is empty because it is out of view port for the first frame [1]), but it becomes visible later for the second frame?

[1] copy_pass->damage_rect.Intersect(damage_rect_in_render_pass_space) in the code. 
https://cs.chromium.org/chromium/src/components/viz/service/display/surface_aggregator.cc?l=737&rcl=9fd12adae19d303c4106393d4ccdd1598355c9c5

Oh I think I get the intent of the ComputeScissorRectForRenderPass(). It doesn't even check for partial swap, it wasn't meant to skip passes we don't have damage for at all. It was really only to prevent caching in some cases.

Basically caching isn't used if the renderpass says it has damage in it, which is the has_damage_from_contributing_content flag. But SurfaceAggregator may have added some damage to the surface in some cases (which is https://codereview.chromium.org/1927413002 - where its saying if there's a filter then we dont know the damage, so damage everything).

However problems that I see here:

1. It damages everything even if there was no damage before, if it sees any RPDQ that has a filter.
2. It damages everything if the RP has a filter, even if the RP (with filter) don't intersect the root RP.

These 2 decrease out efficiency in the presence of pixel-moving filters proportional to their size when anything else on the page changes.

3. It copies damage from the root into the child, which will prevent caching from being used when anything is changing below/above the RenderPass for no reason.

This one decreases our ability to make use of RenderPass cacheing.

But importantly for this bug, we shouldn't skip renderpasses based on the scissor rect, only prevent cacheing.
Cc: piman@chromium.org
And regarding caching also:

We have a flag RenderPass::has_damage_from_contributing_content, which is supposed to be true if the texture for the RenderPass needs to be redrawn (ie damage is present). It ignores damage on the mask since that is applied after the RenderPass is drawn (https://cs.chromium.org/chromium/src/cc/trees/damage_tracker.cc?rcl=20b971fb48c5496b6925b84952f42c47b606e5eb&l=178)

But the damage from the mask will cause damage in the root that overlaps the RenderPass (cuz thats the mask's job) so it makes this bool no different than just checking the render pass damage_rect. In otherwords the damage rect being non-empty should be a superset of the cases where has_damage_from_contributing_content is true, so as long as we dont cache when the child has any damage that intersects it in the root, there's no reason for has_damage_from_contributing_content to exist AFAICT.
Actually SurfaceAggregator always makes the damage rect on the RenderPass == output_rect when |cache_render_pass| is true. So, I don't actually see how we would ever use the cached texture in that case, because of the ComputeScissorRectForRenderPass() check.

Comment 28 by piman@chromium.org, Nov 10 2017

Cc: reve...@chromium.org

Comment 29 by wutao@chromium.org, Nov 10 2017

#27, there are only a few places turning on caching (login UI/overview mode on CrOS, window close animation, window maximization animation).
Yeah, I think that this ComputeScissorRect check will make us redraw the cached surface every frame no matter what we do. At least I can't see how not. I also think it's not needed.
I have a unit test to reproduce the 2nd crash:

[30761:30761:1109/201807.001664:3236984305987:FATAL:display_resource_provider.cc(387)] Check failed: resource->allocated. 
#0 0x7f98c32edda7 base::debug::StackTrace::StackTrace()
#1 0x7f98c3316111 logging::LogMessage::~LogMessage()
#2 0x7f98c21d7a9e cc::DisplayResourceProvider::LockForRead()
#3 0x7f98c21d82ba cc::DisplayResourceProvider::ScopedSamplerGL::ScopedSamplerGL()
#4 0x7f98c188a7dd viz::GLRenderer::UpdateRPDQTexturesForSampling()
#5 0x7f98c1888c1f viz::GLRenderer::DrawRenderPassQuadInternal()
#6 0x7f98c1884636 viz::GLRenderer::DrawRenderPassQuad()
#7 0x7f98c18835de viz::GLRenderer::DoDrawQuad()
#8 0x7f98c18741f8 viz::DirectRenderer::DrawRenderPass()

Oops, here's a better one :)

[33660:33660:1109/203106.200077:3237763504398:FATAL:gl_renderer.cc(1113)] Check failed: contents_texture->id(). 
#0 0x7efd6941bda7 base::debug::StackTrace::StackTrace()
#1 0x7efd69444111 logging::LogMessage::~LogMessage()
#2 0x7efd679b26a8 viz::GLRenderer::DrawRenderPassQuad()
#3 0x7efd679b166e viz::GLRenderer::DoDrawQuad()
#4 0x7efd679a2288 viz::DirectRenderer::DrawRenderPass()
#5 0x7efd679a13cb viz::DirectRenderer::DrawRenderPassAndExecuteCopyRequests()
#6 0x7efd679a0edb viz::DirectRenderer::DrawFrame()

The id is 0 which means it uses a resource not in the map as I was thinking. The test simply draws a frame with 2 renderpass, and no damage overlapping the child, without partial swap on.
Issue 783537 has been merged into this issue.
Issue 783543 has been merged into this issue.
Issue 783542 has been merged into this issue.
783542 has a slightly different call stack that also looks like a 0 resource id.

0x00007ffc0f0d78c6 (chrome.dll - display_resource_provider.cc: 429)	cc::DisplayResourceProvider::UnlockForRead(unsigned int)
0x00007ffc0f0d7d32 (chrome.dll - display_resource_provider.cc: 473)	cc::DisplayResourceProvider::ScopedSamplerGL::~ScopedSamplerGL()
0x00007ffc0e6a758d (chrome.dll - gl_renderer.cc: 187)	viz::DrawRenderPassDrawQuadParams::~DrawRenderPassDrawQuadParams()
0x00007ffc0e6a4488 (chrome.dll - gl_renderer.cc: 1120)	viz::GLRenderer::DrawRenderPassQuad(viz::RenderPassDrawQuad const *,gfx::QuadF const *)
0x00007ffc0e6b635e (chrome.dll - direct_renderer.cc: 595)	viz::DirectRenderer::DrawRenderPass(viz::RenderPass const *)
0x00007ffc0e6b586a (chrome.dll - direct_renderer.cc: 487)	viz::DirectRenderer::DrawRenderPassAndExecuteCopyRequests(viz::RenderPass *)
0x00007ffc0e6b550f (chrome.dll - direct_renderer.cc: 333)	viz::DirectRenderer::DrawFrame(std::vector<std::unique_ptr<viz::RenderPass,std::default_delete<viz::RenderPass> >,std::allocator<std::unique_ptr<viz::RenderPass,std::default_delete<viz::RenderPass> > > > *,float,gfx::Size const &)
0x00007ffc0e68f0b7 (chrome.dll - display.cc: 349)	viz::Display::DrawAndSwap()

But in this case, somehow it managed to LockForRead, but then crashes on UnlockForRead later.

I feel confident the other fixes will resolve this too, but am not sure how this would happen either.
Labels: -OS-Mac
void DisplayResourceProvider::UnlockForRead(viz::ResourceId id) {
  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
  ResourceMap::iterator it = resources_.find(id);
  CHECK(it != resources_.end());  <--- Where that last crash happens. So its working with a resource not in the map. Somehow on lock it manages to not crash, while using the map's end() iterator, so I guess it just did some undefined stuff instead with a random Resource* pointer. There's no CHECK on the lock but there is on the unlock.
Project Member

Comment 39 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/986d87a5fada7910886f1fc07e592c7698da128f

commit 986d87a5fada7910886f1fc07e592c7698da128f
Author: danakj <danakj@chromium.org>
Date: Fri Nov 10 19:30:54 2017

When calling UseRenderPass() to restore, ensure it runs.

UseRenderPass() has two use cases, either to set up a render pass
as the framebuffer for drawing at the start of the draw, or to restore
us back to that state after making some intermediate change to which
framebuffer is bound.

UseRenderPass() has some optimizations where it can return false and
not do anything, if it detects that we don't want to draw the render
pass. The first use case, in DrawRenderPass() makes use of this return
value to avoid drawing anything.

Other callers to UseRenderPass() do not check the return value because
we're already drawing the pass and they are trying to restore state. If
it early outs for these it would be unexpected and produce bugs.

Currently DecideRenderPassAllocationsForFrame modifies render passes to
try avoid such bugs, but there are other ways to encounter the bug.

This changes UseRenderPass() to never early out so the latter cases will
not avoid resetting the framebuffer. The former case can avoid
UseRenderPass() entirely by checking CanSkipRenderPass(). If it is true,
then it does not need to be drawn.

Also, we will now ensure that UseRenderPass() is called after using a
ScopedUseGrContext when drawing a RenderPassDrawQuad. Currently on errors
it would early out before restoring the framebuffer, which could cause
bugs for the next quads in the RenderPass. This was introduced when the
UseRenderPass() call was moved out of the ~ScopedUseGrContext()
destructor in https://codereview.chromium.org/2203033005.

When skipping a RenderPass, current_render_pass is no longer set, which
causes a crash in code that assumes it is, incorrectly. So this updates
some paths in RenderPassDrawQuad drawing to avoid crashing, by passing
the colorspace directly instead of trying to pull it off
current_render_pass, since the caller knows where to find the correct
one, and by using the root_render_pass instead for copying
RenderPassDrawQuads to textures, since they draw into the root.

This all came up via https://chromium-review.googlesource.com/c/chromium/src/+/757027#message-b6e905925e834419ff448e2093a392053d5d321a

R=enne@chromium.org, wutao@chromium.org

Bug:  782044 ,  738190 , 782042,  783087 
Change-Id: I62ea68be6b3008ec8b8370466db0e26321f022f9
Reviewed-on: https://chromium-review.googlesource.com/760558
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515633}
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/direct_renderer.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/direct_renderer.h
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer.h
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/gl_renderer_unittest.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/renderer_pixeltest.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/skia_renderer.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/skia_renderer.h
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/software_renderer.cc
[modify] https://crrev.com/986d87a5fada7910886f1fc07e592c7698da128f/components/viz/service/display/software_renderer.h

Issue 783551 has been merged into this issue.
Status: Fixed (was: Started)
One more followup coming to fix caching.

I added a TODO in CanSkipRenderPass():

  // TODO(crbug.com/783275): It's possible to skip a child RenderPass if damage
  // does not overlap it, since that means nothing has changed:
  //   ComputeScissorRectForRenderPass(render_pass).IsEmpty()
  // However that caused crashes where the RenderPass' texture was not present
  // (never seen the RenderPass before, or the texture was deleted when not used
  // for a frame). It could avoid skipping if there is no texture present, which
  // is what was done for a while, but this seems to papering over a missing
  // damage problem, or we're failing to understand the system wholey.
  // If attempted again this should probably CHECK() that the texture exists,
  // and attempt to figure out where the new RenderPass texture without damage
  // is coming from.

wutao@ suggested maybe that SurfaceAggregator changes renderpass ids, which would make the texture get lost without damage. I'm not sure tho, it would be surprising cuz the RPDQ would need to change too.
Project Member

Comment 42 by bugdroid1@chromium.org, Nov 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd829cadf22696834edf4aefc9033f5c8e272183

commit bd829cadf22696834edf4aefc9033f5c8e272183
Author: danakj <danakj@chromium.org>
Date: Sat Nov 11 00:19:32 2017

Remove caching avoidance when there is a non-empty scissor.

See https://chromium-review.googlesource.com/c/chromium/src/+/760558#message-83efb689457770dfabb6d7371847f06f8091dfd7
and https://bugs.chromium.org/p/chromium/issues/detail?id=783087#c24
for explanations, but tl;dr this check would break caching, and
shouldn't be there. Before it was used to skip render passes
regardless of caching, but incorrectly, and the unittest in
https://chromium-review.googlesource.com/c/chromium/src/+/760558
demonstrates that, and fails with that check.

This removes the check from the caching block.

R=enne@chromium.org, wutao@chromium.org

Bug:  782044 ,  738190 , 782042,  783087 
Change-Id: Idc5baea6da5e6d663da9c1aa23ce81a55e61168d
Reviewed-on: https://chromium-review.googlesource.com/763710
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515757}
[modify] https://crrev.com/bd829cadf22696834edf4aefc9033f5c8e272183/components/viz/service/display/direct_renderer.cc
[modify] https://crrev.com/bd829cadf22696834edf4aefc9033f5c8e272183/components/viz/service/display/gl_renderer_unittest.cc

Sign in to add a comment