Bug 94790 – Derived JS binding code throws a different exception than the normal one for overloaded functions when argument count is not enough

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Xiaobo Wang]

Observed by 2 failed DRT tests if MEDIA_STREAM is enabled.
  fast/files/create-blob-url-crash.html
  fast/files/url-required-arguments.html

The reason is DOMURL::createObjectURL() is a overloaded when MEDIA_STREAM is enabled. A dispatching function is added for overloaded functions, and it doesn't check argument count if failed to dispatch the call to one of the overloaded functions.

Chromium and Gtk face the same issue but they created platform specific expected files for the 2 tests. I think a better solution is to fix the issue in the code generator, so that all platforms can share the same expected files.

Even if we only consider one platform, for example BlackBerry, it's possible we enable or disable MEDIA_STREAM explicitly when building WebKit. If we just change the expected result for the ENABLED scenario, the tests will still fail for the other scenario (DISABLE scenario).

[Xiaobo Wang]

Created attachment 160116 [details]
patch

[Kentaro Hara]

Comment on attachment 160116 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160116&action=review

> Source/WebCore/ChangeLog:8
> +        Throw NotEnoughArguments exception for overloaded functions if actualrgument

Typo: actual argument

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1336
> +    my $leastNumMandatoryParams = 255;

Where does 255 come from?

[Xiaobo Wang]

(In reply to comment #2)
> (From update of attachment 160116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160116&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Throw NotEnoughArguments exception for overloaded functions if actualrgument
> 
> Typo: actual argument
Oops, sorry for the typo.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1336
> > +    my $leastNumMandatoryParams = 255;
> 
> Where does 255 come from?
Just want to initialize variable $leastNumMandatoryParams with a number big enough for argument count, do you think 255 is a reasonable number?

[WebKit Review Bot]

Comment on attachment 160116 [details]
patch

Attachment 160116 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13562633

New failing tests:
fast/files/url-required-arguments.html
fast/files/create-blob-url-crash.html

[WebKit Review Bot]

Created attachment 160118 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Kentaro Hara]

Comment on attachment 160116 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160116&action=review

Why don't we need the same fix in CodeGeneratorV8.pm?

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1336
>>> +    my $leastNumMandatoryParams = 255;
>> 
>> Where does 255 come from?
> 
> Just want to initialize variable $leastNumMandatoryParams with a number big enough for argument count, do you think 255 is a reasonable number?

Nit: OK. I was just curious.

[Xiaobo Wang]

Created attachment 160142 [details]
patch wich V8 update

[Kentaro Hara]

Comment on attachment 160142 [details]
patch wich V8 update

View in context: https://bugs.webkit.org/attachment.cgi?id=160142&action=review

The change looks OK. I think you need to update ./run-binding-tests results. See here:
https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests

> Source/WebCore/ChangeLog:8
> +        Throw NotEnoughArguments exception for overloaded functions if actualrgument

Typo: actual argument

[WebKit Review Bot]

Comment on attachment 160142 [details]
patch wich V8 update

Attachment 160142 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13566665

New failing tests:
storage/indexeddb/transaction-and-objectstore-calls.html
platform/chromium/virtual/gpu/fast/canvas/canvas-overloads-setStrokeColor.html
fast/canvas/canvas-overloads-setShadow.html
storage/indexeddb/transaction-storeNames-required.html
platform/chromium/virtual/gpu/fast/canvas/canvas-overloads-setFillColor.html
storage/indexeddb/objectStore-required-arguments.html
fast/canvas/canvas-overloads-setFillColor.html
fast/canvas/canvas-overloads-setStrokeColor.html
fast/canvas/canvas-overloads-drawImage.html
storage/indexeddb/index-get-key-argument-required.html
platform/chromium/virtual/gpu/fast/canvas/canvas-overloads-setShadow.html
fast/canvas/webgl/texImageTest.html
platform/chromium/virtual/gpu/fast/canvas/canvas-overloads-drawImage.html
platform/chromium/virtual/gpu/fast/canvas/webgl/texImageTest.html

[WebKit Review Bot]

Created attachment 160152 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Xiaobo Wang]

(In reply to comment #8)
> (From update of attachment 160142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160142&action=review
> 
> The change looks OK. I think you need to update ./run-binding-tests results. See here:
> https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests
> 
> > Source/WebCore/ChangeLog:8
> > +        Throw NotEnoughArguments exception for overloaded functions if actualrgument
> 
> Typo: actual argument

Thanks for the reminder. I'll update binding results.

[Xiaobo Wang]

Created attachment 160353 [details]
patch revision 3

Updated failed DRT tests.
Updated JS/V8 binding tests result.

[Kentaro Hara]

Comment on attachment 160353 [details]
patch revision 3

Looks OK

[WebKit Review Bot]

Comment on attachment 160353 [details]
patch revision 3

Clearing flags on attachment: 160353

Committed r126562: <http://trac.webkit.org/changeset/126562>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.