STEPS TO REPRODUCE: Load this testcase: try { postMessage(5, "*", null); alert("FAIL: null is not a valid value for optional sequence"); } catch (e) { alert("PASS: " + e); } EXPECTED RESULT: An alert saying "PASS" followed by an exception that indicates that the null argument is not a valid third argument for postMessage on Window. ACTUAL RESULT: An alert saying "FAIL". ADDITIONAL INFORMATION: The IDL in the spec for Window is: void postMessage(any message, DOMString targetOrigin, optional sequence<Transferable> transfer); Per http://heycam.github.io/webidl/#es-sequence only objects can represent a sequence. The optional annotation means that undefined is also allowed. But null is not undefined and not an object, so can't be converted to an argument of type "optional sequence<Transferable>".
Probably fixed now?
Created attachment 295633 [details] Test Case
(In reply to comment #1) > Probably fixed now? Nope.
Created attachment 295975 [details] Patch
Attachment 295975 [details] did not pass style-queue: ERROR: Source/WebCore/page/DOMWindow.h:42: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ChangeLog:15: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 2 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 295981 [details] Patch
Created attachment 295989 [details] Patch
Created attachment 295991 [details] Patch
Created attachment 296002 [details] Patch
Created attachment 296009 [details] Patch
Created attachment 296046 [details] Patch
Comment on attachment 296046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296046&action=review > Source/WebCore/bindings/generic/IDLTypes.h:38 > +template <typename T> class Strong; I would omit the space before the "<" and also omit the identifier "T", which is not needed. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2692 > + Vector<RefPtr<MessagePort>> dummyMessagePorts; > + Vector<RefPtr<JSC::ArrayBuffer>> dummyArrayBuffers; Too bad we can’t just use { } for these arguments. Or maybe we can? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2693 > + auto code = CloneSerializer::serialize(&exec, value, dummyMessagePorts, dummyArrayBuffers, blobURLs, buffer); Really seems like this thing should be changed to have a more complex return type in the future instead of it having a return code plus two out arguments. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2710 > + return Exception { StackOverflowError }; I think we can omit the identifier Exception from all these return statements. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2721 > + default: > + ASSERT_NOT_REACHED(); > + return Exception { TypeError }; I suggest we do that thing where we leave out default, and include all the cases in the switch so we get a warning if we add a serialization return code but don’t add anything to this function. > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2729 > + if (RefPtr<ArrayBuffer> arrayBuffer = toPossiblySharedArrayBuffer(transferable.get())) { auto? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2735 > + if (RefPtr<MessagePort> port = JSMessagePort::toWrapped(transferable.get())) { auto? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2863 > + return (code == SerializationReturnCode::SuccessfullyCompleted); I would remove these parentheses. > Source/WebCore/bindings/js/SerializedScriptValue.h:101 > - typedef Vector<JSC::ArrayBufferContents> ArrayBufferContentsArray; > static void maybeThrowExceptionIfSerializationFailed(JSC::ExecState*, SerializationReturnCode); > static bool serializationDidCompleteSuccessfully(SerializationReturnCode); > - static std::unique_ptr<ArrayBufferContentsArray> transferArrayBuffers(JSC::ExecState*, Vector<RefPtr<JSC::ArrayBuffer>>&, SerializationReturnCode&); > + static Exception exceptionForSerializationFailure(SerializationReturnCode); > + > + static ExceptionOr<std::unique_ptr<ArrayBufferContentsArray>> transferArrayBuffers(const Vector<RefPtr<JSC::ArrayBuffer>>&); Seems like these could be non-member functions and not be declared in the header. > Source/WebCore/dom/ExceptionCode.h:79 > + // Used to indicate to the bindings that a JS exception was thrown below and it should be propogated. > + ExistingExceptionError, Seems a bit inelegant. I went out of my way to disallow 0 in Exception, and now we have something that is a similar semantic to ExceptionCode 0. Is there no way to sidestep the need for this? I would have expected that the function would act like there was no exception, and then the caller would notice that an exception was set in the ExecState. > Source/WebCore/dom/MessagePort.idl:38 > + // event handlers I don’t think this comment adds much. What do you think? > Source/WebCore/page/DOMWindow.h:52 > +template <typename T> class Strong; Same comment as above. No need for the space before "<" or for the "T". > LayoutTests/fast/events/message-port-multi-expected.txt:9 > +PASS channel.port1.postMessage("notAPort", [channel3.port1, {}, channel3.port2]) threw exception DataCloneError (DOM Exception 25): The object can not be cloned.. Strange use of "can not" in the DataCloneError string; not introduced by this patch but the first time I noticed it. I think the word is "cannot".
(In reply to comment #12) > Comment on attachment 296046 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296046&action=review > > > Source/WebCore/bindings/generic/IDLTypes.h:38 > > +template <typename T> class Strong; > > I would omit the space before the "<" and also omit the identifier "T", > which is not needed. Done. > > > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2692 > > + Vector<RefPtr<MessagePort>> dummyMessagePorts; > > + Vector<RefPtr<JSC::ArrayBuffer>> dummyArrayBuffers; > > Too bad we can’t just use { } for these arguments. Or maybe we can? We can't. I will be modernizing SerializedScriptValue to avoid these parameters. > > > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2693 > > + auto code = CloneSerializer::serialize(&exec, value, dummyMessagePorts, dummyArrayBuffers, blobURLs, buffer); > > Really seems like this thing should be changed to have a more complex return > type in the future instead of it having a return code plus two out arguments. > Yes. > > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2710 > > + return Exception { StackOverflowError }; > > I think we can omit the identifier Exception from all these return > statements. The Exception constructor is explicit, so they are required. > > > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2721 > > + default: > > + ASSERT_NOT_REACHED(); > > + return Exception { TypeError }; > > I suggest we do that thing where we leave out default, and include all the > cases in the switch so we get a warning if we add a serialization return > code but don’t add anything to this function. > Done. > > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2729 > > + if (RefPtr<ArrayBuffer> arrayBuffer = toPossiblySharedArrayBuffer(transferable.get())) { > > auto? Done. > > > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2735 > > + if (RefPtr<MessagePort> port = JSMessagePort::toWrapped(transferable.get())) { > > auto? Done. > > > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2863 > > + return (code == SerializationReturnCode::SuccessfullyCompleted); > > I would remove these parentheses. I got rid of the function. > > > Source/WebCore/bindings/js/SerializedScriptValue.h:101 > > - typedef Vector<JSC::ArrayBufferContents> ArrayBufferContentsArray; > > static void maybeThrowExceptionIfSerializationFailed(JSC::ExecState*, SerializationReturnCode); > > static bool serializationDidCompleteSuccessfully(SerializationReturnCode); > > - static std::unique_ptr<ArrayBufferContentsArray> transferArrayBuffers(JSC::ExecState*, Vector<RefPtr<JSC::ArrayBuffer>>&, SerializationReturnCode&); > > + static Exception exceptionForSerializationFailure(SerializationReturnCode); > > + > > + static ExceptionOr<std::unique_ptr<ArrayBufferContentsArray>> transferArrayBuffers(const Vector<RefPtr<JSC::ArrayBuffer>>&); > > Seems like these could be non-member functions and not be declared in the > header. Yup. Done. > > > Source/WebCore/dom/ExceptionCode.h:79 > > + // Used to indicate to the bindings that a JS exception was thrown below and it should be propogated. > > + ExistingExceptionError, > > Seems a bit inelegant. I went out of my way to disallow 0 in Exception, and > now we have something that is a similar semantic to ExceptionCode 0. Is > there no way to sidestep the need for this? I would have expected that the > function would act like there was no exception, and then the caller would > notice that an exception was set in the ExecState. Agreed. Unfortunately, propagateException doesn't currently do this, and I didn't want to make fundamental change like that at the same time, and this seemed like a good short term solution. I'll continue to think about this. > > > Source/WebCore/dom/MessagePort.idl:38 > > + // event handlers > > I don’t think this comment adds much. What do you think? No, it's worthless. Just an artifact of copying from the spec. Removed. > > > Source/WebCore/page/DOMWindow.h:52 > > +template <typename T> class Strong; > > Same comment as above. No need for the space before "<" or for the "T". Done. > > > LayoutTests/fast/events/message-port-multi-expected.txt:9 > > +PASS channel.port1.postMessage("notAPort", [channel3.port1, {}, channel3.port2]) threw exception DataCloneError (DOM Exception 25): The object can not be cloned.. > > Strange use of "can not" in the DataCloneError string; not introduced by > this patch but the first time I noticed it. I think the word is "cannot". Yeah.
Committed r209303: <http://trac.webkit.org/changeset/209303>
Windows build is failing now: c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\bindings\js\serializedscriptvalue.cpp(2735): warning C4715: 'WebCore::exceptionForSerializationFailure': not all control paths return a value [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] EFL build is also failing: ../../Source/WebCore/bindings/js/SerializedScriptValue.cpp:2735:1: error: control reaches end of non-void function [-Werror=return-type]