Bug 166046 – CallWith and ConstructorCallWith with multiple values do not respect order

• Status: NEW
• Component: Bindings

[Daniel Bates]

The IDL extended attributes CallWith and ConstructorCallWith may have more than one value, separated by a '&'. When these attributes have more than one value then the generated code does not respect the order these values were written. So, CallWith=A&B may generate code that passes B and then A or passes A then B depending on the types of A and B. I propose that we standardize on the ordering written in the IDL such that CallWith=A&B always generates code that passes A and then B in that order.

[Daniel Bates]

Created attachment 297489 [details]
Patch and tests

[Daniel Bates]

Created attachment 297490 [details]
Patch and tests

[Darin Adler]

Comment on attachment 297490 [details]
Patch and tests

I’m not sure that matching the order the extended attribute lists things is better than a fixed order. Sam, Chris, do you agree it is better?

[Darin Adler]

If our concern is type mismatches, we might want to pursue a solution where we are guaranteed we get a compile time error rather than changing the rule for what the order is. I think that could be done by making sure we use arguments with specific types, and avoiding vague types like a "nullptr" that is not casted to anything.

Is there some example we already ran into where confusion about order resulted in code that actually did the wrong thing rather than just code that didn’t compile?

[Daniel Bates]

(In reply to comment #4)
> If our concern is type mismatches, we might want to pursue a solution where
> we are guaranteed we get a compile time error rather than changing the rule
> for what the order is. I think that could be done by making sure we use
> arguments with specific types, and avoiding vague types like a "nullptr"
> that is not casted to anything.
> 

We should introduce specific types to catch correctness issues at compile time. This does not help make IDL authoring intuitive (maybe we do not care?). I elaborate further on this below.

> Is there some example we already ran into where confusion about order
> resulted in code that actually did the wrong thing rather than just code
> that didn’t compile?

Briefly grep'ing through the source code we do not appear have run into this confusion, yet. When working on the fix for bug #162075 I ran into this ordering issue after I changed the IDL prototype of replace() in Location.idl, <http://trac.webkit.org/browser/trunk/Source/WebCore/page/Location.idl?rev=210070> from:

[DoNotCheckSecurity, CallWith=ActiveWindow&FirstWindow, ForwardDeclareInHeader] void replace(USVString url);

to:

[DoNotCheckSecurity, CallWith=CallerWindow&FirstWindow, ForwardDeclareInHeader] void replace(USVString url);

Notice that the C++ prototype for Location::replace() is (*):

void replace(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);

The following is the paraphrased generated code when using [CallWith=ActiveWindow&FirstWindow]:

[[
static inline JSC::EncodedJSValue jsLocationInstanceFunctionReplaceCaller(JSC::ExecState* state, JSLocation* castedThis, JSC::ThrowScope& throwScope)
{
    UNUSED_PARAM(state);
    ...
    impl.replace(activeDOMWindow(state), firstDOMWindow(state), WTFMove(url));
    return JSValue::encode(jsUndefined());
}
]]

Using [CallWith=CallerWindow&FirstWindow] we get the following paraphrased generated code:

[[
static inline JSC::EncodedJSValue jsLocationInstanceFunctionReplaceCaller(JSC::ExecState* state, JSLocation* castedThis, JSC::ThrowScope& throwScope)
{
    UNUSED_PARAM(state);
    ...
    impl.replace(firstDOMWindow(state), callerDOMWindow(state), WTFMove(url));
    return JSValue::encode(jsUndefined());
}
]]

Notice that the order of the arguments in the call to impl.replace() changed. Without a corresponding renaming of the arguments in (*) this will cause a correctness issue. Although we can catch such correctness issues at compile-time by passing specific types (and we should do this) it is unintuitive from an IDL authoring perspective that the order of the values for the CallWith and ConstructorCallWith attributes may disagree with the order of the arguments that the bindings code marshals to the invoked C++ function. I mean, it is not possible for a person to know the correct argument order for a C++ function that corresponds to an IDL function with a multi-value CallWith or ConstructorCallWith attribute without compiling the code or reading Source/WebCore/bindings/scripts/CodeGeneratorJS.pm to determine the argument order to use. If the person uses the compile and check method this may result in an extraneous compile to determine the correct argument order.

[Daniel Bates]

Summarizing my remarks in comment #5, I see two issues with respect to multi-value CallWith and ConstructorCallWith attributes:

1. IDL authoring is unintuitive (this bug covers fixing this issue): the order of the values of the CallWith and ConstructorCallWith attributes may disagree with the argument order used in the C++ function.

2. Prevent type mismatch when bindings code calls implementation function: Define specific types for CallWith and ConstructorCallWith arguments to catch type mismatch errors at compile-time. For example, introduce a dedicated ActiveDOMWindow type to represent the active DOM window object instead of passing this object as type DOMWindow.

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 297490 [details]
> Patch and tests
> 
> I’m not sure that matching the order the extended attribute lists things is
> better than a fixed order. Sam, Chris, do you agree it is better?

I like the patch. I do think this is better to have the IDL order match the C++ order and it is less error-prone (especially for the various Window* typed parameters).

Otherwise, the order does not have much meaning, it is merely the order in the bindings generator which does not mean anything to developers editing the IDL.

My only worry with this patch is that we have to review each site where it is used carefully to make sure it does not change behavior. Assuming Daniel already did this, I think the patch is fine as is.

[Darin Adler]

Thanks, Chris!

[Brady Eidson]

Comment on attachment 297490 [details]
Patch and tests

Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form.

If this patch is still important please rebase it and post it for review again.