Bug 70816

Created attachment 112332 [details] Patch

Comment on attachment 112332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112332&action=review Did you run ./Tool/Scripts/run-bindings-tests ? You might need to update the expected results. > Source/WebCore/page/DOMWindow.idl:218 > - [DoNotCheckDomainSecurity, Custom] void postMessage(in SerializedScriptValue message, in [Optional] Array messagePorts, in DOMString targetOrigin) > + [DoNotCheckDomainSecurity, Custom] void postMessage(in SerializedScriptValue message, in DOMString targetOrigin) > raises(DOMException); > - [DoNotCheckDomainSecurity, Custom] void webkitPostMessage(in SerializedScriptValue message, in [Optional] Array transferList, in DOMString targetOrigin) > + [DoNotCheckDomainSecurity, Custom] void postMessage(in SerializedScriptValue message, in Array messagePorts, in DOMString targetOrigin) > + raises(DOMException); This is being discussed in the working group, but I see that you're not changing behavior. > LayoutTests/fast/canvas/canvas-putImageData-expected.txt:147 > -PASS context.putImageData({}, 0, 0) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17. > +PASS context.putImageData({}, 0, 0) threw exception TypeError: Type error. Interesting. I didn't know these were different. This looks like an OK behavior change.

+levin because this touches the postMessage parameter tangle.

Actually, I asked Alexey about this optional parameter not long ago. Here is what he said "The goal of IDL is to document actual behavior here - it's the middle argument that is optional indeed. Technically, it does not matter what the arguments are, since the function is [Custom] anyway. See also: https://bugs.webkit.org/show_bug.cgi?id=63141 There is a long story behind it that I forgot, something about gradually adding arguments and bringing Window and MessagePort versions of postMessage in sync. And once we implemented the mad behavior HTML5 had at that point, the spec got changed again." I guess https://bugs.webkit.org/show_bug.cgi?id=63141#c11 says that optional shouldn't be there, so this is following that advice.

That's fine, but we have a long-term goal of converging WebKitIDL to WebIDL and internal optional parameters aren't allowed in WebIDL. The WebIDL way to express this behavior is using overloading, which is what this patch does.

(In reply to comment #5) > That's fine, but we have a long-term goal of converging WebKitIDL to WebIDL and internal optional parameters aren't allowed in WebIDL. The WebIDL way to express this behavior is using overloading, which is what this patch does. Thanks!

Comment on attachment 112332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112332&action=review Specific consequences of this change look like they are both for the worse. I don't feel very strong about this, and postMessage specifically seems to be in need of change, but this needs to be more clearly an improvement. Specifically, postMessage documentation changes should go together actual implementation changes (in a dedicated patch). >> Source/WebCore/page/DOMWindow.idl:218 >> + raises(DOMException); > > This is being discussed in the working group, but I see that you're not changing behavior. But is this change an improvement? [Optional] is a very useful bit of documentation here. Documentation is why there are arguments specified for [Custom] functions - they are ignored otherwise. What this change does is making documentation not match reality. >> LayoutTests/fast/canvas/canvas-putImageData-expected.txt:147 >> +PASS context.putImageData({}, 0, 0) threw exception TypeError: Type error. > > Interesting. I didn't know these were different. This looks like an OK behavior change. Is it specified what should happen? This looks like a change for worse - incorrect arguments for a built-in function should raise a DOM exception, not a JS one.

> I don't feel very strong about this, and postMessage specifically seems to be in need of change, but this needs to be more clearly an improvement. Specifically, postMessage documentation changes should go together actual implementation changes (in a dedicated patch). Respectfully, I disagree. This patch doesn't change behavior. It just better aligns the syntax of our IDL files with WebIDL. We can consider changing behavior in a future patch. There's no reason to block syntax convergence with WebIDL on behavioral changes. > Is it specified what should happen? This looks like a change for worse - incorrect arguments for a built-in function should raise a DOM exception, not a JS one. That's not how we handle this situation throughout the engine.

Comment on attachment 112332 [details] Patch Thanks for your feedback Alexey, but, as far as I can tell, you aren't an active contributor to this area of the code anymore and your comments are based on an outdated understanding of how this code works and the direction we'd like this code to take.

To be clear, I certainly support getting rid of optional arguments in the middle. I also think that changing postMessage to match spec is good, subject to compatibility testing. > Respectfully, I disagree. This patch doesn't change behavior. The issue that I have with this patch is that it changes documentation (IDL) to not match implementation. That's clearly a step in the wrong direction, and the fact that the patch doesn't change behavior is actually the problem. The whole reason to have argument lists in IDL for [Custom] functions is for documentation. Is the change for DOMWindow.idl because CodeGenerator parses argument lists even for [Custom] functions? In that case, an expedient way to resolve this could be to stop parsing them. > That's not how we handle this situation throughout the engine. Could you explain this in a little more detail? We throw TYPE_MISMATCH_ERR a lot, both from generated bindings code and from DOM code. What are the situations similar to this one where we throw a JS exception?

Simplifying the diff a bit, here's the relevant portion of the change: - void postMessage(in SerializedScriptValue message, in [Optional] Array messagePorts, in DOMString targetOrigin) + void postMessage(in SerializedScriptValue message, in DOMString targetOrigin) + void postMessage(in SerializedScriptValue message, in Array messagePorts, in DOMString targetOrigin) Aside from the fact that the first declaration is technically meaningless, the two forms of declaring postMessage are equivalent. The only difference is that that the new syntax is closer to valid WebIDL. > > That's not how we handle this situation throughout the engine. > > Could you explain this in a little more detail? We throw TYPE_MISMATCH_ERR a lot, both from generated bindings code and from DOM code. What are the situations similar to this one where we throw a JS exception? Conceptually, it depends on which layer detects the error. In the course of calling a DOM API, at some point you leave the "JavaScript" world and enter in the "DOM" world. The JavaScript world is responsible for figuring out which API you're calling and for converting the arguments from the JavaScript world to the DOM world (e.g., calling toString on DOMString arguments). If the error is detected before we leave the JavaScript world, we throw JavaScript errors (conceptually because this code is JavaScript code, not DOM). If the error is detected after we enter the DOM world, we through DOM errors. If you read the WebIDL specs, this is related to how the spec is factored between language neutral processing of WebIDL and JavaScript-specific processing.

> - void postMessage(in SerializedScriptValue message, in [Optional] Array messagePorts, in DOMString targetOrigin) > + void postMessage(in SerializedScriptValue message, in DOMString targetOrigin) > + void postMessage(in SerializedScriptValue message, in Array messagePorts, in DOMString targetOrigin) I missed that there are two overrides added, as they were interspersed with webkitPostMessage. I don't have any objection against this part any more. > > > That's not how we handle this situation throughout the engine. > > > > Could you explain this in a little more detail? We throw TYPE_MISMATCH_ERR a lot, both from generated bindings code and from DOM code. What are the situations similar to this one where we throw a JS exception? > > Conceptually, it depends on which layer detects the error. I know the theory. You claimed that this is how we handle this situation throughout the engine. Is it difficult to give an example that's similar to this case?

Comment on attachment 112332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112332&action=review >>> LayoutTests/fast/canvas/canvas-putImageData-expected.txt:147 >>> +PASS context.putImageData({}, 0, 0) threw exception TypeError: Type error. >> >> Interesting. I didn't know these were different. This looks like an OK behavior change. > > Is it specified what should happen? This looks like a change for worse - incorrect arguments for a built-in function should raise a DOM exception, not a JS one. You can run this command to see a number of modern examples: LayoutTests/fast/canvas/webgl$ grep -r "threw exception TypeError" .

If you're going to be poking at the postMessage() part of the codebase, please just fix the order of the arguments to match the spec... the spec was changed specifically to avoid having optional arguments in the middle of the argument list. See bug 63141.

Yes, that's what I mentioned earlier. This patch doesn't actually change any behavior. It just changes the syntax we use to express the current behavior. I suspect we should change our behavior, but that's a question for another bug.

Adam, seeing dozens of unrelated TypeError exceptions certainly opens my eyes as to why a patch that removes [Optional] from fourth argument of putImageData results in different exception being raised when {} is passed as its first argument. I'm puzzled that you have such attitude mere weeks after asking me about some basic facts of how bindings work on IRC.

Committed r98379: <http://trac.webkit.org/changeset/98379>

> I'm puzzled that you have such attitude mere weeks after asking me about some basic facts of how bindings work on IRC. I'm not sure what you're trying to imply with this statement.

The test added by this patch is failing on SL, GTK, & Qt: http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r98427%20(2715)/fast/canvas/canvas-putImageData-pretty-diff.html It's probably a V8/JSC difference.

> It's probably a V8/JSC difference. Thanks. Sounds like we need to create new baselines.

(In reply to comment #20) > > It's probably a V8/JSC difference. > > Thanks. Sounds like we need to create new baselines. I don't think if "FAIL" is correct ... It sounds like a bug. Reopen the bug to fix it. --- /ramdisk/qt-linux-release/build/layout-test-results/fast/canvas/canvas-putImageData-expected.txt +++ /ramdisk/qt-linux-release/build/layout-test-results/fast/canvas/canvas-putImageData-actual.txt @@ -144,7 +144,7 @@ PASS getPixel(9,9) is [0,128,0,255] PASS getPixel(1,1) is [0,128,0,255] PASS getPixel(9,9) is [0,128,0,255] -PASS context.putImageData({}, 0, 0) threw exception TypeError: Type error. +FAIL context.putImageData({}, 0, 0) should throw TypeError: Type error. Threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17. PASS context.putImageData(buffer, NaN, 0, 0, 0, 0, 0) threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9. PASS context.putImageData(buffer, 0, NaN, 0, 0, 0, 0) threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9. PASS context.putImageData(buffer, 0, 0, NaN, 0, 0, 0) threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9.

@Pavel: I've rolled out your patch. It looks like we'll need to iterate some more to avoid breaking tests on ports that use JavaScriptCore.

Created attachment 112481 [details] Patch

Overloaded methods throw different exceptions in V8 and JSC. Updated the test to call "context.putImageData(null, 0, 0)" just like canvas-createPattern and canvas-createImageData tests do (those won't pass with {} as well).

Comment on attachment 112481 [details] Patch This looks like we're losing test coverage. Can we make the two bindings throw the same exception?

Comment on attachment 112481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112481&action=review > LayoutTests/ChangeLog:11 > + Optional parameters in the middle are prohibited by WebIDL spec. > + > + * fast/canvas/canvas-putImageData-expected.txt: > + * fast/canvas/canvas-putImageData.js: What is the change to this test? Why change what’s being tested, rather than just adding a new test case?

Created attachment 112562 [details] Patch

Generated overloaded methods in V8 throw Type Error. putImageData has to be custom to throw DOM Error.

(In reply to comment #26) > (From update of attachment 112481 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112481&action=review > > > LayoutTests/ChangeLog:11 > > + Optional parameters in the middle are prohibited by WebIDL spec. > > + > > + * fast/canvas/canvas-putImageData-expected.txt: > > + * fast/canvas/canvas-putImageData.js: > > What is the change to this test? Why change what’s being tested, rather than just adding a new test case? putImageData was the only canvas method that throws DOM Exception when called with "{}" as first argument. All other methods throw Type Error in exact same situation. New patch preserves existing behavior of putImageData.

Comment on attachment 112562 [details] Patch We shouldn't add a custom putImageDataCallback just to throw a different exception. We should fix the code generators to both throw the same exception. (Either exception is fine, although I'd prefer the JS exception to the DOM exception, as discussed above.)

Created attachment 112659 [details] Patch

(In reply to comment #30) > (From update of attachment 112562 [details]) > We shouldn't add a custom putImageDataCallback just to throw a different exception. We should fix the code generators to both throw the same exception. (Either exception is fine, although I'd prefer the JS exception to the DOM exception, as discussed above.) You are right, context.putImageData({}, 0, 0) should throw JS exception. I was tricked by wrong test expectation. It threw DOM Exception due to wrong custom implementation in JSC and due to a bug in V8 generated version.

Comment on attachment 112659 [details] Patch Yay for deleting buggy custom bindings. Thanks.

Comment on attachment 112659 [details] Patch Clearing flags on attachment: 112659 Committed r98617: <http://trac.webkit.org/changeset/98617>

All reviewed patches have been landed. Closing bug.

This patch may have caused canvas/philip/tests/2d.imageData.put.wrongtype.html and canvas/philip/tests/2d.missingargs.html to fail on Snow Leopard bots: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98676%20(34249)/results.html http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=canvas%2Fphilip%2Ftests%2F2d.missingargs.html%2Ccanvas%2Fphilip%2Ftests%2F2d.imageData.put.wrongtype.html&group=%40ToT%20-%20webkit.org

(In reply to comment #36) > This patch may have caused canvas/philip/tests/2d.imageData.put.wrongtype.html and canvas/philip/tests/2d.missingargs.html to fail on Snow Leopard bots: > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98676%20(34249)/results.html > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=canvas%2Fphilip%2Ftests%2F2d.missingargs.html%2Ccanvas%2Fphilip%2Ftests%2F2d.imageData.put.wrongtype.html&group=%40ToT%20-%20webkit.org These tests need new expectations, see https://bugs.webkit.org/show_bug.cgi?id=71104.