Bug 88886

Created attachment 147109 [details] Patch

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

Comment on attachment 147109 [details] Patch Clearing flags on attachment: 147109 Committed r120103: <http://trac.webkit.org/changeset/120103>

All reviewed patches have been landed. Closing bug.

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!

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.

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

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

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

I would say no.