Summary: | [WebGL2] Query Objects | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||||||||
Component: | WebGL | Assignee: | 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
Dean Jackson
2014-01-13 15:42:58 PST
Created attachment 404781 [details]
Patch
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 on attachment 404781 [details]
Patch
Attempting to remove duplicate comments.
Created attachment 404890 [details]
review feedback
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 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 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. Created attachment 404998 [details]
rebase
Created attachment 405002 [details]
rebaseline layout test expando-loss-2.html
Created attachment 405100 [details]
mark reviewed
Committed r264807: <https://trac.webkit.org/changeset/264807> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405100 [details]. |