88886 2012-06-12 10:18:51 -0700 [chromium] Don't crash in CCLayerIterator if the root layer doesn't have a render surface 2012-06-13 09:44:34 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 89004 1 skyostil skyostil cc-bugs danakj jamesr webkit.review.bot oldest_to_newest 647037 0 skyostil 2012-06-12 10:18:51 -0700 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. 647043 1 147109 skyostil 2012-06-12 10:25:10 -0700 Created attachment 147109 Patch 647092 2 danakj 2012-06-12 11:07:58 -0700 I'm confused why the root layer would not have a render surface, and how saving the RenderSurfaceLayerList would affect that. 647169 3 147109 webkit.review.bot 2012-06-12 12:18:08 -0700 Comment on attachment 147109 Patch Clearing flags on attachment: 147109 Committed r120103: <http://trac.webkit.org/changeset/120103> 647170 4 webkit.review.bot 2012-06-12 12:18:12 -0700 All reviewed patches have been landed. Closing bug. 647406 5 danakj 2012-06-12 15:25:06 -0700 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! 647853 6 skyostil 2012-06-13 03:15:19 -0700 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. 648058 7 danakj 2012-06-13 08:30:51 -0700 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). 648072 8 skyostil 2012-06-13 08:46:53 -0700 (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. 648077 9 danakj 2012-06-13 08:51:29 -0700 (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? 648148 10 jamesr 2012-06-13 09:44:34 -0700 I would say no. 147109 2012-06-12 10:25:10 -0700 2012-06-12 12:18:08 -0700 Patch bug-88886-20120612182509.patch text/plain 3693 skyostil U3VidmVyc2lvbiBSZXZpc2lvbjogMTIwMDUxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZDdlNmMwODc1NzY1NGZk MGIzMDJmMWY2Yzc4NDMxMzEwYThhNGUxYS4uYzljM2IzMWI3NGJjNDI4OWFjZDg2ZDMyNWM2MjVk OWQ2ODU5ODJkZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTA2LTEyICBTYW1p IEt5b3N0aWxhICA8c2t5b3N0aWxAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFtjaHJvbWl1bV0g RG9uJ3QgY3Jhc2ggaW4gQ0NMYXllckl0ZXJhdG9yIGlmIHRoZSByb290IGxheWVyIGRvZXNuJ3Qg aGF2ZSBhIHJlbmRlciBzdXJmYWNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD04ODg4NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIElmIHdlIGFyZSBpdGVyYXRpbmcgb3ZlciBhIHJlbmRlciByZW5kZXIgc3Vy ZmFjZSBsYXllciBsaXN0IHdoZXJlIHRoZQorICAgICAgICByb290IGxheWVyIGRvZXMgbm90IGhh dmUgYSByZW5kZXIgc3VyZmFjZSwgZmFpbCBncmFjZWZ1bGx5IGluc3RlYWQgb2YKKyAgICAgICAg Y3Jhc2hpbmcuCisKKyAgICAgICAgVGVzdHM6IENDTGF5ZXJJdGVyYXRvclRlc3Que2VtcHR5VHJl ZSxyb290TGF5ZXJXaXRob3V0UmVuZGVyU3VyZmFjZX0KKworICAgICAgICAqIHBsYXRmb3JtL2dy YXBoaWNzL2Nocm9taXVtL2NjL0NDTGF5ZXJJdGVyYXRvci5oOgorICAgICAgICAoV2ViQ29yZTo6 Q0NMYXllckl0ZXJhdG9yOjpDQ0xheWVySXRlcmF0b3IpOgorCiAyMDEyLTAzLTI4ICBTYW1pIEt5 b3N0aWxhICA8c2t5b3N0aWxAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFtjaHJvbWl1bV0gQWxs b3cgc2Nyb2xsaW5nIG5vbi1yb290IGxheWVycyBpbiB0aGUgY29tcG9zaXRvciB0aHJlYWQKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0ND TGF5ZXJJdGVyYXRvci5oIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21p dW0vY2MvQ0NMYXllckl0ZXJhdG9yLmgKaW5kZXggMTJjNjI3ZjZhMmU0YjliMTU5ZjgxNDBmYmRm NTdkYWNlNzg4MDY4Zi4uMDZlMTAyZjE1ODg0ZTQ5NjNkYjU2ZWRmNDU5MTg2MDU2NjhjMjhlMCAx MDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vY2Mv Q0NMYXllckl0ZXJhdG9yLmgKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv Y2hyb21pdW0vY2MvQ0NMYXllckl0ZXJhdG9yLmgKQEAgLTE0Myw4ICsxNDMsOSBAQCBwdWJsaWM6 CiBwcml2YXRlOgogICAgIENDTGF5ZXJJdGVyYXRvcihjb25zdCBMYXllckxpc3QqIHJlbmRlclN1 cmZhY2VMYXllckxpc3QsIGJvb2wgc3RhcnQpCiAgICAgICAgIDogbV9yZW5kZXJTdXJmYWNlTGF5 ZXJMaXN0KHJlbmRlclN1cmZhY2VMYXllckxpc3QpCisgICAgICAgICwgbV90YXJnZXRSZW5kZXJT dXJmYWNlTGF5ZXJJbmRleCgwKQogICAgIHsKLSAgICAgICAgaWYgKHN0YXJ0ICYmICFyZW5kZXJT dXJmYWNlTGF5ZXJMaXN0LT5pc0VtcHR5KCkpCisgICAgICAgIGlmIChzdGFydCAmJiAhcmVuZGVy U3VyZmFjZUxheWVyTGlzdC0+aXNFbXB0eSgpICYmIHRhcmdldFJlbmRlclN1cmZhY2UoKSkKICAg ICAgICAgICAgIG1fYWN0aW9ucy5iZWdpbigqdGhpcyk7CiAgICAgICAgIGVsc2UKICAgICAgICAg ICAgIG1fYWN0aW9ucy5lbmQoKnRoaXMpOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9jaHJv bWl1bS90ZXN0cy9DQ0xheWVySXRlcmF0b3JUZXN0LmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21p dW0vdGVzdHMvQ0NMYXllckl0ZXJhdG9yVGVzdC5jcHAKaW5kZXggNWZjNjI1ZjIyMTkwMTdhNDhk YzAxNjYzZWU3ZWViMzZkY2QyOGE0My4uY2ZjODZiMDVhZTcyMzg3YWVmNmZmM2IwMGM2Y2RjMGQw YzE3YmNmNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS90ZXN0cy9DQ0xheWVy SXRlcmF0b3JUZXN0LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3Rlc3RzL0NDTGF5 ZXJJdGVyYXRvclRlc3QuY3BwCkBAIC04Miw2ICs4Miw4IEBAIHZvaWQgcmVzZXRDb3VudHMoVmVj dG9yPFJlZlB0cjxMYXllckNocm9taXVtPiA+JiByZW5kZXJTdXJmYWNlTGF5ZXJMaXN0KQogICAg ICAgICByZW5kZXJTdXJmYWNlTGF5ZXItPm1fY291bnRSZXByZXNlbnRpbmdDb250cmlidXRpbmdT dXJmYWNlID0gLTE7CiAgICAgICAgIHJlbmRlclN1cmZhY2VMYXllci0+bV9jb3VudFJlcHJlc2Vu dGluZ0l0c2VsZiA9IC0xOwogCisgICAgICAgIGlmICghcmVuZGVyU3VyZmFjZSkKKyAgICAgICAg ICAgIGNvbnRpbnVlOwogICAgICAgICBmb3IgKHVuc2lnbmVkIGxheWVySW5kZXggPSAwOyBsYXll ckluZGV4IDwgcmVuZGVyU3VyZmFjZS0+bGF5ZXJMaXN0KCkuc2l6ZSgpOyArK2xheWVySW5kZXgp IHsKICAgICAgICAgICAgIFRlc3RMYXllckNocm9taXVtKiBsYXllciA9IHN0YXRpY19jYXN0PFRl c3RMYXllckNocm9taXVtKj4ocmVuZGVyU3VyZmFjZS0+bGF5ZXJMaXN0KClbbGF5ZXJJbmRleF0u Z2V0KCkpOwogCkBAIC0xMjIsNiArMTI0LDE0IEBAIHZvaWQgaXRlcmF0ZUJhY2tUb0Zyb250KFZl Y3RvcjxSZWZQdHI8TGF5ZXJDaHJvbWl1bT4gPiogcmVuZGVyU3VyZmFjZUxheWVyTGlzdCkKICAg ICB9CiB9CiAKK1RFU1QoQ0NMYXllckl0ZXJhdG9yVGVzdCwgZW1wdHlUcmVlKQoreworICAgIFZl Y3RvcjxSZWZQdHI8TGF5ZXJDaHJvbWl1bT4gPiByZW5kZXJTdXJmYWNlTGF5ZXJMaXN0OworCisg ICAgaXRlcmF0ZUJhY2tUb0Zyb250KCZyZW5kZXJTdXJmYWNlTGF5ZXJMaXN0KTsKKyAgICBpdGVy YXRlRnJvbnRUb0JhY2soJnJlbmRlclN1cmZhY2VMYXllckxpc3QpOworfQorCiBURVNUKENDTGF5 ZXJJdGVyYXRvclRlc3QsIHNpbXBsZVRyZWUpCiB7CiAgICAgUmVmUHRyPFRlc3RMYXllckNocm9t aXVtPiByb290TGF5ZXIgPSBUZXN0TGF5ZXJDaHJvbWl1bTo6Y3JlYXRlKCk7CkBAIC0yNzcsNCAr Mjg3LDE3IEBAIFRFU1QoQ0NMYXllckl0ZXJhdG9yVGVzdCwgY29tcGxleFRyZWVNdWx0aVN1cmZh Y2UpCiAgICAgRVhQRUNUX0NPVU5UKHJvb3QzLCAtMSwgLTEsIDApOwogfQogCitURVNUKENDTGF5 ZXJJdGVyYXRvclRlc3QsIHJvb3RMYXllcldpdGhvdXRSZW5kZXJTdXJmYWNlKQoreworICAgIFJl ZlB0cjxUZXN0TGF5ZXJDaHJvbWl1bT4gcm9vdExheWVyID0gVGVzdExheWVyQ2hyb21pdW06OmNy ZWF0ZSgpOworICAgIFZlY3RvcjxSZWZQdHI8TGF5ZXJDaHJvbWl1bT4gPiByZW5kZXJTdXJmYWNl TGF5ZXJMaXN0OworICAgIHJlbmRlclN1cmZhY2VMYXllckxpc3QuYXBwZW5kKHJvb3RMYXllci5n ZXQoKSk7CisKKyAgICBpdGVyYXRlQmFja1RvRnJvbnQoJnJlbmRlclN1cmZhY2VMYXllckxpc3Qp OworICAgIEVYUEVDVF9DT1VOVChyb290TGF5ZXIsIC0xLCAtMSwgLTEpOworCisgICAgaXRlcmF0 ZUZyb250VG9CYWNrKCZyZW5kZXJTdXJmYWNlTGF5ZXJMaXN0KTsKKyAgICBFWFBFQ1RfQ09VTlQo cm9vdExheWVyLCAtMSwgLTEsIC0xKTsKK30KKwogfSAvLyBuYW1lc3BhY2UK