76239 2012-01-12 18:35:25 -0800 DrawingBuffer is not cleared correctly 2012-02-21 12:47:43 -0800 1 1 1 Unclassified WebKit WebGL 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 53201 76562 76654 0 zhuyongsheng2010 yongsheng.zhu cmarrin jamesr kbr mdelaney7 twiz webkit.review.bot yongsheng.zhu zmo oldest_to_newest 535368 0 zhuyongsheng2010 2012-01-12 18:35:25 -0800 In the DrawingBuffer::clear(), when m_size is not empty, it should be set as empty. Because it makes 's_currentResourceUsePixels' is substracted with m_size again when DrawingBuffer is destroyed. The issue is found from chromium. See http://code.google.com/p/chromium/issues/detail?id=104740 The patch is ready. I'll submit it to webkit. 536396 1 122599 yongsheng.zhu 2012-01-16 00:26:18 -0800 Created attachment 122599 Patch 536400 2 yongsheng.zhu 2012-01-16 00:36:54 -0800 sorry, I upload the patch with a wrong account. If you don't think it's appropriate, please delete it and I'll re-upload it. 536411 3 yongsheng.zhu 2012-01-16 00:55:53 -0800 I changed my real name of the account. So no problem now. Please help review it. 536561 4 webkit.review.bot 2012-01-16 08:16:35 -0800 Attachment 122599 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:3: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:4: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 536562 5 122599 twiz 2012-01-16 08:17:32 -0800 Comment on attachment 122599 Patch LGTM. Thanks for investigating this. 536829 6 122699 yongsheng.zhu 2012-01-16 17:43:25 -0800 Created attachment 122699 Patch Only 'tab' -> 'white space' change, Please help submit it. Thanks. 537249 7 122699 kbr 2012-01-17 11:47:59 -0800 Comment on attachment 122699 Patch Looks fine. Let's wait for this to clear the EWS before committing. (It would be best if you marked the bug "cq?".) r=me 537574 8 122699 webkit.review.bot 2012-01-17 19:24:53 -0800 Comment on attachment 122699 Patch Clearing flags on attachment: 122699 Committed r105233: <http://trac.webkit.org/changeset/105233> 537575 9 webkit.review.bot 2012-01-17 19:24:58 -0800 All reviewed patches have been landed. Closing bug. 538030 10 jamesr 2012-01-18 11:33:56 -0800 This patch appears to have broken fast/canvas/webgl/drawingbuffer-test.html: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcanvas%2Fwebgl%2Fdrawingbuffer-test.html 538063 11 kbr 2012-01-18 12:16:38 -0800 (In reply to comment #10) > This patch appears to have broken fast/canvas/webgl/drawingbuffer-test.html: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcanvas%2Fwebgl%2Fdrawingbuffer-test.html yongsheng, did you run the layout tests? Filed https://bugs.webkit.org/show_bug.cgi?id=76562 to track this regression. 538374 12 yongsheng.zhu 2012-01-18 18:00:37 -0800 > (In reply to comment #10) > > This patch appears to have broken fast/canvas/webgl/drawingbuffer-test.html: > > > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcanvas%2Fwebgl%2Fdrawingbuffer-test.html > > yongsheng, did you run the layout tests? > > Filed https://bugs.webkit.org/show_bug.cgi?id=76562 to track this regression. Sorry, I should notify that the fix for this might expose the failure for these kind of cases which don't occur before. This kind of failure is somewhat random. Actually both 2 cases are almost the same. The reason could be see from here: http://code.google.com/p/chromium/issues/detail?id=104740#c5 This case is also to test DrawingBuffer. And the root cause of this case is the same. The failure is because of the V8 GC issue. There are 2 ways: 1) revert the patch and commit it together with the patch for #76225(release GL context). 2) Or don't revert it and it will get fixed with the patch for #76225. what's your opinion? I should raise it clearly. It's my fault. Sorry about that. 538386 13 kbr 2012-01-18 18:22:55 -0800 (In reply to comment #12) > > (In reply to comment #10) > > > This patch appears to have broken fast/canvas/webgl/drawingbuffer-test.html: > > > > > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcanvas%2Fwebgl%2Fdrawingbuffer-test.html > > > > yongsheng, did you run the layout tests? > > > > Filed https://bugs.webkit.org/show_bug.cgi?id=76562 to track this regression. > Sorry, I should notify that the fix for this might expose the failure for these kind of cases which don't occur before. This kind of failure is somewhat random. Actually both 2 cases are almost the same. > The reason could be see from here: http://code.google.com/p/chromium/issues/detail?id=104740#c5 > This case is also to test DrawingBuffer. And the root cause of this case is the same. The failure is because of the V8 GC issue. > > There are 2 ways: > 1) revert the patch and commit it together with the patch for #76225(release GL context). > 2) Or don't revert it and it will get fixed with the patch for #76225. > what's your opinion? > > I should raise it clearly. It's my fault. Sorry about that. I don't see how fixing Bug 76225 will affect the execution of fast/canvas/webgl/drawingbuffer-test.html. There is no JavaScript-level garbage collection involved when the canvas's width and height are changed. Bug 76255 will only release WebGL resources more eagerly when the page is unloaded. Is this analysis correct? If it is, then this patch should definitely be rolled out and rethought. 538399 14 yongsheng.zhu 2012-01-18 19:00:43 -0800 > I don't see how fixing Bug 76225 will affect the execution of fast/canvas/webgl/drawingbuffer-test.html. There is no JavaScript-level garbage collection involved when the canvas's width and height are changed. Bug 76255 will only release WebGL resources more eagerly when the page is unloaded. Does the cases 'drawingbuffer-static-canvas-test' and 'drawingbuffer-test' was ran in one renderer process in the test framework? If yes, it's similar to refresh the page: the previously used drawingbuffer was not released and then the next case have no enough buffer to use. So a gc or resource release is needed. In my own test results, these 2 cases running in one tab in turn are in one renderer process. Could you please help check it in the test server? 538915 15 kbr 2012-01-19 12:19:23 -0800 (In reply to comment #14) > > I don't see how fixing Bug 76225 will affect the execution of fast/canvas/webgl/drawingbuffer-test.html. There is no JavaScript-level garbage collection involved when the canvas's width and height are changed. Bug 76255 will only release WebGL resources more eagerly when the page is unloaded. > > Does the cases 'drawingbuffer-static-canvas-test' and 'drawingbuffer-test' was ran in one renderer process in the test framework? > > If yes, it's similar to refresh the page: the previously used drawingbuffer was not released and then the next case have no enough buffer to use. So a gc or resource release is needed. > > In my own test results, these 2 cases running in one tab in turn are in one renderer process. > Could you please help check it in the test server? Yes, it looks like they run in the same renderer process. I see what is happening -- we are hitting the s_maximumResourceUsePixels limit in the DrawingBuffer code when running almost any other test in the same renderer process as drawingbuffer-test.html. When just running the layout tests, it's sufficient to run just fast/canvas/webgl/canvas-test.html and fast/canvas/webgl/drawingbuffer-test.html in the same invocation of run-webkit-tests to provoke the failure. While fixing Bug 76562 will mask this problem, there is a deeper issue; the WebGL context must retry the allocation of the back buffer at a smaller size if it fails. There is really no provision in the spec for the context's back buffer shrinking to 0x0 in response to resizing of the canvas. I've filed https://bugs.webkit.org/show_bug.cgi?id=76654 to track this issue. 539192 16 yongsheng.zhu 2012-01-19 17:51:52 -0800 > I see what is happening -- we are hitting the s_maximumResourceUsePixels limit in the DrawingBuffer code when running almost any other test in the same renderer process as drawingbuffer-test.html. When just running the layout tests, it's sufficient to run just fast/canvas/webgl/canvas-test.html and fast/canvas/webgl/drawingbuffer-test.html in the same invocation of run-webkit-tests to provoke the failure. Yes, you got it :). That's the root cause. This fix extremely exposes the bug #76225 & #76562. I'm fixing it. Will propose a patch. > While fixing Bug 76562 will mask this problem, there is a deeper issue; the WebGL context must retry the allocation of the back buffer at a smaller size if it fails. There is really no provision in the spec for the context's back buffer shrinking to 0x0 in response to resizing of the canvas. I've filed https://bugs.webkit.org/show_bug.cgi?id=76654 to track this issue. Reasonable. thanks for exposing this issue. 122599 2012-01-16 00:26:18 -0800 2012-01-16 17:43:25 -0800 Patch Patch text/plain 1376 yongsheng.zhu SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwNTAzNikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTAxLTE2ICBZb25nc2hl bmcgWmh1ICA8eW9uZ3NoZW5nLnpodUBpbnRlbC5jb20+CisKKwkJQ2xlYXIgJ21fc2l6ZScgb2Yg RHJhd2luZ0J1ZmZlciBpbiB0aGUgJ2NsZWFyJyBmdW5jdGlvbgorCQlodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzYyMzkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKwkJCisJCUNsZWFyIHRoZSByZXNvdXJjZXMgb2YgRHJhd2luZ0J1ZmZlciBi dXQgZG9uJ3QgY2xlYXIgJ21fc2l6ZScuIFRoaXMgbWFrZXMKKwkJJ3NfY3VycmVudFJlc291cmNl VXNlUGl4ZWxzJyBpcyBub3QgY2FsY3VsYXRlZCBjb3JyZWN0bHkuCisKKyAgICAgICAgKiBwbGF0 Zm9ybS9ncmFwaGljcy9ncHUvRHJhd2luZ0J1ZmZlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpE cmF3aW5nQnVmZmVyOjpjbGVhcik6CisKIDIwMTItMDEtMTUgIFhpbmNoYW8gSGUgIDx4aW5jaGFv LmhlQGludGVsLmNvbT4KIAogICAgICAgICBBZGQgRGV2aWNlT3JpZW50YXRpb25FdmVudC5hYnNv bHV0ZQpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3B1L0RyYXdpbmdC dWZmZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L2dwdS9EcmF3aW5nQnVmZmVyLmNwcAkocmV2aXNpb24gMTA0Nzk0KQorKysgU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3B1L0RyYXdpbmdCdWZmZXIuY3BwCSh3b3JraW5nIGNvcHkp CkBAIC03Miw4ICs3MiwxMCBAQCB2b2lkIERyYXdpbmdCdWZmZXI6OmNsZWFyKCkKICAgICAgICAg cmV0dXJuOwogCiAgICAgbV9jb250ZXh0LT5tYWtlQ29udGV4dEN1cnJlbnQoKTsKLSAgICBpZiAo IW1fc2l6ZS5pc0VtcHR5KCkpCisgICAgaWYgKCFtX3NpemUuaXNFbXB0eSgpKSB7CiAgICAgICAg IHNfY3VycmVudFJlc291cmNlVXNlUGl4ZWxzIC09IG1fc2l6ZS53aWR0aCgpICogbV9zaXplLmhl aWdodCgpOworICAgICAgICBtX3NpemUgPSBJbnRTaXplKCk7CisgICAgfQogCiAgICAgaWYgKG1f Y29sb3JCdWZmZXIpIHsKICAgICAgICAgbV9jb250ZXh0LT5kZWxldGVUZXh0dXJlKG1fY29sb3JC dWZmZXIpOwo= 122699 2012-01-16 17:43:25 -0800 2012-01-17 19:24:53 -0800 Patch Patch text/plain 1428 yongsheng.zhu SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwNTExNikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTAxLTE2ICBZb25nc2hl bmcgWmh1ICA8eW9uZ3NoZW5nLnpodUBpbnRlbC5jb20+CisKKyAgICAgICAgQ2xlYXIgJ21fc2l6 ZScgb2YgRHJhd2luZ0J1ZmZlciBpbiB0aGUgJ2NsZWFyJyBmdW5jdGlvbgorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzYyMzkKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgCisgICAgICAgIENsZWFyIHRoZSByZXNv dXJjZXMgb2YgRHJhd2luZ0J1ZmZlciBidXQgZG9uJ3QgY2xlYXIgJ21fc2l6ZScuIFRoaXMgbWFr ZXMKKyAgICAgICAgJ3NfY3VycmVudFJlc291cmNlVXNlUGl4ZWxzJyBpcyBub3QgY2FsY3VsYXRl ZCBjb3JyZWN0bHkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9ncHUvRHJhd2luZ0J1 ZmZlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpEcmF3aW5nQnVmZmVyOjpjbGVhcik6CisKIDIw MTItMDEtMTYgIEVucmljYSBDYXN1Y2NpICA8ZW5yaWNhQGFwcGxlLmNvbT4KIAogICAgICAgICBS RUdSRVNTSU9OOiByMTAyNTUzIEF1dG9jb3JyZWN0aW9uIGJ1YmJsZSBkb2Vzbid0IHNob3cgdXAK SW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2dwdS9EcmF3aW5nQnVmZmVy LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9ncHUv RHJhd2luZ0J1ZmZlci5jcHAJKHJldmlzaW9uIDEwNTExNikKKysrIFNvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dyYXBoaWNzL2dwdS9EcmF3aW5nQnVmZmVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAt NzIsOCArNzIsMTAgQEAgdm9pZCBEcmF3aW5nQnVmZmVyOjpjbGVhcigpCiAgICAgICAgIHJldHVy bjsKIAogICAgIG1fY29udGV4dC0+bWFrZUNvbnRleHRDdXJyZW50KCk7Ci0gICAgaWYgKCFtX3Np emUuaXNFbXB0eSgpKQorICAgIGlmICghbV9zaXplLmlzRW1wdHkoKSkgewogICAgICAgICBzX2N1 cnJlbnRSZXNvdXJjZVVzZVBpeGVscyAtPSBtX3NpemUud2lkdGgoKSAqIG1fc2l6ZS5oZWlnaHQo KTsKKyAgICAgICAgbV9zaXplID0gSW50U2l6ZSgpOworICAgIH0KIAogICAgIGlmIChtX2NvbG9y QnVmZmVyKSB7CiAgICAgICAgIG1fY29udGV4dC0+ZGVsZXRlVGV4dHVyZShtX2NvbG9yQnVmZmVy KTsK