Bug 126940

Summary: [WebGL2] Query Objects
Product: WebKit Reporter: Dean Jackson <dino>
Component: WebGLAssignee: James Darpinian <jdarpinian>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, ddkilzer, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, jdarpinian, kbr, kondapallykalyan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 214622    
Bug Blocks: 126404    
Attachments:
Description Flags
Patch
none
review feedback
none
rebase
none
rebaseline layout test expando-loss-2.html
none
mark reviewed none

Description Dean Jackson 2014-01-13 15:42:58 PST
Implement the WebGL 2 Query objects.

/* Query Objects */
  WebGLQuery? createQuery();
  void deleteQuery(WebGLQuery? query);
  [WebGLHandlesContextLoss] GLboolean isQuery(WebGLQuery? query);
  void beginQuery(GLenum target, WebGLQuery? query);
  void endQuery(GLenum target);
  /* TODO: document return type */
  any getQuery(GLenum target, GLenum pname);
  /* TODO: document return type */
  any getQueryParameter(WebGLQuery? query, GLenum pname);
Comment 1 Dean Jackson 2014-01-13 15:43:13 PST
<rdar://problem/15002395>
Comment 2 David Kilzer (:ddkilzer) 2016-09-09 10:52:10 PDT
<rdar://problem/28228150>
Comment 3 David Kilzer (:ddkilzer) 2016-09-09 10:53:14 PDT
<rdar://problem/15002395>
Comment 4 James Darpinian 2020-07-20 17:27:53 PDT
Created attachment 404781 [details]
Patch
Comment 5 Kenneth Russell 2020-07-21 11:29:19 PDT
Comment on attachment 404781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404781&action=review

Looking good overall James - couple of comments.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879
> +    if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query))

Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920
>      if (!addResult.isNewEntry) {

It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by:
 - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN)
 - Attempt to start query B of the same type (leads to INVALID_OPERATION)
 - Attempt to end query A

This will yield INVALID_OPERATION when it shouldn't.

Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests?
Comment 6 Kenneth Russell 2020-07-21 12:01:04 PDT
Comment on attachment 404781 [details]
Patch

Attempting to remove duplicate comments.
Comment 7 James Darpinian 2020-07-21 17:45:19 PDT
Created attachment 404890 [details]
review feedback
Comment 8 James Darpinian 2020-07-21 17:46:23 PDT
Comment on attachment 404781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404781&action=review

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879
>> +    if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query))
> 
> Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests?

Fixed. Looks like maybe there isn't a test for that, or at least it isn't running in the WebKit layout tests.

>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920
>>      if (!addResult.isNewEntry) {
> 
> It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by:
>  - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN)
>  - Attempt to start query B of the same type (leads to INVALID_OPERATION)
>  - Attempt to end query A
> 
> This will yield INVALID_OPERATION when it shouldn't.
> 
> Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests?

There are tests for this. It turns out WTF::HashMap::add does not overwrite.
Comment 9 Kenneth Russell 2020-07-21 17:58:26 PDT
Comment on attachment 404781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404781&action=review

>>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1879
>>> +    if (isContextLostOrPending() || !validateWebGLObject("deleteQuery", query))
>> 
>> Deleting a null query, like other objects, is supposed to be a no-op, so using validateWebGLObject here will generate an incorrect INVALID_VALUE. Is this covered by the WebGL conformance tests?
> 
> Fixed. Looks like maybe there isn't a test for that, or at least it isn't running in the WebKit layout tests.

Possible to quickly add a test to sdk/tests/conformance2/query/query.html ? It looks like this isn't checked at least by that test.

>>> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1920
>>>      if (!addResult.isNewEntry) {
>> 
>> It looks like adding, without first testing whether an entry exists, means that if there is an attempt to incorrectly begin two queries of the same type one after the other, m_activeQueries will overwrite the first (actually active) one with the second one, breaking the tracking state. This can be tested by:
>>  - Attempt to start query A (like TRANSFORM_FEEDBACK_PRIMITIVES_WRITTEN)
>>  - Attempt to start query B of the same type (leads to INVALID_OPERATION)
>>  - Attempt to end query A
>> 
>> This will yield INVALID_OPERATION when it shouldn't.
>> 
>> Could you confirm whether this bug exists and if so make sure it's covered by the upstream WebGL conformance tests?
> 
> There are tests for this. It turns out WTF::HashMap::add does not overwrite.

Ah right, I misread the documentation for HashMap::add. Thanks for checking.
Comment 10 Kenneth Russell 2020-07-21 18:07:16 PDT
Comment on attachment 404890 [details]
review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=404890&action=review

Looks good to me!

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1890
> +    if (isContextLostOrPending() || !query || !query->object() || !validateWebGLObject("isQuery", query))

This will generate INVALID_VALUE if it receives a query from a different context, where Chromium will just return false. Not sure what the better behavior is.
Comment 11 James Darpinian 2020-07-22 17:12:13 PDT
Created attachment 404998 [details]
rebase
Comment 12 James Darpinian 2020-07-22 17:57:36 PDT
Created attachment 405002 [details]
rebaseline layout test expando-loss-2.html
Comment 13 James Darpinian 2020-07-23 17:19:40 PDT
Created attachment 405100 [details]
mark reviewed
Comment 14 EWS 2020-07-23 17:50:33 PDT
Committed r264807: <https://trac.webkit.org/changeset/264807>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405100 [details].