Bug 80005 - Implement strict testing criterion for callback function definition
Summary: Implement strict testing criterion for callback function definition
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-29 23:04 PST by Yanbin
Modified: 2012-03-22 21:31 PDT (History)
11 users (show)

See Also:


Attachments
patch (4.29 KB, patch)
2012-03-06 18:21 PST, Yanbin
haraken: review-
Details | Formatted Diff | Diff
original test result (47.30 KB, application/octet-stream)
2012-03-06 18:30 PST, Yanbin
no flags Details
test result after patch was added (99.70 KB, application/octet-stream)
2012-03-06 18:32 PST, Yanbin
no flags Details
updated patch (6.85 KB, patch)
2012-03-06 23:51 PST, Yanbin
haraken: review-
gns: commit-queue-
Details | Formatted Diff | Diff
patch (4.87 KB, patch)
2012-03-07 00:36 PST, Yanbin
haraken: review-
haraken: commit-queue-
Details | Formatted Diff | Diff
update new patch (6.05 KB, patch)
2012-03-07 00:59 PST, Yanbin
haraken: review-
haraken: commit-queue-
Details | Formatted Diff | Diff
patch (6.05 KB, patch)
2012-03-13 19:41 PDT, Yanbin
haraken: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch (6.24 KB, patch)
2012-03-14 02:05 PDT, Yanbin
barraclough: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (12.26 KB, patch)
2012-03-20 23:35 PDT, Jeremy Mao
barraclough: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (8.96 KB, patch)
2012-03-22 00:54 PDT, Jeremy Mao
barraclough: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch for Landing (13.31 KB, patch)
2012-03-22 20:17 PDT, Jeremy Mao
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yanbin 2012-02-29 23:04:34 PST
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.
Comment 1 Yanbin 2012-03-04 17:59:31 PST
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 .
Comment 2 Adam Barth 2012-03-04 22:45:43 PST
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.
Comment 3 Yanbin 2012-03-04 22:49:59 PST
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,{})
Comment 4 Yanbin 2012-03-04 22:58:01 PST
I can generate a patch to fix this issue
Comment 5 Yanbin 2012-03-06 18:21:57 PST
Created attachment 130508 [details]
patch
Comment 6 Yanbin 2012-03-06 18:30:08 PST
Created attachment 130511 [details]
original test result

original test result used 
$ new-run-webkit-tests --chromium
Comment 7 Yanbin 2012-03-06 18:32:33 PST
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
Comment 8 Kentaro Hara 2012-03-06 20:17:32 PST
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?
Comment 9 Yanbin 2012-03-06 23:51:55 PST
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.
Comment 10 Kentaro Hara 2012-03-06 23:58:23 PST
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.
Comment 11 Kentaro Hara 2012-03-07 00:03:16 PST
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.
Comment 12 Gustavo Noronha (kov) 2012-03-07 00:05:44 PST
Comment on attachment 130557 [details]
updated patch

Attachment 130557 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11836806
Comment 13 Kentaro Hara 2012-03-07 00:10:43 PST
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?
Comment 14 Yanbin 2012-03-07 00:23:59 PST
(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 .
Comment 15 Yanbin 2012-03-07 00:36:57 PST
Created attachment 130559 [details]
patch

updated patch
Comment 16 Kentaro Hara 2012-03-07 00:51:17 PST
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.
Comment 17 Yanbin 2012-03-07 00:59:38 PST
Created attachment 130565 [details]
update new patch
Comment 18 Kentaro Hara 2012-03-07 01:01:59 PST
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?
Comment 19 Yanbin 2012-03-07 01:12:26 PST
(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.
Comment 20 Yanbin 2012-03-07 01:16:22 PST
(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).
Comment 21 Kentaro Hara 2012-03-07 01:19:26 PST
(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.
Comment 22 Yanbin 2012-03-07 01:25:45 PST
(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).
Comment 23 Kentaro Hara 2012-03-07 01:35:09 PST
(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?
Comment 24 Yanbin 2012-03-07 02:46:01 PST
(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.
Comment 25 Yanbin 2012-03-07 03:15:30 PST
(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.
Comment 26 Kentaro Hara 2012-03-07 04:12:02 PST
(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".
Comment 27 Yanbin 2012-03-07 04:17:54 PST
(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
Comment 28 Kentaro Hara 2012-03-07 04:39:34 PST
(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().
Comment 29 Yanbin 2012-03-07 04:54:42 PST
> 
> 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.
Comment 30 Kentaro Hara 2012-03-07 04:57:26 PST
(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.
Comment 31 Yanbin 2012-03-09 01:08:54 PST
(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
Comment 32 Kentaro Hara 2012-03-09 01:15:08 PST
(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().
Comment 33 Yanbin 2012-03-09 01:17:48 PST
(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.
Comment 34 Kentaro Hara 2012-03-09 01:20:08 PST
> 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.
Comment 35 Kentaro Hara 2012-03-13 19:25:12 PDT
(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()".
Comment 36 Kentaro Hara 2012-03-13 19:25:44 PDT
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")?
Comment 37 Yanbin 2012-03-13 19:34:06 PDT
(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
Comment 38 Kentaro Hara 2012-03-13 19:36:13 PDT
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".
Comment 39 Yanbin 2012-03-13 19:41:08 PDT
Created attachment 131774 [details]
patch

update patch according to Comment #38
Comment 40 Kentaro Hara 2012-03-13 19:42:36 PDT
Comment on attachment 131774 [details]
patch

Looks OK! Thanks for the patch.
Comment 41 WebKit Review Bot 2012-03-13 19:46:52 PDT
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
Comment 42 Kentaro Hara 2012-03-13 19:53:32 PDT
Maybe you need to rebase your patch with the latest WebKit trunk.
Comment 43 Yanbin 2012-03-14 02:05:26 PDT
Created attachment 131809 [details]
patch
Comment 44 Kentaro Hara 2012-03-14 02:07:11 PDT
Comment on attachment 131809 [details]
patch

OK
Comment 45 WebKit Review Bot 2012-03-14 08:08:23 PDT
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
Comment 46 Gavin Barraclough 2012-03-14 13:11:10 PDT
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.
Comment 47 Jeremy Mao 2012-03-20 23:35:46 PDT
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
Comment 48 Kentaro Hara 2012-03-20 23:41:20 PDT
(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?
Comment 49 Yanbin 2012-03-21 00:03:39 PDT
> 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.
Comment 50 Jeremy Mao 2012-03-21 00:11:37 PDT
(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.
Comment 51 WebKit Review Bot 2012-03-21 01:29:25 PDT
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
Comment 52 Jeremy Mao 2012-03-21 02:13:56 PDT
(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.
Comment 53 Kentaro Hara 2012-03-21 02:16:49 PDT
(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.
Comment 54 Kentaro Hara 2012-03-21 02:17:27 PDT
> New failing tests:
> fast/dom/MediaStream/argument-types.html

Also, would you please fix the layout test failure?
Comment 55 Jeremy Mao 2012-03-21 04:28:32 PDT
(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
Comment 56 Kentaro Hara 2012-03-21 04:33:01 PDT
(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?
Comment 57 Yanbin 2012-03-21 06:40:21 PDT
(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.
Comment 58 Yanbin 2012-03-21 06:42:24 PDT
(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
Comment 59 Kentaro Hara 2012-03-21 06:46:26 PDT
(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?
Comment 60 Jeremy Mao 2012-03-21 07:05:11 PDT
(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.
Comment 61 Gavin Barraclough 2012-03-21 17:43:14 PDT
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.
Comment 62 Jeremy Mao 2012-03-22 00:54:02 PDT
Created attachment 133202 [details]
Patch
Comment 63 Jeremy Mao 2012-03-22 01:11:27 PDT
(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.
Comment 64 WebKit Review Bot 2012-03-22 01:56:15 PDT
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
Comment 65 Yanbin 2012-03-22 03:59:41 PDT
(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.
Comment 66 Gavin Barraclough 2012-03-22 11:56:55 PDT
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.
Comment 67 Gavin Barraclough 2012-03-22 11:59:40 PDT
(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.
Comment 68 Jeremy Mao 2012-03-22 20:17:07 PDT
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
Comment 69 Jeremy Mao 2012-03-22 20:20:12 PDT
(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
Comment 70 Kentaro Hara 2012-03-22 21:01:01 PDT
r+ for the V8 part and the test.
Comment 71 Adam Barth 2012-03-22 21:02:02 PDT
Comment on attachment 133421 [details]
Patch for Landing

Looks great!
Comment 72 WebKit Review Bot 2012-03-22 21:31:48 PDT
Comment on attachment 133421 [details]
Patch for Landing

Clearing flags on attachment: 133421

Committed r111825: <http://trac.webkit.org/changeset/111825>
Comment 73 WebKit Review Bot 2012-03-22 21:31:58 PDT
All reviewed patches have been landed.  Closing bug.