Bug 40372 – CodeGeneratorJS.pm incorrectly increments $paramIndex when a method is declared with [CallWith]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andrei Popescu]

CodeGeneratorJS.pm allows IDL methods to be declared with extended attribute [CallWith=SomeType]. This allows the bindings to insert an additional argument of type SomeType when calling the C++ implementation of the method. This means that the number of arguments passed to the C++ method may be larger than the number of arguments that were passed to the JS method.

For example, given the following IDL snippet:

[CallWith=ScriptExecutionContext] void foo(in DOMString bar);

The corresponding C++ method is

class SomeClass {
...
void foo(ScriptExecutionContext* context, String bar);
}

The extra parameter ('context') is generated by the bindings code.

Right now, the CodeGeneratorJS script uses a single variable to keep track of both number of arguments: the ones passed from JS and the ones that need to be passed to the C++ method. If a method uses [CallWith], this variable is incremented. Next time a JS argument needs to be extracted, the index will be off by 1. This wasn't detected so far because it appears that all the methods that use [CallWith] do not take any arguments.

However, in http://trac.webkit.org/changeset/60776/ a method was added that used both [CallWith] and also takes several other arguments. When adding layout tests I noticed that, on JSC, all the arguments passed to my method were off by 1. On V8, the behavior is correct.

The right solution seems to be use two counters: one for keeping track of the arguments read from the JS side and one for the actual number of arguments passed to the corresponding C++ method.

Patch and layout test coming.

[Andrei Popescu]

Created attachment 58266 [details]
Patch

[Adam Barth]

Comment on attachment 58266 [details]
Patch

Oh man, that's embarrassing.  Thanks for the fix.

[Jeremy Orlow]

Comment on attachment 58266 [details]
Patch

No show stoppers, but a bunch of things I think could be better/cleaner.


LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:25
 +      doCreateOrOpen(function() { return db.createObjectStore('storeName', 'keyPath'); }, createSuccess);
Why evaluate this as a function?  You might as well just pass the IDBResult into it.  That said, I actually don't think that helper function is worth the code obfuscation.

LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:5
 +  function openSuccess()
You should put these functions in some sort of order.  RIght now you jump from bottom to top to second from the bottom to second from the top.  Linear is probably easiest to read, though from the bottom up would work as well.

LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:18
 +      shouldBeEqualToString("store.keyPath", "keyPath");
Put fixmes for the other stuff that needs to be done.

LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:28
 +  function doCreateOrOpen(func, successHandler)
Instead of a 'doCreateOrOpen' we really need to clean up the environment before we start.  For now, just put in a 'FIXME' to clean up before we start and put a debug comment explaining that it'll only work once per instance of WebKit.

We should add a way to enumerate databases to the spec to make this easier.  (I already talked to nikunj about this and convinced him, so it's mostly a matter of doing it.)

WebCore/bindings/scripts/CodeGeneratorJS.pm:1872
 +                      my $argsIndex = 0;
$argsIndex and $paramIndex aren't the most clear names.  Maybe even just $inParamIndex and $outParamIndex would be better.

[Andrei Popescu]

(In reply to comment #3)
> (From update of attachment 58266 [details])
> No show stoppers, but a bunch of things I think could be better/cleaner.
> 
> 
> LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:25
>  +      doCreateOrOpen(function() { return db.createObjectStore('storeName', 'keyPath'); }, createSuccess);
> Why evaluate this as a function?  You might as well just pass the IDBResult into it.  That said, I actually don't think that helper function is worth the code obfuscation.
> 

Done.

> LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:5
>  +  function openSuccess()
> You should put these functions in some sort of order.  RIght now you jump from bottom to top to second from the bottom to second from the top.  Linear is probably easiest to read, though from the bottom up would work as well.
>

Done.
 
> LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:18
>  +      shouldBeEqualToString("store.keyPath", "keyPath");
> Put fixmes for the other stuff that needs to be done.
>

Added.
 
> LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:28
>  +  function doCreateOrOpen(func, successHandler)
> Instead of a 'doCreateOrOpen' we really need to clean up the environment before we start.  For now, just put in a 'FIXME' to clean up before we start and put a debug comment explaining that it'll only work once per instance of WebKit.
>

Added FIXME.
 
> 
> WebCore/bindings/scripts/CodeGeneratorJS.pm:1872
>  +                      my $argsIndex = 0;
> $argsIndex and $paramIndex aren't the most clear names.  Maybe even just $inParamIndex and $outParamIndex would be better.

Well, $paramIndex is not an index for output parameters. It is an index for input parameters, just as argsIndex. However, we already have the convention that "args" / "arguments" denote the parameters passed in from JS (see $argsCount or exec->arguments()). $paramsIndex is only used for the in params to the C++ function, so I think it's all reasonably clear.

 Landing manually.

Thanks,
Andrei

[Andrei Popescu]

Committed r60952: <http://trac.webkit.org/changeset/60952>