Bug 76239

Created attachment 122599 [details] Patch

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.

I changed my real name of the account. So no problem now. Please help review it.

Attachment 122599 [details] 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.

Comment on attachment 122599 [details] Patch LGTM. Thanks for investigating this.

Created attachment 122699 [details] Patch Only 'tab' -> 'white space' change, Please help submit it. Thanks.

Comment on attachment 122699 [details] 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

Comment on attachment 122699 [details] Patch Clearing flags on attachment: 122699 Committed r105233: <http://trac.webkit.org/changeset/105233>

All reviewed patches have been landed. Closing bug.

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

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

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

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

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

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

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