Bug 88886 – [chromium] Don't crash in CCLayerIterator if the root layer doesn't have a render surface

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sami Kyostila]

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.

[Sami Kyostila]

Created attachment 147109 [details]
Patch

[Dana Jansens]

I'm confused why the root layer would not have a render surface, and how saving the RenderSurfaceLayerList would affect that.

[WebKit Review Bot]

Comment on attachment 147109 [details]
Patch

Clearing flags on attachment: 147109

Committed r120103: <http://trac.webkit.org/changeset/120103>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

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!

[Sami Kyostila]

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.

[Dana Jansens]

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).

[Sami Kyostila]

(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.

[Dana Jansens]

(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?

[James Robinson]

I would say no.