Bug 142562 – optional sequence values not handled correctly by binding generator

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Boris Zbarsky]

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>".

[Simon Fraser (smfr)]

Probably fixed now?

[Sam Weinig]

Created attachment 295633 [details]
Test Case

[Sam Weinig]

(In reply to comment #1)
> Probably fixed now?

Nope.

[Sam Weinig]

Created attachment 295975 [details]
Patch

[WebKit Commit Bot]

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.

[Sam Weinig]

Created attachment 295981 [details]
Patch

[Sam Weinig]

Created attachment 295989 [details]
Patch

[Sam Weinig]

Created attachment 295991 [details]
Patch

[Sam Weinig]

Created attachment 296002 [details]
Patch

[Sam Weinig]

Created attachment 296009 [details]
Patch

[Sam Weinig]

Created attachment 296046 [details]
Patch

[Darin Adler]

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".

[Sam Weinig]

(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.

[Sam Weinig]

Committed r209303: <http://trac.webkit.org/changeset/209303>

[Darin Adler]

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]