Bug 76239 – DrawingBuffer is not cleared correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[yongsheng]

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.

[yongsheng]

Created attachment 122599 [details]
Patch

[yongsheng]

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.

[yongsheng]

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

[WebKit Review Bot]

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.

[Jeff Timanus]

Comment on attachment 122599 [details]
Patch

LGTM.  Thanks for investigating this.

[yongsheng]

Created attachment 122699 [details]
Patch

Only 'tab' -> 'white space' change, Please help submit it. Thanks.

[Kenneth Russell]

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

[WebKit Review Bot]

Comment on attachment 122699 [details]
Patch

Clearing flags on attachment: 122699

Committed r105233: <http://trac.webkit.org/changeset/105233>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

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

[Kenneth Russell]

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

[yongsheng]

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

[Kenneth Russell]

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

[yongsheng]

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

[Kenneth Russell]

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

[yongsheng]

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