Bug 88886

Summary: [chromium] Don't crash in CCLayerIterator if the root layer doesn't have a render surface
Product: WebKit Reporter: Sami Kyostila <skyostil>
Component: PlatformAssignee: Sami Kyostila <skyostil>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, danakj, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89004    
Attachments:
Description Flags
Patch none

Description Sami Kyostila 2012-06-12 10:18:51 PDT
When iterating over render surface layer list, fail gracefully if the root layer in the list doesn't have a render surface.

This came up in the context of bug 73350 where we save the most recent render surface layer list for input event hit testing purposes.
Comment 1 Sami Kyostila 2012-06-12 10:25:10 PDT
Created attachment 147109 [details]
Patch
Comment 2 Dana Jansens 2012-06-12 11:07:58 PDT
I'm confused why the root layer would not have a render surface, and how saving the RenderSurfaceLayerList would affect that.
Comment 3 WebKit Review Bot 2012-06-12 12:18:08 PDT
Comment on attachment 147109 [details]
Patch

Clearing flags on attachment: 147109

Committed r120103: <http://trac.webkit.org/changeset/120103>
Comment 4 WebKit Review Bot 2012-06-12 12:18:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Dana Jansens 2012-06-12 15:25:06 PDT
Hey Sami, heard you're OOO, so to add some more comments and shorten the iteration cycle on this convo:

It seems to me that a renderSurfaceLayerList is only valid if all layers in the list have a renderSurface, and always contain the root layer.

I'm worried that this should be an ASSERT instead of silently dealing with it, as this seems like an invalid list to be iterating over to me. Can you comment on how we would get into this state in a way that would be considered valid?

Thanks!
Comment 6 Sami Kyostila 2012-06-13 03:15:19 PDT
Hi Dana. The issue was that we are saving the render surface layer list for longer than just the draw cycle. After the list is calculated, the render surfaces attached to the layers could go away, e.g., because of CCLayerTreeHostImpl:: initializeLayerRenderer(), making the list invalid.

I think currently we could catch these cases in CCLayerTreeHostImpl and invalidate the list as a side effect, but I'm worried that in the future another code path might clear the render surfaces and trigger the crash again. So this is really a defensive patch against that.

Maybe it would make sense to have both this and an ASSERT to make sure we catch the bugs in debug mode and not crash in release mode? Also, I'm only checking the root layer here because I didn't immediately see a way to make things work if an arbitrary layer has its render surface cleared.
Comment 7 Dana Jansens 2012-06-13 08:30:51 PDT
Okay! I agree about not crashing in release, so thanks for making this change. I'm going to open another bug where I'll add an assert.

The test with the empty list is perfect, but I think we should remove the unit test for the root layer with no surface (since this would hit the assert and is not valid behaviour).
Comment 8 Sami Kyostila 2012-06-13 08:46:53 PDT
(In reply to comment #7)

Thanks.

> The test with the empty list is perfect, but I think we should remove the unit test for the root layer with no surface (since this would hit the assert and is not valid behaviour).

We could also wrap it inside #ifdef NDEBUG to only run it in release builds.
Comment 9 Dana Jansens 2012-06-13 08:51:29 PDT
(In reply to comment #8)
> (In reply to comment #7)
> 
> Thanks.
> 
> > The test with the empty list is perfect, but I think we should remove the unit test for the root layer with no surface (since this would hit the assert and is not valid behaviour).
> 
> We could also wrap it inside #ifdef NDEBUG to only run it in release builds.

Ya that's true! I'm not sure if we want unit tests for "invalid" behaviours though? I'm happy either way.. JamesR what do you think, should we have that test?
Comment 10 James Robinson 2012-06-13 09:44:34 PDT
I would say no.