120537 2013-08-30 10:04:58 -0700 Wrong for SlowPathCall to load callFrame reg from vm.topCallFrame after call 2013-09-09 16:02:22 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 msaboff msaboff oldest_to_newest 923039 0 msaboff 2013-08-30 10:04:58 -0700 After making a slow path call in the exception path, we load the call frame register from vm.topCallFrame. This isn't needed and wrong. The callFrame register is a callee save and therefore its contents will be preserved across the slow path call. Also, there are no guarantees that vm.topCallFrame is still valid. Given these two facts, we actually need to be storing the callFrameRegister TO vm.topCallFrame. 923043 1 210127 msaboff 2013-08-30 10:09:32 -0700 Created attachment 210127 Patch 923091 2 210127 mark.lam 2013-08-30 11:14:51 -0700 Comment on attachment 210127 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210127&action=review > Source/JavaScriptCore/jit/SlowPathCall.h:91 > - m_jit->reloadCallFrameFromTopCallFrame(); > + m_jit->updateTopCallFrame(); This looks strange to me. At this point, given that we have an exception, the caller may have unwound the stack already. We should not assume that the current stack frame is even valid. Have you tested against the tests in https://bugs.webkit.org/show_bug.cgi?id=119848 yet? 924652 3 210127 ggaren 2013-09-04 15:44:57 -0700 Comment on attachment 210127 Patch Michael, what's the right solution here? 924657 4 msaboff 2013-09-04 16:03:33 -0700 (In reply to comment #3) > (From update of attachment 210127 [details]) > Michael, what's the right solution here? I need to run the tests that Mark suggests to verify this change. From what I've seen, both locations have the same (correct) value. It seem to make more sense that we update the one in vm.topCallFrame from the register. I don't think Mark's concern is valid as this is the code that first discovers that there was an exception in the called C code. Therefore there shouldn't have been any unwinding. We're about ready to call ctiVMHandleException() which will call cti_vm_handle_exception() which will call jitThrow() which will unwind. 926771 5 msaboff 2013-09-09 10:25:39 -0700 (In reply to comment #2) > (From update of attachment 210127 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210127&action=review > > > Source/JavaScriptCore/jit/SlowPathCall.h:91 > > - m_jit->reloadCallFrameFromTopCallFrame(); > > + m_jit->updateTopCallFrame(); > > This looks strange to me. At this point, given that we have an exception, the caller may have unwound the stack already. We should not assume that the current stack frame is even valid. Have you tested against the tests in https://bugs.webkit.org/show_bug.cgi?id=119848 yet? I tested the patch with w276-reduced.js attached to https://bugs.webkit.org/show_bug.cgi?id=119848 and it worked fine. 926949 6 210127 ggaren 2013-09-09 15:26:05 -0700 Comment on attachment 210127 Patch r=me 926965 7 210127 oliver 2013-09-09 15:42:49 -0700 Comment on attachment 210127 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210127&action=review >>> Source/JavaScriptCore/jit/SlowPathCall.h:91 >>> + m_jit->updateTopCallFrame(); >> >> This looks strange to me. At this point, given that we have an exception, the caller may have unwound the stack already. We should not assume that the current stack frame is even valid. Have you tested against the tests in https://bugs.webkit.org/show_bug.cgi?id=119848 yet? > > I tested the patch with w276-reduced.js attached to https://bugs.webkit.org/show_bug.cgi?id=119848 and it worked fine. Mark, we cannot unwind the stack through host code - if execution is returning to js we have by definition rolled back only as far the the frame we're moving into. 926974 8 msaboff 2013-09-09 16:02:22 -0700 Committed r155399: <http://trac.webkit.org/changeset/155399> 210127 2013-08-30 10:09:32 -0700 2013-09-09 15:42:48 -0700 Patch 120537.patch text/plain 2515 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU0ODkyKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDEzLTA4LTMwICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIFdyb25nIGZvciBTbG93UGF0aENhbGwgdG8gbG9hZCBjYWxsRnJhbWUgcmVnIGZyb20gdm0u dG9wQ2FsbEZyYW1lIGFmdGVyIGNhbGwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTEyMDUzNworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIENoYW5nZWQgSklUU2xvd1BhdGhDYWxsOjpjYWxsKCkgdG8gdXBkYXRl IHZtLnRvcENhbGxGcmFtZSBmcm9tIHRoZSBjYWxsRnJhbWVSZWdpc3RlciBpbnN0ZWFkIG9mIHRo ZQorICAgICAgICBvdGhlciB3YXkgYXJvdW5kLgorCisgICAgICAgICogaml0L0pJVC5oOgorICAg ICAgICAqIGppdC9KSVRJbmxpbmVzLmg6CisgICAgICAgICogaml0L1Nsb3dQYXRoQ2FsbC5oOgor ICAgICAgICAoSlNDOjpKSVRTbG93UGF0aENhbGw6OmNhbGwpOgorCiAyMDEzLTA4LTMwICBDaHJp cyBDdXJ0aXMgIDxjaHJpc19jdXJ0aXNAYXBwbGUuY29tPgogCiAgICAgICAgIENsZWFuaW5nIGVy cm9yRGVzY3JpcHRpb25Gb3JWYWx1ZSBhZnRlciByMTU0ODM5CkluZGV4OiBTb3VyY2UvSmF2YVNj cmlwdENvcmUvaml0L0pJVC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9q aXQvSklULmgJKHJldmlzaW9uIDE1NDg0NSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQv SklULmgJKHdvcmtpbmcgY29weSkKQEAgLTgzOCw3ICs4MzgsNiBAQCBuYW1lc3BhY2UgSlNDIHsK IAogICAgICAgICB2b2lkIHJlc3RvcmVBcmd1bWVudFJlZmVyZW5jZUZvclRyYW1wb2xpbmUoKTsK ICAgICAgICAgdm9pZCB1cGRhdGVUb3BDYWxsRnJhbWUoKTsKLSAgICAgICAgdm9pZCByZWxvYWRD YWxsRnJhbWVGcm9tVG9wQ2FsbEZyYW1lKCk7CiAKICAgICAgICAgQ2FsbCBlbWl0TmFrZWRDYWxs KENvZGVQdHIgZnVuY3Rpb24gPSBDb2RlUHRyKCkpOwogCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlw dENvcmUvaml0L0pJVElubGluZXMuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENv cmUvaml0L0pJVElubGluZXMuaAkocmV2aXNpb24gMTU0ODQ1KQorKysgU291cmNlL0phdmFTY3Jp cHRDb3JlL2ppdC9KSVRJbmxpbmVzLmgJKHdvcmtpbmcgY29weSkKQEAgLTE5MSwxMSArMTkxLDYg QEAgQUxXQVlTX0lOTElORSB2b2lkIEpJVDo6dXBkYXRlVG9wQ2FsbEZyYQogICAgIHN0b3JlUHRy KGNhbGxGcmFtZVJlZ2lzdGVyLCAmbV92bS0+dG9wQ2FsbEZyYW1lKTsKIH0KIAotQUxXQVlTX0lO TElORSB2b2lkIEpJVDo6cmVsb2FkQ2FsbEZyYW1lRnJvbVRvcENhbGxGcmFtZSgpCi17Ci0gICAg bG9hZFB0cigmbV92bS0+dG9wQ2FsbEZyYW1lLCBjYWxsRnJhbWVSZWdpc3Rlcik7Ci19Ci0KIEFM V0FZU19JTkxJTkUgdm9pZCBKSVQ6OnJlc3RvcmVBcmd1bWVudFJlZmVyZW5jZUZvclRyYW1wb2xp bmUoKQogewogI2lmIENQVShYODYpCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L1Ns b3dQYXRoQ2FsbC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvU2xv d1BhdGhDYWxsLmgJKHJldmlzaW9uIDE1NDg0NSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9q aXQvU2xvd1BhdGhDYWxsLmgJKHdvcmtpbmcgY29weSkKQEAgLTg4LDcgKzg4LDcgQEAgcHVibGlj OgogI2Vsc2UKICAgICAgICAgSklUOjpKdW1wIG5vRXhjZXB0aW9uID0gbV9qaXQtPmJyYW5jaFRl c3Q2NChKSVQ6Olplcm8sIEpJVDo6QWJzb2x1dGVBZGRyZXNzKG1faml0LT5tX2NvZGVCbG9jay0+ dm0oKS0+YWRkcmVzc09mRXhjZXB0aW9uKCkpKTsKICNlbmRpZgotICAgICAgICBtX2ppdC0+cmVs b2FkQ2FsbEZyYW1lRnJvbVRvcENhbGxGcmFtZSgpOworICAgICAgICBtX2ppdC0+dXBkYXRlVG9w Q2FsbEZyYW1lKCk7CiAgICAgICAgIG1faml0LT5tb3ZlKEpJVDo6VHJ1c3RlZEltbVB0cihGdW5j dGlvblB0cihjdGlWTUhhbmRsZUV4Y2VwdGlvbikudmFsdWUoKSksIEpJVDo6cmVnVDEpOwogICAg ICAgICBtX2ppdC0+anVtcChKSVQ6OnJlZ1QxKTsKICAgICAgICAgbm9FeGNlcHRpb24ubGluayht X2ppdCk7Cg==