Summary: | Implement strict testing criterion for callback function definition | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yanbin <yanbin.zhang> | ||||||||||||||||||||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, barraclough, dglazkov, ggaren, gustavo, haraken, japhet, sam, webkit.review.bot, xan.lopez, yujie.mao | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||||
Attachments: |
|
Description
Yanbin
2012-02-29 23:04:34 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 . 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. 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,{}) I can generate a patch to fix this issue Created attachment 130508 [details]
patch
Created attachment 130511 [details]
original test result
original test result used
$ new-run-webkit-tests --chromium
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 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? 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 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 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 on attachment 130557 [details] updated patch Attachment 130557 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11836806 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? (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 . Created attachment 130559 [details]
patch
updated patch
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.
Created attachment 130565 [details]
update new patch
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? (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. (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). (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 #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). (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? (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. (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. (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". (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 (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(). > > 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. (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. (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 (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(). (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. > 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.
(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 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")? (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 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". Created attachment 131774 [details] patch update patch according to Comment #38 Comment on attachment 131774 [details]
patch
Looks OK! Thanks for the patch.
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 Maybe you need to rebase your patch with the latest WebKit trunk. Created attachment 131809 [details]
patch
Comment on attachment 131809 [details]
patch
OK
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 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. 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
(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: 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. (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 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 (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. (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. > New failing tests:
> fast/dom/MediaStream/argument-types.html
Also, would you please fix the layout test failure?
(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 (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? (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. (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 (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? (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 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. Created attachment 133202 [details]
Patch
(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 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 (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 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.
(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. 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
(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 r+ for the V8 part and the test. Comment on attachment 133421 [details]
Patch for Landing
Looks great!
Comment on attachment 133421 [details] Patch for Landing Clearing flags on attachment: 133421 Committed r111825: <http://trac.webkit.org/changeset/111825> All reviewed patches have been landed. Closing bug. |