62318 2011-06-08 13:59:25 -0700 Avoid always binding FBO 0 implicitly when deleting FBO in DrawingBuffer code because it invalidates Ganesh's cache of the current FBO. 2011-06-08 16:25:54 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 1 bsalomon bsalomon jamesr reed senorblanco vangelis webkit.review.bot oldest_to_newest 417485 0 bsalomon 2011-06-08 13:59:25 -0700 Don't bind aframebuffer before deleting it in DrawingBuffer::clear() 417564 1 96487 reed 2011-06-08 14:52:18 -0700 Created attachment 96487 Patch 417572 2 96487 jamesr 2011-06-08 14:57:29 -0700 Comment on attachment 96487 Patch Could you expand on how this leads to a crash? Binding before deleting is weird, but it shouldn't be harmful unless there is a bug somewhere else. 417577 3 bsalomon 2011-06-08 15:02:20 -0700 Deleting the bound FBO implicitly binds FBO 0. This confuses Ganesh which thinks it knows the currently bound FBO. This isn't a fix-all for coordinating GL state changes between DrawingBuffer and Ganesh. But it is a simple low risk change that fixes a M13 blocker. 417578 4 96487 jamesr 2011-06-08 15:04:02 -0700 Comment on attachment 96487 Patch I think you need to do the same thing on line 87. Are you sure that this codepath is never hit with the drawingbuffer's m_fbo already bound? 417581 5 jamesr 2011-06-08 15:06:58 -0700 (In reply to comment #3) > Deleting the bound FBO implicitly binds FBO 0. This confuses Ganesh which thinks it knows the currently bound FBO. > > This isn't a fix-all for coordinating GL state changes between DrawingBuffer and Ganesh. But it is a simple low risk change that fixes a M13 blocker. OK, so it sounds like the real bug here is that DrawingBuffer::clear() changes the currently bound framebuffer, and it shouldn't? Could you update the bug title + ChangeLog text to indicate that's what is going on? It seems pretty fragile to depend on this function not changing the current framebuffer binding... 417588 6 96491 bsalomon 2011-06-08 15:17:15 -0700 Created attachment 96491 Patch 417592 7 bsalomon 2011-06-08 15:22:04 -0700 (In reply to comment #4) >(From update of attachment 96487 [details]) >I think you need to do the same thing on line 87. Done in the latest patch. >Are you sure that this codepath is never hit with the drawingbuffer's m_fbo >already bound? In that case we're actually OK. The Gr object that wraps the DrawingBuffer's FBO is destroyed which causes Gr to invalidate its cache of the currently bound FBO. (In reply to comment #5) > OK, so it sounds like the real bug here is that DrawingBuffer::clear() changes the currently bound framebuffer, and it shouldn't? Could you update the bug title + ChangeLog text to indicate that's what is going on? > Yup, done in the latest patch. > It seems pretty fragile to depend on this function not changing the current framebuffer binding... I agree it is fragile. There will be a more robust fix for M14+. 417610 8 96491 jamesr 2011-06-08 15:35:12 -0700 Comment on attachment 96491 Patch Seems OK 417618 9 bsalomon 2011-06-08 15:40:47 -0700 (In reply to comment #8) > (From update of attachment 96491 [details]) > Seems OK Thanks! I forgot to set commit:? after I uploaded. 417670 10 96491 webkit.review.bot 2011-06-08 16:25:50 -0700 Comment on attachment 96491 Patch Clearing flags on attachment: 96491 Committed r88403: <http://trac.webkit.org/changeset/88403> 417671 11 webkit.review.bot 2011-06-08 16:25:54 -0700 All reviewed patches have been landed. Closing bug. 96487 2011-06-08 14:52:18 -0700 2011-06-08 15:17:09 -0700 Patch bug-62318-20110608175216.patch text/plain 1311 reed SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDg4MzkxKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTggQEAKKzIwMTEtMDYtMDggIE1pa2UgUmVl ZCAgPHJlZWRAZ29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBEb24ndCBiaW5kIGEgZnJhbWVidWZmZXIgYmVmb3JlIGRlbGV0aW5nIGl0 IGluIERyYXdpbmdCdWZmZXI6OmNsZWFyKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTYyMzE4CisKKyAgICAgICAgTm8gbmV3IHRlc3RzLgorICAgICAg ICBDaHJvbWl1bS1vbmx5IGNyYXNoCisgICAgICAgIEZhaWx1cmVzIHRha2VzIG11bHRpcGxlIHNl Y29uZHMgdG8gb2NjdXIsIHJlbHlpbmcgb24gSlMgZ2FyYmFnZS1jb2xsZWN0aW9uIHRvIG9jY3Vy CisgICAgICAgIGh0dHA6Ly93d3cuaG90YmF6b29rYS5jb20vcHJpdmF0ZWpvZS1sYXJnZQorCisg ICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvZ3B1L0RyYXdpbmdCdWZmZXIuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6RHJhd2luZ0J1ZmZlcjo6Y2xlYXIpOgorCiAyMDExLTA2LTA2ICBOYXRlIENo YXBpbiAgPGphcGhldEBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQWRhbSBC YXJ0aC4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2dwdS9EcmF3aW5n QnVmZmVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGlj cy9ncHUvRHJhd2luZ0J1ZmZlci5jcHAJKHJldmlzaW9uIDg4Mzg2KQorKysgU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3B1L0RyYXdpbmdCdWZmZXIuY3BwCSh3b3JraW5nIGNvcHkp CkBAIC04OSw3ICs4OSw2IEBAIHZvaWQgRHJhd2luZ0J1ZmZlcjo6Y2xlYXIoKQogICAgICAgICBt X211bHRpc2FtcGxlRkJPID0gMDsKICAgICB9CiAgICAgICAgIAotICAgIG1fY29udGV4dC0+Ymlu ZEZyYW1lYnVmZmVyKEdyYXBoaWNzQ29udGV4dDNEOjpGUkFNRUJVRkZFUiwgbV9mYm8pOwogICAg IG1fY29udGV4dC0+ZGVsZXRlRnJhbWVidWZmZXIobV9mYm8pOwogICAgIG1fZmJvID0gMDsKIH0K 96491 2011-06-08 15:17:15 -0700 2011-06-08 16:25:50 -0700 Patch bug-62318-20110608181713.patch text/plain 1551 bsalomon SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDg4Mzk1KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDYtMDggIEJyaWFuIFNh bG9tb24gIDxic2Fsb21vbkBnb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIEF2b2lkIGFsd2F5cyBiaW5kaW5nIEZCTyAwIGltcGxpY2l0 bHkgd2hlbiBkZWxldGluZyBGQk8gaW4gRHJhd2luZ0J1ZmZlciBjb2RlIGJlY2F1c2UgaXQgaW52 YWxpZGF0ZXMgR2FuZXNoJ3MgY2FjaGUgb2YgdGhlIGN1cnJlbnQgRkJPLgorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjIzMTgKKworICAgICAgICBDaHJv bWl1bS1vbmx5IGNyYXNoCisgICAgICAgIEZhaWx1cmVzIHRha2VzIG11bHRpcGxlIHNlY29uZHMg dG8gb2NjdXIsIHJlbHlpbmcgb24gSlMgZ2FyYmFnZS1jb2xsZWN0aW9uIHRvIG9jY3VyIAorICAg ICAgICBodHRwOi8vd3d3LmhvdGJhem9va2EuY29tL3ByaXZhdGVqb2UtbGFyZ2UKKworICAgICAg ICAqIHBsYXRmb3JtL2dyYXBoaWNzL2dwdS9EcmF3aW5nQnVmZmVyLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OkRyYXdpbmdCdWZmZXI6OmNsZWFyKToKKwogMjAxMS0wNi0wNiAgTmF0ZSBDaGFwaW4g IDxqYXBoZXRAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0gQmFydGgu CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9ncHUvRHJhd2luZ0J1ZmZl ci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3B1 L0RyYXdpbmdCdWZmZXIuY3BwCShyZXZpc2lvbiA4ODMyNikKKysrIFNvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dyYXBoaWNzL2dwdS9EcmF3aW5nQnVmZmVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAt ODQsMTIgKzg0LDEwIEBAIHZvaWQgRHJhd2luZ0J1ZmZlcjo6Y2xlYXIoKQogICAgIH0KICAgICAK ICAgICBpZiAobV9tdWx0aXNhbXBsZUZCTykgewotICAgICAgICBtX2NvbnRleHQtPmJpbmRGcmFt ZWJ1ZmZlcihHcmFwaGljc0NvbnRleHQzRDo6RlJBTUVCVUZGRVIsIG1fbXVsdGlzYW1wbGVGQk8p OwogICAgICAgICBtX2NvbnRleHQtPmRlbGV0ZUZyYW1lYnVmZmVyKG1fbXVsdGlzYW1wbGVGQk8p OwogICAgICAgICBtX211bHRpc2FtcGxlRkJPID0gMDsKICAgICB9CiAgICAgICAgIAotICAgIG1f Y29udGV4dC0+YmluZEZyYW1lYnVmZmVyKEdyYXBoaWNzQ29udGV4dDNEOjpGUkFNRUJVRkZFUiwg bV9mYm8pOwogICAgIG1fY29udGV4dC0+ZGVsZXRlRnJhbWVidWZmZXIobV9mYm8pOwogICAgIG1f ZmJvID0gMDsKIH0K