Bug 76239 - DrawingBuffer is not cleared correctly
Summary: DrawingBuffer is not cleared correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: yongsheng
URL:
Keywords:
Depends on: 53201
Blocks: 76562 76654
  Show dependency treegraph
 
Reported: 2012-01-12 18:35 PST by yongsheng
Modified: 2012-02-21 12:47 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.34 KB, patch)
2012-01-16 00:26 PST, yongsheng
no flags Details | Formatted Diff | Diff
Patch (1.39 KB, patch)
2012-01-16 17:43 PST, yongsheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yongsheng 2012-01-12 18:35:25 PST
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.
Comment 1 yongsheng 2012-01-16 00:26:18 PST
Created attachment 122599 [details]
Patch
Comment 2 yongsheng 2012-01-16 00:36:54 PST
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.
Comment 3 yongsheng 2012-01-16 00:55:53 PST
I changed my real name of the account. So no problem now. Please help review it.
Comment 4 WebKit Review Bot 2012-01-16 08:16:35 PST
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 5 Jeff Timanus 2012-01-16 08:17:32 PST
Comment on attachment 122599 [details]
Patch

LGTM.  Thanks for investigating this.
Comment 6 yongsheng 2012-01-16 17:43:25 PST
Created attachment 122699 [details]
Patch

Only 'tab' -> 'white space' change, Please help submit it. Thanks.
Comment 7 Kenneth Russell 2012-01-17 11:47:59 PST
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 8 WebKit Review Bot 2012-01-17 19:24:53 PST
Comment on attachment 122699 [details]
Patch

Clearing flags on attachment: 122699

Committed r105233: <http://trac.webkit.org/changeset/105233>
Comment 9 WebKit Review Bot 2012-01-17 19:24:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 James Robinson 2012-01-18 11:33:56 PST
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
Comment 11 Kenneth Russell 2012-01-18 12:16:38 PST
(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.
Comment 12 yongsheng 2012-01-18 18:00:37 PST
> (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.
Comment 13 Kenneth Russell 2012-01-18 18:22:55 PST
(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.
Comment 14 yongsheng 2012-01-18 19:00:43 PST
> 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?
Comment 15 Kenneth Russell 2012-01-19 12:19:23 PST
(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.
Comment 16 yongsheng 2012-01-19 17:51:52 PST
> 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.