Bug 80005 – Implement strict testing criterion for callback function definition

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Yanbin]

Form bug 79096 comment2

There is a fail test cases: new webkitPeerConnection("TURNS NONE",{}) at Philippe’s test result.while it pass on V8 javascript engine environment.
I looked into this failure and think it is a bug for V8. Test case should fail explicitly by throwing an exception, otherwise the codeshould not run correctly in any case. According to the WebIDL spec for “Callback types”(http://dev.w3.org/2006/webapi/WebIDL/#es-callback): If V is not a Function object, then throw a TypeError.

[Yanbin]

The issue is V8Peerconnection.cpp

  if (args.Length() <= 1 || !args[1]->IsObject())

should be
  
 if (args.Length() <= 1 || !args[1]->IsFunction())

IsFunction is inherited from IsObject. While, Callback function should be defined as Function strictly instead of Object.

V8Peerconnetion.cpp is generated by Peerconnection.idl.
We should modify idl definition .

[Adam Barth]

Presumably we should modify CodeGeneratorV8.pm, which creates V8Peerconnection.cpp from Peerconnection.idl.  We should also check whether other similar generated (and non-generated) code has this same bug.

[Yanbin]

I also try following JS code.
var mycallBackExample = {
    myFirstFunction : function( param1, param2, callback ) {
        // Do something with param1 and param2.
        if ( arguments.length == 3 ) {
                // Execute callback function.
                // What is the "best" way to do this?
        }
    },
    mySecondFunction : function() {
        myFirstFunction( false, true, function() {
                // When this anonymous function is called, execute it.
        });
    }
};

It also don't throw exception  when we call mycallBackExample.myFirstFunction(false,true,{})

[Yanbin]

I can generate a patch to fix this issue

[Yanbin]

Created attachment 130508 [details]
patch

[Yanbin]

Created attachment 130511 [details]
original test result

original test result used 
$ new-run-webkit-tests --chromium

[Yanbin]

Created attachment 130514 [details]
test result after patch was added

Test result after patch was added.
same command:
$new-run-webkit-tests chromium 

The test result are same as original test result

[Kentaro Hara]

Comment on attachment 130508 [details]
patch

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

The change in CodeGeneratorV8.pm looks OK.

I think CodeGeneratorJS.pm has the same issue. Would you please fix it? Otherwise, argument-types.html will fail in Mac.

> Source/WebCore/ChangeLog:3
> +        V8 javascript engine should throw an exception on this case

More descriptive title please

> Source/WebCore/ChangeLog:7
> +

Please describe what your patch is doing.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You can write "Test: fast/mediastream/argument-types.html"

> LayoutTests/ChangeLog:8
> +        * fast/mediastream/script-tests/argument-types.js:

Aren't there any tests that are affected by this change?

[Yanbin]

Created attachment 130557 [details]
updated patch

Implement strict testing criterion for callback function definition.Callback function should be defined as Function strictly instead of Object. While, IsFunction is inherited from IsObject .Existing code only check callback function should be an object. This patch is implement strict testing criterion for callback function definition.

corresponding test cases has been modified. fast/mediastream/script-tests/argument-types.js



> LayoutTests/ChangeLog:8
> +        * fast/mediastream/script-tests/argument-types.js:

Aren't there any tests that are affected by this change?
--- I only find this test case now. And i have run test cases via "new-run-webkit-tests chromium ". no other test cases was found.

[Kentaro Hara]

Comment on attachment 130557 [details]
updated patch

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

Looks OK.

>> Aren't there any tests that are affected by this change?
>--- I only find this test case now. And i have run test cases via "new-run-webkit-tests chromium ". no other test cases was found.

What's "chromium "? Anyway, the commit queue will tell you if there are any failing tests. If you want to commit it, please change "cq:" to "cq:?".

> Source/WebCore/ChangeLog:7
> +

You can write the comment "Implement strict testing criterion for callback function definition.Callback function should be defined as Function strictly instead of Object. While, IsFunction is inherited from IsObject .Existing code only check callback function should be an object. This patch is implement strict testing criterion for callback function definition." here. Also it might be a good idea to add a link to the Web IDL spec that supports this change.

[Kentaro Hara]

Comment on attachment 130557 [details]
updated patch

(1) Please add the comment to ChangeLog.
(2) Upload the patch again.
(3) Change "cq:" to "cq:?".

Then I can change the "cq:?" to "cq:+", and the commit queue will run.

[Gustavo Noronha (kov)]

Comment on attachment 130557 [details]
updated patch

Attachment 130557 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11836806

[Kentaro Hara]

Comment on attachment 130557 [details]
updated patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1233
> +            push(@andExpression, "(${value}.isNull() || ${value}.isFunction())");

Sorry. There is no method JSValue.isFunction():
http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/JavaScriptCore/runtime/JSValue.h&exact_package=chromium&q=jsvalue&type=cs

Is there any alternative in JavaScriptCore?

[Yanbin]

(In reply to comment #13)
> (From update of attachment 130557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130557&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1233
> > +            push(@andExpression, "(${value}.isNull() || ${value}.isFunction())");
> 
> Sorry. There is no method JSValue.isFunction():
> http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/JavaScriptCore/runtime/JSValue.h&exact_package=chromium&q=jsvalue&type=cs
> 
> Is there any alternative in JavaScriptCore?

I have looked into JSValue.h. There is no suitable expect isObject in JavaScriptCore.

It seem we should keep the original version for CodeGeneratorJS.pm.
I will generate a new patch .

[Yanbin]

Created attachment 130559 [details]
patch

updated patch

[Kentaro Hara]

Comment on attachment 130559 [details]
patch

Sorry for the iterative comments.

(1) You need to update the test result, i.e. fast/mediastream/peerconnection-argument-types-expected.txt. In this case, the result of chromium would be different from the result of other platforms. So you can add the result of chromium to platform/chromium/fast/mediastream/peerconnection-argument-types-expected.txt, and update the result of other platforms in fast/mediastream/peerconnection-argument-types-expected.txt.

(2) Would you please file a bug of JSValue? It would be a good idea to add a comment "// FAIL in JavaScriptCore due to http://<link to the bug>" on the 'test('new webkitPeerConnection("TURNS NONE",{})', true);' line.

[Yanbin]

Created attachment 130565 [details]
update new patch

[Kentaro Hara]

Comment on attachment 130565 [details]
update new patch

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

> LayoutTests/ChangeLog:8
> +        * fast/mediastream/peerconnection-argument-types-expected.txt:

As I mentioned, I guess you need to add the result of chromium to platform/chromium/fast/mediastream/peerconnection-argument-types-expected.txt, and update the result of other platforms (e.g. Mac) in fast/mediastream/peerconnection-argument-types-expected.txt. These two results are different, right?

[Yanbin]

(In reply to comment #16)
> (From update of attachment 130559 [details])
> Sorry for the iterative comments.
> 
> (1) You need to update the test result, i.e. fast/mediastream/peerconnection-argument-types-expected.txt. In this case, the result of chromium would be different from the result of other platforms. So you can add the result of chromium to platform/chromium/fast/mediastream/peerconnection-argument-types-expected.txt, and update the result of other platforms in fast/mediastream/peerconnection-argument-types-expected.txt.
> 
Sorry for forgot to update the expected result file .
new patch has been added :)

> (2) Would you please file a bug of JSValue? It would be a good idea to add a comment "// FAIL in JavaScriptCore due to http://<link to the bug>" on the 'test('new webkitPeerConnection("TURNS NONE",{})', true);' line.

I will report a bug for JSValue after confirm the failure at JavaScriptCore environment.

[Yanbin]

(In reply to comment #18)
> (From update of attachment 130565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130565&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        * fast/mediastream/peerconnection-argument-types-expected.txt:
> 
> As I mentioned, I guess you need to add the result of chromium to platform/chromium/fast/mediastream/peerconnection-argument-types-expected.txt, and update the result of other platforms (e.g. Mac) in fast/mediastream/peerconnection-argument-types-expected.txt. These two results are different, right?

I only find one peerconnection-argument-types-expected.txt
at ./LayoutTest/fast/mediastream/peerconnection-argument-types-expected.txt

for original design:
Philippe’s test result(on GTK environment) ,new webkitPeerConnection("TURNS NONE",{}) will throw exception .
new webkitPeerConnection("TURNS NONE",{}) will not throw exception on V8 environment(chromium).

[Kentaro Hara]

(In reply to comment #20)
> I only find one peerconnection-argument-types-expected.txt
> at ./LayoutTest/fast/mediastream/peerconnection-argument-types-expected.txt

Yes, because CodeGeneratorJS.pm and CodeGeneratorV8.pm have been behaving the same. Your patch is going to change their behaviors and generate two kind of results, one for chromium and the other for other platforms.

[Yanbin]

(In reply to comment #21)
> (In reply to comment #20)
> > I only find one peerconnection-argument-types-expected.txt
> > at ./LayoutTest/fast/mediastream/peerconnection-argument-types-expected.txt
> 
> Yes, because CodeGeneratorJS.pm and CodeGeneratorV8.pm have been behaving the same. Your patch is going to change their behaviors and generate two kind of results, one for chromium and the other for other platforms.

(In reply to comment #21)
> (In reply to comment #20)
> > I only find one peerconnection-argument-types-expected.txt
> > at ./LayoutTest/fast/mediastream/peerconnection-argument-types-expected.txt
> 
> Yes, because CodeGeneratorJS.pm and CodeGeneratorV8.pm have been behaving the same. Your patch is going to change their behaviors and generate two kind of results, one for chromium and the other for other platforms.

(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 130565 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=130565&action=review
> > 
> > > LayoutTests/ChangeLog:8
> > > +        * fast/mediastream/peerconnection-argument-types-expected.txt:
> > 
> > As I mentioned, I guess you need to add the result of chromium to platform/chromium/fast/mediastream/peerconnection-argument-types-expected.txt, and update the result of other platforms (e.g. Mac) in fast/mediastream/peerconnection-argument-types-expected.txt. These two results are different, right?
> 
> I only find one peerconnection-argument-types-expected.txt
> at ./LayoutTest/fast/mediastream/peerconnection-argument-types-expected.txt
> 
> for original design:
> Philippe’s test result(on GTK environment) ,new webkitPeerConnection("TURNS NONE",{}) will throw exception .
> new webkitPeerConnection("TURNS NONE",{}) will not throw exception on V8 environment(chromium).

If this patch check in 
Philippe’s test result(on GTK environment) ,new webkitPeerConnection("TURNS NONE",{}) will throw exception .
> new webkitPeerConnection("TURNS NONE",{}) will also throw exception on V8 environment(chromium).

[Kentaro Hara]

(In reply to comment #22)
> If this patch check in 
> Philippe’s test result(on GTK environment) ,new webkitPeerConnection("TURNS NONE",{}) will throw exception .
> > new webkitPeerConnection("TURNS NONE",{}) will also throw exception on V8 environment(chromium).

Maybe I am confused...

Chromium result:
PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.

Other results:
FAIL new webkitPeerConnection("TURNS NONE",{}) did not throw exception.

Am I missing something?

[Yanbin]

(In reply to comment #23)
> (In reply to comment #22)
> > If this patch check in 
> > Philippe’s test result(on GTK environment) ,new webkitPeerConnection("TURNS NONE",{}) will throw exception .
> > > new webkitPeerConnection("TURNS NONE",{}) will also throw exception on V8 environment(chromium).
> 
> Maybe I am confused...
> 
> Chromium result:
> PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
> 
> Other results:
> FAIL new webkitPeerConnection("TURNS NONE",{}) did not throw exception.
> 
> Am I missing something?

original result:
> Chromium result:
> new webkitPeerConnection("TURN NONE",{})  did not throw exception
> 
> Other results:
> new webkitPeerConnection("TURNS NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
while,old test cases expected result is 
PASS new webkitPeerConnection("TURN NONE",{})  did not throw exception

if this patch in
Chromium result will equal other result:
new webkitPeerConnection("TURNS NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
so test cases also updated. now, test case expected result is
PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.

[Yanbin]

(In reply to comment #23)
> (In reply to comment #22)
> > If this patch check in 
> > Philippe’s test result(on GTK environment) ,new webkitPeerConnection("TURNS NONE",{}) will throw exception .
> > > new webkitPeerConnection("TURNS NONE",{}) will also throw exception on V8 environment(chromium).
> 
> Maybe I am confused...
> 
> Chromium result:
> PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
> 
> Other results:
> FAIL new webkitPeerConnection("TURNS NONE",{}) did not throw exception.
> 
> Am I missing something?

original result:
> Chromium result:
> new webkitPeerConnection("TURN NONE",{})  did not throw exception
> 
> Other results:
> new webkitPeerConnection("TURNS NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
while,old test cases expected result is 
PASS new webkitPeerConnection("TURN NONE",{})  did not throw exception

if this patch in
Chromium result will equal other result:
new webkitPeerConnection("TURNS NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
so test cases also updated. now, test case expected result is
PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.

[Kentaro Hara]

(In reply to comment #25)
> while,old test cases expected result is 
> PASS new webkitPeerConnection("TURN NONE",{})  did not throw exception
> 
> if this patch in
> Chromium result will equal other result:
> new webkitPeerConnection("TURNS NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
> so test cases also updated. now, test case expected result is
> PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.

Why will JavaScriptCore throw exception for "new webkitPeerConnection("TURN NONE",{})" ?

Given that your patch does not change any behavior of JavaScriptCore, JavaScriptCore won't throw exception for "new webkitPeerConnection("TURN NONE",{})". Then the JavaScriptCore result would be "FAIL new webkitPeerConnection("TURN NONE",{})  did not throw exception".

[Yanbin]

(In reply to comment #26)

> Given that your patch does not change any behavior of JavaScriptCore, JavaScriptCore won't throw exception for "new webkitPeerConnection("TURN NONE",{})". Then the JavaScriptCore result would be "FAIL new webkitPeerConnection("TURN NONE",{})  did not throw exception".

The patch only fix V8 issue.
I am also confused...
form https://bugs.webkit.org/show_bug.cgi?id=79096#c2
Philippe's test environment is GTK. It throw exception :  FAIL new webkitPeerConnection("TURNS NONE",{}) should not throw exception. Threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17

[Kentaro Hara]

(In reply to comment #27)
> The patch only fix V8 issue.
> I am also confused...
> form https://bugs.webkit.org/show_bug.cgi?id=79096#c2
> Philippe's test environment is GTK. It throw exception :  FAIL new webkitPeerConnection("TURNS NONE",{}) should not throw exception. Threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17

Sorry for the confusion. Without you patch,

- V8 does not throw exception
- JSC throws exception

With your patch,

- V8 throws exception
- JSC throws excpetion

Is my understanding correct?

If it is correct, my question is why JSC can throw exception correctly despite the fact that JSC is using (not isFunction() but) isObject().

[Yanbin]

> 
> Sorry for the confusion. Without you patch,
> 
> - V8 does not throw exception
> - JSC throws exception
> 
> With your patch,
> 
> - V8 throws exception
> - JSC throws excpetion
> 
> Is my understanding correct?

Yes. May be should confirm JSC throw exception. I am not sure Philippe's test environment is standard webkit GTK port version.
> 
> If it is correct, my question is why JSC can throw exception correctly despite the fact that JSC is using (not isFunction() but) isObject().

I am also confused about this issue.

[Kentaro Hara]

(In reply to comment #29)
> Yes. May be should confirm JSC throw exception. I am not sure Philippe's test environment is standard webkit GTK port version.

I think GTK is using JSC anyway.

> > If it is correct, my question is why JSC can throw exception correctly despite the fact that JSC is using (not isFunction() but) isObject().
> 
> I am also confused about this issue.

OK, now the problem is clear. Let us confirm why JSC can throw exception correctly.

[Yanbin]

(In reply to comment #30)
> (In reply to comment #29)

> 
> OK, now the problem is clear. Let us confirm why JSC can throw exception correctly.

JSC can throw exception correctly.

here is a bug entry 
https://bugs.webkit.org/show_bug.cgi?id=79203

[Kentaro Hara]

(In reply to comment #31)
> (In reply to comment #30)
> JSC can throw exception correctly.

Do you mean that isObject() in JSC behaves equivalent to (not IsObject() but) IsFunction() in V8?

I think that our original question was _why_ JSC can throw exception despite the fact that JSC is using (not isFunction() but) isObject().

[Yanbin]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > JSC can throw exception correctly.
> 
> Do you mean that isObject() in JSC behaves equivalent to (not IsObject() but) IsFunction() in V8?
> 
> I think that our original question was _why_ JSC can throw exception despite the fact that JSC is using (not isFunction() but) isObject().

I also confuse about this issue. maybe this is another issue.
It should be looked into this issue.

[Kentaro Hara]

> Do you mean that isObject() in JSC behaves equivalent to (not IsObject() but) IsFunction() in V8?
> 
> I think that our original question was _why_ JSC can throw exception despite the fact that JSC is using (not isFunction() but) isObject().

CCing JSC folks.

[Kentaro Hara]

(In reply to comment #34)
> > Do you mean that isObject() in JSC behaves equivalent to (not IsObject() but) IsFunction() in V8?
> > 
> > I think that our original question was _why_ JSC can throw exception despite the fact that JSC is using (not isFunction() but) isObject().

I think this is not a serious issue. Let's move things forward. Yanbin is blocked by this bug-fix.

Yanbin, let's commit this patch first. And would you file a bug that describes "JSC's isObject() behaves the same as (not V8's IsObject() but) V8's IsFunction()".

[Kentaro Hara]

Comment on attachment 130565 [details]
update new patch

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

> LayoutTests/fast/mediastream/peerconnection-argument-types-expected.txt:48
> +PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.

Should this be "TURNS NONE" (not "TURN NONE")?

[Yanbin]

(In reply to comment #36)
> (From update of attachment 130565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130565&action=review
> 
> > LayoutTests/fast/mediastream/peerconnection-argument-types-expected.txt:48
> > +PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
> 
> Should this be "TURNS NONE" (not "TURN NONE")?

TURNS or TURN both are OK . according to http://dev.w3.org/2011/webrtc/editor/webrtc.html

[Kentaro Hara]

Comment on attachment 130565 [details]
update new patch

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

>>> LayoutTests/fast/mediastream/peerconnection-argument-types-expected.txt:48
>>> +PASS new webkitPeerConnection("TURN NONE",{}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
>> 
>> Should this be "TURNS NONE" (not "TURN NONE")?
> 
> TURNS or TURN both are OK . according to http://dev.w3.org/2011/webrtc/editor/webrtc.html

OK, but let's keep it consistent between argument-types.js and expected.txt. argument-types.js is using "TURNS NONE" and this expected.txt is using "TURN NONE".

[Yanbin]

Created attachment 131774 [details]
patch

update patch according to Comment #38

[Kentaro Hara]

Comment on attachment 131774 [details]
patch

Looks OK! Thanks for the patch.

[WebKit Review Bot]

Comment on attachment 131774 [details]
patch

Rejecting attachment 131774 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/mediastream/peerconnection-argument-types-expected.txt.rej
patching file LayoutTests/fast/mediastream/script-tests/argument-types.js
Hunk #1 FAILED at 79.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/mediastream/script-tests/argument-types.js.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kentaro Ha..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11948853

[Kentaro Hara]

Maybe you need to rebase your patch with the latest WebKit trunk.

[Yanbin]

Created attachment 131809 [details]
patch

[Kentaro Hara]

Comment on attachment 131809 [details]
patch

OK

[WebKit Review Bot]

Comment on attachment 131809 [details]
patch

Rejecting attachment 131809 [details] from commit-queue.

New failing tests:
fast/dom/MediaStream/argument-types.html
Full output: http://queues.webkit.org/results/11956088

[Gavin Barraclough]

There is no reason to fix this for V8 only - this patch should be changing CodeGeneratorJS.pm too.

I'm not sure if there is a convenient method exposed by JSC to test for functions right now, but it's easy enough to do - you just need to just check:

    value.isObject() && (value.asObject()->inherits(&JSFunction::s_info) || value.asObject()->inherits(&InternalFunction::s_info))

I'd suggest adding an idFunction method in JSValue (you probably will want to implement this in JSValueINlineMethods.h):

bool JSValue::isFunction() const
{
    return value.isObject() && (value.asObject()->inherits(&JSFunction::s_info) || value.asObject()->inherits(&InternalFunction::s_info));
}

and change CodeGeneratorJS.pm to call this.

cheers, G.

[Jeremy Mao]

Created attachment 132979 [details]
Patch

New Update:
1. Add a isFunction() type checking util in JavaScriptCore runtime;
2. Update CodeGeneratorJS.pm to make the callback function do the strict function type checking in the generated code.

Could you please take a look at this when you are free?
Thanks /Jeremy

[Kentaro Hara]

(In reply to comment #47)
> Could you please take a look at this when you are free?

r+ for the V8 side change.

barraclough: Would you please check the JSC side change?

yanbin: BTW, our original question was "why does JSC's isObject() behave the same as V8's IsFunction()?". Changing isObject() to isFunction() looks fine, but it does not solve the original question. Do you have any idea for it now?

[Yanbin]

> yanbin: BTW, our original question was "why does JSC's isObject() behave the same as V8's IsFunction()?". Changing isObject() to isFunction() looks fine, but it does not solve the original question. Do you have any idea for it now?

It seems isObject's implement on JSC is different from the one on V8. Maybe we should look into and modify isObject related code  and implement isFunction following comment#46.
Jeremy's patch can be one temporary solution.

[Jeremy Mao]

(In reply to comment #48)
> (In reply to comment #47)
> > Could you please take a look at this when you are free?
> 
> r+ for the V8 side change.
> 
> barraclough: Would you please check the JSC side change?
> 
> yanbin: BTW, our original question was "why does JSC's isObject() behave the same as V8's IsFunction()?". Changing isObject() to isFunction() looks fine, but it does not solve the original question. Do you have any idea for it now?

I will take some time to look at the implementation of IsFunction() & IsObject() in V8, comments will be later.

[WebKit Review Bot]

Comment on attachment 132979 [details]
Patch

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

New failing tests:
fast/dom/MediaStream/argument-types.html

[Jeremy Mao]

(In reply to comment #48)
> (In reply to comment #47)
> > Could you please take a look at this when you are free?
> 
> r+ for the V8 side change.
> 
> barraclough: Would you please check the JSC side change?
> 
> yanbin: BTW, our original question was "why does JSC's isObject() behave the same as V8's IsFunction()?". Changing isObject() to isFunction() looks fine, but it does not solve the original question. Do you have any idea for it now?

I have debugged this issue for a while, the root cause is JSC takes {} as a NullType, but V8 takes {} as a ObjectType. So it's clearly JSC and V8 have the different meanings for {}, I am not sure who's correct. I will open a bug to track this issue, maybe we can have a discussion with other people.

[Kentaro Hara]

(In reply to comment #52)
> I have debugged this issue for a while, the root cause is JSC takes {} as a NullType, but V8 takes {} as a ObjectType. So it's clearly JSC and V8 have the different meanings for {}, I am not sure who's correct. I will open a bug to track this issue, maybe we can have a discussion with other people.

Thank you very much for the clarification!

Then (1) the patch looks OK (if barraclough says OK for the JSC change), (2) let's fix how {} should be treated in another bug.

[Kentaro Hara]

> New failing tests:
> fast/dom/MediaStream/argument-types.html

Also, would you please fix the layout test failure?

[Jeremy Mao]

(In reply to comment #54)
> > New failing tests:
> > fast/dom/MediaStream/argument-types.html
> 
> Also, would you please fix the layout test failure?

I am not going to fix the layout test issue, since these test failure are caused by another bug, I have upload another patch to fix them.
https://bugs.webkit.org/show_bug.cgi?id=81727

[Kentaro Hara]

(In reply to comment #55)
> (In reply to comment #54)
> > > New failing tests:
> > > fast/dom/MediaStream/argument-types.html
> > 
> > Also, would you please fix the layout test failure?
> 
> I am not going to fix the layout test issue, since these test failure are caused by another bug, I have upload another patch to fix them.
> https://bugs.webkit.org/show_bug.cgi?id=81727

I do not understand what the failure is (Could you paste the error if you have?), but if the failure is caused by bug 81727, why doesn't the patch for bug 81727 include argument-types-expected.html?

[Yanbin]

(In reply to comment #56)
> (In reply to comment #55)
> > (In reply to comment #54)
> > > > New failing tests:
> > > > fast/dom/MediaStream/argument-types.html
> > > 
> > > Also, would you please fix the layout test failure?
> > 
> > I am not going to fix the layout test issue, since these test failure are caused by another bug, I have upload another patch to fix them.
> > https://bugs.webkit.org/show_bug.cgi?id=81727
> 
> I do not understand what the failure is (Could you paste the error if you have?), but if the failure is caused by bug 81727, why doesn't the patch for bug 81727 include argument-types-expected.html?

If the case "test('new webkitDeprecatedPeerConnection("TURNS NONE",{})', true);" don't update before test cases run. It is failed.

[Yanbin]

(In reply to comment #57)
> (In reply to comment #56)
> > (In reply to comment #55)
> > > (In reply to comment #54)
> > > > > New failing tests:
> > > > > fast/dom/MediaStream/argument-types.html
> > > > 
> > > > Also, would you please fix the layout test failure?
> > > 
> > > I am not going to fix the layout test issue, since these test failure are caused by another bug, I have upload another patch to fix them.
> > > https://bugs.webkit.org/show_bug.cgi?id=81727
> > 
> > I do not understand what the failure is (Could you paste the error if you have?), but if the failure is caused by bug 81727, why doesn't the patch for bug 81727 include argument-types-expected.html?
> 
>  failing test of fast/dom/MediaStream/argument-types.html isn't caused by bug 81727

[Kentaro Hara]

(In reply to comment #58)
> >  failing test of fast/dom/MediaStream/argument-types.html isn't caused by bug 81727

I am a bit confused. The bug 81727 is not related to the failure. What you need is just to update your patch in this bug so that argument-types.html passes, right?

BTW, can you reproduce the failure in your local environment? Do you already know the cause of the failure?

[Jeremy Mao]

(In reply to comment #59)
> (In reply to comment #58)
> > >  failing test of fast/dom/MediaStream/argument-types.html isn't caused by bug 81727
> 
> I am a bit confused. The bug 81727 is not related to the failure. What you need is just to update your patch in this bug so that argument-types.html passes, right?
> 
> BTW, can you reproduce the failure in your local environment? Do you already know the cause of the failure?

Sorry for so late response, I just make a mistake, bug 81727 is not related to this failure, I will update my code later.

[Gavin Barraclough]

Comment on attachment 132979 [details]
Patch

I'm afraid I don't think the JSC changes here are correct.  We use a couple of different classes in JSC to represent functions with different characteristics, JSFunction and InternalFunction.  Your isFunction check is only testing for JSFunctions, and not InternalFunctions.  Please see comment #46 for a check that should work.

[Jeremy Mao]

Created attachment 133202 [details]
Patch

[Jeremy Mao]

(In reply to comment #61)
> (From update of attachment 132979 [details])
> I'm afraid I don't think the JSC changes here are correct.  We use a couple of different classes in JSC to represent functions with different characteristics, JSFunction and InternalFunction.  Your isFunction check is only testing for JSFunctions, and not InternalFunctions.  Please see comment #46 for a check that should work.

Hi barraclough, thanks for your feedback, patch is updated. I think a better way to implement the isFunction() is to add a InternalFunctionType(JSFunctionType is already exists) in JSType, and then set each function type inherited from InternalFunction to InternalFunctionType.

BTW, I've tried comments #46, but some wierd errors block me.

[WebKit Review Bot]

Comment on attachment 133202 [details]
Patch

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

New failing tests:
fast/dom/MediaStream/argument-types.html

[Yanbin]

(In reply to comment #64)
> (From update of attachment 133202 [details])
> Attachment 133202 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12071911
> 
> New failing tests:
> fast/dom/MediaStream/argument-types.html

The failures are test cases issue:
following test cases should be modified because of CodeGenerateV8.pm's modification.
LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js
test('navigator.webkitGetUserMedia("video", {})', false);  => test('navigator.webkitGetUserMedia("video", {})', true; 
test('navigator.webkitGetUserMedia("video", emptyFunction, {})', false); => test('navigator.webkitGetUserMedia("video", emptyFunction, {})', true); 

and corrsponding argument-types-expected.txt also should be modified
PASS navigator.webkitGetUserMedia("video", emptyFunction, {}) did not throw exception.
=> PASS navigator.webkitGetUserMedia("video", emptyFunction, {}) threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.
PASS navigator.webkitGetUserMedia("video", {})  did not throw exception.
=> PASS navigator.webkitGetUserMedia("video", {})  threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17.

[Gavin Barraclough]

Comment on attachment 133202 [details]
Patch

We should do this properly.  Splattering knowledge of the set of classes used to implement functions throughout the bindings script is not a good design, this should be properly encapsulated.

[Gavin Barraclough]

(In reply to comment #63)
> 
> BTW, I've tried comments #46, but some wierd errors block me.

I've added a JSValue::isFunction method here:
    https://bugs.webkit.org/show_bug.cgi?id=81935

Hopefully this should help!  I you have problems using this, let me know.

cheers,
G.

[Jeremy Mao]

Created attachment 133421 [details]
Patch for Landing

Hi barraclough, haraken:
Could you kindly take a look at this again when you are free?

Thanks /Jeremy

[Jeremy Mao]

(In reply to comment #67)
> (In reply to comment #63)
> > 
> > BTW, I've tried comments #46, but some wierd errors block me.
> 
> I've added a JSValue::isFunction method here:
>     https://bugs.webkit.org/show_bug.cgi?id=81935
> 
> Hopefully this should help!  I you have problems using this, let me know.
> 
> cheers,
> G.

Thanks for your great work, this patch works good for me. Now JSC and V8 have the same behavior for the callback function type checking.

Thanks
/Jeremy

[Kentaro Hara]

r+ for the V8 part and the test.

[Adam Barth]

Comment on attachment 133421 [details]
Patch for Landing

Looks great!

[WebKit Review Bot]

Comment on attachment 133421 [details]
Patch for Landing

Clearing flags on attachment: 133421

Committed r111825: <http://trac.webkit.org/changeset/111825>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.