Bug 70215

Created attachment 111222 [details] Patch

Ideally Sam or ggaren would comment on the JavaScriptCore aspect of this change.

Created attachment 111545 [details] Rebased patch for review

Comment on attachment 111545 [details] Rebased patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=111545&action=review We should not be special casing an empty JSValue. If you get one of these, something is probably wrong. > Source/JavaScriptCore/runtime/JSString.h:712 > + if (isEmpty()) > + return UString(); This is wrong. The empty value JSValue should be treated like a null pointer, and you should never call functions on it.

Comment on attachment 111545 [details] Rebased patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=111545&action=review >> Source/JavaScriptCore/runtime/JSString.h:712 >> + return UString(); > > This is wrong. The empty value JSValue should be treated like a null pointer, and you should never call functions on it. Thanks, sam! However, toNumber(), toFloat(), toInt32(), toUInt32() and toInteger() return 0 when an empty value is passed to them. Specifically, in case of the empty value, those methods fallback to JSValue::toNumberSlowCase(), which returns 0. toBoolean() returns false when the empty value is passed. Judging from these behaviors, I guess that it would make sense to return the "default value" of the given type when the empty value is passed. If so, returning UString() from toString() would also make sense. Why do you think that the empty value should be treated like a null pointer?

(In reply to comment #5) > (From update of attachment 111545 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111545&action=review > > >> Source/JavaScriptCore/runtime/JSString.h:712 > >> + return UString(); > > > > This is wrong. The empty value JSValue should be treated like a null pointer, and you should never call functions on it. > > Thanks, sam! > > However, toNumber(), toFloat(), toInt32(), toUInt32() and toInteger() return 0 when an empty value is passed to them. Specifically, in case of the empty value, those methods fallback to JSValue::toNumberSlowCase(), which returns 0. toBoolean() returns false when the empty value is passed. Judging from these behaviors, I guess that it would make sense to return the "default value" of the given type when the empty value is passed. If so, returning UString() from toString() would also make sense. > > Why do you think that the empty value should be treated like a null pointer? The empty value is not a valid value to be passed into JavaScript code (it does not have a a meaning within the language), and JS code will not check for this (e.g. passing the empty value to JIT code could lead to a null pointer exception). The correct interpretation of the empty value is as an in-band signal of an error. It is not a valid value to operate on or to pass to functions. I think the numeric conversions should probably not be accepting the empty value, that this is an error. They should likely be able to ASSERT against this usage.

Created attachment 112804 [details] Patch

(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 111545 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=111545&action=review > > > > >> Source/JavaScriptCore/runtime/JSString.h:712 > > >> + return UString(); > > > > > > This is wrong. The empty value JSValue should be treated like a null pointer, and you should never call functions on it. > > > > Thanks, sam! > > > > However, toNumber(), toFloat(), toInt32(), toUInt32() and toInteger() return 0 when an empty value is passed to them. Specifically, in case of the empty value, those methods fallback to JSValue::toNumberSlowCase(), which returns 0. toBoolean() returns false when the empty value is passed. Judging from these behaviors, I guess that it would make sense to return the "default value" of the given type when the empty value is passed. If so, returning UString() from toString() would also make sense. > > > > Why do you think that the empty value should be treated like a null pointer? > > The empty value is not a valid value to be passed into JavaScript code (it does not have a a meaning within the language), and JS code will not check for this (e.g. passing the empty value to JIT code could lead to a null pointer exception). The correct interpretation of the empty value is as an in-band signal of an error. It is not a valid value to operate on or to pass to functions. > > I think the numeric conversions should probably not be accepting the empty value, that this is an error. They should likely be able to ASSERT against this usage. Thank you, I got it. I fixed the patch. c.f. Why do we need to introduce [CallWithNullValue] and MAYBE_MISSING_PARAMETER() macro: https://bugs.webkit.org/show_bug.cgi?id=67458

Comment on attachment 112804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112804&action=review > Source/WebCore/ChangeLog:12 > + This patch implements [Optional=CallWithNullValue] IDL for JSC. > + While a parameter specified as [Optional=CallWithDefaultValue] is > + handled as a string "undefined", a parameter specified as > + [Optional=CallWithNullValue] is handled as a null string. > + (Note: not a string "null", but a null string). Interesting.

Comment on attachment 112804 [details] Patch Clearing flags on attachment: 112804 Committed r98679: <http://trac.webkit.org/changeset/98679>

All reviewed patches have been landed. Closing bug.

How is Optional=CallWithNullValue different from Optional,ConvertNullToNullString?

> How is Optional=CallWithNullValue different from Optional,ConvertNullToNullString? I would expect the two to be different from when the argument null is supplied. In the ConvertNullToNullString case, the null string will be supplied whereas in the Optional=CallWithNullValue, the string "null" will be supplied.

(In reply to comment #12) > How is Optional=CallWithNullValue different from Optional,ConvertNullToNullString? ConvertNullToNullString defines the behavior when null is passed to a parameter. On the other hand, Optional=CallWithNullValue defines the behavior when the parameter is missing. Please assume func(in [XXXX] DOMString str). If XXXX is "Optional, ConvertNullToNullString": func(null) -> (not a string "null" but) a null string is passed to a WebCore method. func() -> a string "undefined" is passed to a WebCore method. If XXXX is "Optional=CallWithNullValue": func(null) -> a string "null" is passed to a WebCore method. func() -> a null string is passed to a WebCore method. If XXXX is "Optional=CallWithDefaultValue": func(null) -> a string "null" is passed to a WebCore method. func() -> a string "undefined" is passed to a WebCore method.

binding tests are failing :( http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/25283

Rebaselined binding tests in http://trac.webkit.org/changeset/98688. We should really do something about these binding tests. It's shadowing other more serious failures by turning bots red :(

(In reply to comment #16) > We should really do something about these binding tests. Yes, I think we should make sure at least one EWS server runs them.