87755 2012-05-29 10:02:56 -0700 Fix scoping bug involving eval, with, and optimized non-locals 2012-05-29 12:09:33 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 1 wingo wingo fpizlo ggaren msaboff oldest_to_newest 636159 0 wingo 2012-05-29 10:02:56 -0700 Bug 76285 introduced a regression in the handling of dynamic scope lookup. Bug 87158 reports the regression, but it is being used for some other purpose. The patch to be attached fixes bug 87158. 636162 1 144577 wingo 2012-05-29 10:08:20 -0700 Created attachment 144577 Patch 636274 2 144577 msaboff 2012-05-29 11:40:32 -0700 Comment on attachment 144577 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144577&action=review This fix seems a little like a band-aid. Shouldn't isDynamicScope() return true for scope chain objects that are dynamic and require runtime resolution? Then the code would break out of the loop at we'd generate the right resolve opcode. My comment to open a new defect was incorrect. I think we should close out this defect as no change. We should land the reviewed patch in https://bugs.webkit.org/show_bug.cgi?id=87158 on trunk. A new defect should be opened or the original refactoring defect should be reopened for your suggested fix or variations. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1173 > + size_t depthOfFirstScopeWithDynamicChecks = 0; Why can't the logic for depth be modified so that it provides the right answer? Shouldn't this variable's name really indicate the depth before the first scope with dynamic checks? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1207 > + ++depthOfFirstScopeWithDynamicChecks; If there are multiple scope chain nodes that require dynamic checks, won't this be set to the depth before the last one? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1220 > if ((flags & ResolveResult::DynamicFlag) && depth) > return ResolveResult::dynamicGlobalResolve(depth, scope); Doesn't this code also need the depth before the first dynamic check? 636306 3 wingo 2012-05-29 12:09:33 -0700 (In reply to comment #2) > (From update of attachment 144577 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144577&action=review > > This fix seems a little like a band-aid. Shouldn't isDynamicScope() return true for scope chain objects that are dynamic and require runtime resolution? Then the code would break out of the loop at we'd generate the right resolve opcode. Michael, re > > My comment to open a new defect was incorrect. I think we should close out this defect as no change. We should land the reviewed patch in https://bugs.webkit.org/show_bug.cgi?id=87158 on trunk. A new defect should be opened or the original refactoring defect should be reopened for your suggested fix or variations. > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1173 > > + size_t depthOfFirstScopeWithDynamicChecks = 0; > > Why can't the logic for depth be modified so that it provides the right answer? > > Shouldn't this variable's name really indicate the depth before the first scope with dynamic checks? > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1207 > > + ++depthOfFirstScopeWithDynamicChecks; > > If there are multiple scope chain nodes that require dynamic checks, won't this be set to the depth before the last one? > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1220 > > if ((flags & ResolveResult::DynamicFlag) && depth) > > return ResolveResult::dynamicGlobalResolve(depth, scope); > > Doesn't this code also need the depth before the first dynamic check? 144577 2012-05-29 10:08:20 -0700 2012-05-29 11:40:32 -0700 Patch bug-87755-20120529190818.patch text/plain 5997 wingo U3VidmVyc2lvbiBSZXZpc2lvbjogMTE4NzQ3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA0 YTUyZjE1NzdmYjFkOTA2ZGZlNTk0N2Q5ZWNjNDI4ZGQwMmU0ODYzLi4wY2FhZDlhN2Y2NWZjMDQ1 NzlhYTZlMjI3YjQ0NDljOGJiYzIxMDY5IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxMi0wNS0yOSAgQW5keSBXaW5nbyAgPHdpbmdvQGlnYWxpYS5jb20+CisK KyAgICAgICAgRml4IHNjb3BpbmcgYnVnIGludm9sdmluZyBldmFsLCB3aXRoLCBhbmQgb3B0aW1p emVkIG5vbi1sb2NhbHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTg3NzU1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBieXRlY29tcGlsZXIvQnl0ZWNvZGVHZW5lcmF0b3IuY3BwOgorICAgICAgICAoSlND OjpCeXRlY29kZUdlbmVyYXRvcjo6cmVzb2x2ZSk6IElmIHdlIGhhdmUgdG8gYmFpbCBvdXQgdG8K KyAgICAgICAgZHluYW1pY1Jlc29sdmUoKSwgb25seSBza2lwIHN0YXRpYyBzY29wZXMgZnJvbSB0 aGUgaGVhZCBvZiB0aGUKKyAgICAgICAgc2NvcGUgY2hhaW4uICBCZWZvcmUsIHdlIHdlcmUgYWxz byBza2lwcGluZyBhY3RpdmF0aW9ucyB3aXRoCisgICAgICAgIGRpcmVjdCBldmFsIGFzIHdlbGws IHdoaWNoIHdhcyBpbmNvcnJlY3QuCisKIDIwMTItMDUtMjggIFNoZXJpZmYgQm90ICA8d2Via2l0 LnJldmlldy5ib3RAZ21haWwuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0 IHIxMTg2NDYuCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYnl0ZWNvbXBpbGVy L0J5dGVjb2RlR2VuZXJhdG9yLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGls ZXIvQnl0ZWNvZGVHZW5lcmF0b3IuY3BwCmluZGV4IGZiMDVlNDhmZjlhNjQxOGE0NjRhOWIxYWMw YzI4M2I4OTQ0NjMyZjIuLmRmNTdkOTY1MjYxNGFmNTI1Nzc1YjE4MGUxMzIwOGI1Y2UyMjFmNjYg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvQnl0ZWNvZGVH ZW5lcmF0b3IuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvQnl0 ZWNvZGVHZW5lcmF0b3IuY3BwCkBAIC0xMTcwLDYgKzExNzAsNyBAQCBSZXNvbHZlUmVzdWx0IEJ5 dGVjb2RlR2VuZXJhdG9yOjpyZXNvbHZlKGNvbnN0IElkZW50aWZpZXImIHByb3BlcnR5KQogICAg IFNjb3BlQ2hhaW5JdGVyYXRvciBpdGVyID0gbV9zY29wZUNoYWluLT5iZWdpbigpOwogICAgIFNj b3BlQ2hhaW5JdGVyYXRvciBlbmQgPSBtX3Njb3BlQ2hhaW4tPmVuZCgpOwogICAgIHNpemVfdCBk ZXB0aCA9IDA7CisgICAgc2l6ZV90IGRlcHRoT2ZGaXJzdFNjb3BlV2l0aER5bmFtaWNDaGVja3Mg PSAwOwogICAgIHVuc2lnbmVkIGZsYWdzID0gMDsKICAgICBmb3IgKDsgaXRlciAhPSBlbmQ7ICsr aXRlciwgKytkZXB0aCkgewogICAgICAgICBKU09iamVjdCogY3VycmVudFNjb3BlID0gaXRlci0+ Z2V0KCk7CkBAIC0xMTk5LDE5ICsxMjAwLDI3IEBAIFJlc29sdmVSZXN1bHQgQnl0ZWNvZGVHZW5l cmF0b3I6OnJlc29sdmUoY29uc3QgSWRlbnRpZmllciYgcHJvcGVydHkpCiAgICAgICAgIGJvb2wg c2NvcGVSZXF1aXJlc0R5bmFtaWNDaGVja3MgPSBmYWxzZTsKICAgICAgICAgaWYgKGN1cnJlbnRW YXJpYWJsZU9iamVjdC0+aXNEeW5hbWljU2NvcGUoc2NvcGVSZXF1aXJlc0R5bmFtaWNDaGVja3Mp KQogICAgICAgICAgICAgYnJlYWs7Ci0gICAgICAgIGlmIChzY29wZVJlcXVpcmVzRHluYW1pY0No ZWNrcykKLSAgICAgICAgICAgIGZsYWdzIHw9IFJlc29sdmVSZXN1bHQ6OkR5bmFtaWNGbGFnOwor ICAgICAgICBpZiAoIShmbGFncyAmIFJlc29sdmVSZXN1bHQ6OkR5bmFtaWNGbGFnKSkgeworICAg ICAgICAgICAgaWYgKHNjb3BlUmVxdWlyZXNEeW5hbWljQ2hlY2tzKQorICAgICAgICAgICAgICAg IGZsYWdzIHw9IFJlc29sdmVSZXN1bHQ6OkR5bmFtaWNGbGFnOworICAgICAgICAgICAgZWxzZQor ICAgICAgICAgICAgICAgICsrZGVwdGhPZkZpcnN0U2NvcGVXaXRoRHluYW1pY0NoZWNrczsKKyAg ICAgICAgfQogICAgIH0KIAogICAgIC8vIENhbid0IGxvY2F0ZSB0aGUgcHJvcGVydHkgYnV0IHdl J3JlIGFibGUgdG8gYXZvaWQgYSBmZXcgbG9va3Vwcy4KICAgICBKU09iamVjdCogc2NvcGUgPSBp dGVyLT5nZXQoKTsKKyAgICAvLyBTdGVwIG92ZXIgdGhlIGZ1bmN0aW9uJ3MgYWN0aXZhdGlvbiwg aWYgaXQgbmVlZHMgb25lLiBBdCB0aGlzIHBvaW50IHdlCisgICAgLy8ga25vdyB0aGVyZSBpcyBu byBkeW5hbWljIHNjb3BlIGluIHRoZSBmdW5jdGlvbiBpdHNlbGYsIHNvIHRoaXMgaXMgc2FmZSB0 bworICAgIC8vIGRvLgogICAgIGRlcHRoICs9IG1fY29kZUJsb2NrLT5uZWVkc0Z1bGxTY29wZUNo YWluKCk7CisgICAgZGVwdGhPZkZpcnN0U2NvcGVXaXRoRHluYW1pY0NoZWNrcyArPSBtX2NvZGVC bG9jay0+bmVlZHNGdWxsU2NvcGVDaGFpbigpOwogICAgIGlmICgrK2l0ZXIgPT0gZW5kKSB7CiAg ICAgICAgIGlmICgoZmxhZ3MgJiBSZXNvbHZlUmVzdWx0OjpEeW5hbWljRmxhZykgJiYgZGVwdGgp CiAgICAgICAgICAgICByZXR1cm4gUmVzb2x2ZVJlc3VsdDo6ZHluYW1pY0dsb2JhbFJlc29sdmUo ZGVwdGgsIHNjb3BlKTsKICAgICAgICAgcmV0dXJuIFJlc29sdmVSZXN1bHQ6Omdsb2JhbFJlc29s dmUoc2NvcGUpOwogICAgIH0KLSAgICByZXR1cm4gUmVzb2x2ZVJlc3VsdDo6ZHluYW1pY1Jlc29s dmUoZGVwdGgpOworICAgIHJldHVybiBSZXNvbHZlUmVzdWx0OjpkeW5hbWljUmVzb2x2ZShkZXB0 aE9mRmlyc3RTY29wZVdpdGhEeW5hbWljQ2hlY2tzKTsKIH0KIAogUmVzb2x2ZVJlc3VsdCBCeXRl Y29kZUdlbmVyYXRvcjo6cmVzb2x2ZUNvbnN0RGVjbChjb25zdCBJZGVudGlmaWVyJiBwcm9wZXJ0 eSkKZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5n ZUxvZwppbmRleCA5MzQ4MTY3ZGRmNmMzOWY2YmZmN2Q2YjViYWMyZWQxZGM5OTVjY2E3Li4yZTcy NDRjMzU3ZWFkZDA1NWU3OTZhNjNmOWY5NzNkYWNmMmQ5ZTMxIDEwMDY0NAotLS0gYS9MYXlvdXRU ZXN0cy9DaGFuZ2VMb2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTItMDUtMjkgIEFuZHkgV2luZ28gIDx3aW5nb0BpZ2FsaWEuY29tPgorCisgICAgICAg IEZpeCBzY29waW5nIGJ1ZyBpbnZvbHZpbmcgZXZhbCwgd2l0aCwgYW5kIG9wdGltaXplZCBub24t bG9jYWxzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04 Nzc1NQorCisgICAgICAgIEFkZGVkIHJlZ3Jlc3Npb24gdGVzdHMgZm9yIGZ1bmN0aW9ucyBpbnNp ZGUgZXZhbCBpbnNpZGUgYSB3aXRoLgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogZmFzdC9qcy9ldmFsLWFuZC13aXRoLWV4cGVjdGVkLnR4dDogQWRk ZWQuCisgICAgICAgICogZmFzdC9qcy9ldmFsLWFuZC13aXRoLmh0bWw6IEFkZGVkLgorICAgICAg ICAqIGZhc3QvanMvc2NyaXB0LXRlc3RzL2V2YWwtYW5kLXdpdGguanM6IEFkZGVkLgorCiAyMDEy LTA1LTI5ICBLZW50IFRhbXVyYSAgPHRrZW50QGNocm9taXVtLm9yZz4KIAogICAgICAgICBbQ2hy b21pdW1dIFVwZGF0ZSB0ZXN0IGV4cGVjdGF0aW9ucwpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMv ZmFzdC9qcy9ldmFsLWFuZC13aXRoLWV4cGVjdGVkLnR4dCBiL0xheW91dFRlc3RzL2Zhc3QvanMv ZXZhbC1hbmQtd2l0aC1leHBlY3RlZC50eHQKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMC4uMDlkZTRmODJlNmU4MDlmNGJj ODFlNTk4ZDU4MmVjMzg3NTZiZTc5NAotLS0gL2Rldi9udWxsCisrKyBiL0xheW91dFRlc3RzL2Zh c3QvanMvZXZhbC1hbmQtd2l0aC1leHBlY3RlZC50eHQKQEAgLTAsMCArMSwxMyBAQAorVGhpcyB0 ZXN0IGNhc2UgY2hlY2tzIHZhcmlhYmxlIHJlc29sdXRpb24gaW4gdGhlIHByZXNlbmNlIG9mIGJv dGggZXZhbCBhbmQgd2l0aC4KKworT24gc3VjY2VzcywgeW91IHdpbGwgc2VlIGEgc2VyaWVzIG9m ICJQQVNTIiBtZXNzYWdlcywgZm9sbG93ZWQgYnkgIlRFU1QgQ09NUExFVEUiLgorCisKK1BBU1Mg aW5zaWRlX2V2YWxfYW5kX3dpdGgoe30pKCJ0cnVlIikgaXMgdHJ1ZQorUEFTUyBpbnNpZGVfZXZh bF9hbmRfd2l0aCh7fSkoImZhbHNlIikgaXMgZmFsc2UKK1BBU1MgaW5zaWRlX2V2YWxfYW5kX3dp dGgoe30pKCJ2YXIgeCA9IDEwOyB4IikgPT0gMTAgaXMgdHJ1ZQorUEFTUyBpbnNpZGVfZXZhbF9h bmRfd2l0aCh7fSkoInZhciB4ID0gMTA7IChmdW5jdGlvbiAoKXtyZXR1cm4geDt9KSIpKCkgPT0g MTAgaXMgdHJ1ZQorUEFTUyBzdWNjZXNzZnVsbHlQYXJzZWQgaXMgdHJ1ZQorCitURVNUIENPTVBM RVRFCisKZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2Zhc3QvanMvZXZhbC1hbmQtd2l0aC5odG1s IGIvTGF5b3V0VGVzdHMvZmFzdC9qcy9ldmFsLWFuZC13aXRoLmh0bWwKbmV3IGZpbGUgbW9kZSAx MDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMC4uZmZh YTI4ZmU3MzY4NjhlN2RlNWYwYTQwNGRhOTU3OWRhMDMxOWZmMwotLS0gL2Rldi9udWxsCisrKyBi L0xheW91dFRlc3RzL2Zhc3QvanMvZXZhbC1hbmQtd2l0aC5odG1sCkBAIC0wLDAgKzEsMTAgQEAK KzwhRE9DVFlQRSBIVE1MIFBVQkxJQyAiLS8vSUVURi8vRFREIEhUTUwvL0VOIj4KKzxodG1sPgor PGhlYWQ+Cis8c2NyaXB0IHNyYz0icmVzb3VyY2VzL2pzLXRlc3QtcHJlLmpzIj48L3NjcmlwdD4K KzwvaGVhZD4KKzxib2R5PgorPHNjcmlwdCBzcmM9InNjcmlwdC10ZXN0cy9ldmFsLWFuZC13aXRo LmpzIj48L3NjcmlwdD4KKzxzY3JpcHQgc3JjPSJyZXNvdXJjZXMvanMtdGVzdC1wb3N0LmpzIj48 L3NjcmlwdD4KKzwvYm9keT4KKzwvaHRtbD4KZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL2Zhc3Qv anMvc2NyaXB0LXRlc3RzL2V2YWwtYW5kLXdpdGguanMgYi9MYXlvdXRUZXN0cy9mYXN0L2pzL3Nj cmlwdC10ZXN0cy9ldmFsLWFuZC13aXRoLmpzCm5ldyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLmI1NjMxMDg3NmE5ODU5ZGJl Y2FiZGIwYjY0YjcxYzExZWZlNWVhZDUKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9m YXN0L2pzL3NjcmlwdC10ZXN0cy9ldmFsLWFuZC13aXRoLmpzCkBAIC0wLDAgKzEsMTggQEAKK2Rl c2NyaXB0aW9uKAorIlRoaXMgdGVzdCBjYXNlIGNoZWNrcyB2YXJpYWJsZSByZXNvbHV0aW9uIGlu IHRoZSBwcmVzZW5jZSBvZiBib3RoIGV2YWwgYW5kIHdpdGguIgorKTsKKworLy8gRGlyZWN0IG5v bi1zdHJpY3QgZXZhbCBpbnNpZGUgYSB3aXRoLgorCitmdW5jdGlvbiBpbnNpZGVfZXZhbF9hbmRf d2l0aChvKQoreworICAgIHdpdGggKG8pCisgICAgeworICAgICAgICByZXR1cm4gZnVuY3Rpb24g KHN0cikgeyByZXR1cm4gZXZhbChzdHIpOyB9CisgICAgfQorfQorCitzaG91bGRCZVRydWUoJ2lu c2lkZV9ldmFsX2FuZF93aXRoKHt9KSgidHJ1ZSIpJykKK3Nob3VsZEJlRmFsc2UoJ2luc2lkZV9l dmFsX2FuZF93aXRoKHt9KSgiZmFsc2UiKScpCitzaG91bGRCZVRydWUoJ2luc2lkZV9ldmFsX2Fu ZF93aXRoKHt9KSgidmFyIHggPSAxMDsgeCIpID09IDEwJykKK3Nob3VsZEJlVHJ1ZSgnaW5zaWRl X2V2YWxfYW5kX3dpdGgoe30pKCJ2YXIgeCA9IDEwOyAoZnVuY3Rpb24gKCl7cmV0dXJuIHg7fSki KSgpID09IDEwJykK