146767 2015-07-08 18:18:38 -0700 REGRESSION (r180248): Repro Crash: com.apple.WebKit.WebContent at com.apple.JavaScriptCore: JSC::createRangeError + 20 2015-07-23 08:32:45 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED http://recc.robertelder.org/compile-demo InRadar P2 Normal --- 1 msaboff msaboff bfulgham mark.lam oldest_to_newest 1108034 0 msaboff 2015-07-08 18:18:38 -0700 We are crashing trying to execute a JavaScript main program. <rdar://problem/21659085> 1108229 1 256484 msaboff 2015-07-09 10:27:32 -0700 Created attachment 256484 Patch This also fixes LayoutTests/js/function-apply-aliased.html when run with the C Loop. 1108250 2 256484 mark.lam 2015-07-09 11:13:38 -0700 Comment on attachment 256484 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256484&action=review > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-478 > - // This stack check is done in the prologue for a function call, and the > - // CallFrame is not completely set up yet. For example, if the frame needs > - // a lexical environment object, the lexical environment object will only be > - // set up after we start executing the function. If we need to throw a > - // StackOverflowError here, then we need to tell the prologue to start the > - // stack unwinding from the caller frame (which is fully set up) instead. > - // To do that, we return the caller's CallFrame in the second return value. > - // The original patch that added this comment is http://trac.webkit.org/changeset/161084 (see https://bugs.webkit.org/show_bug.cgi?id=126174). It was added to fix a test failure: jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-cjit. However, the key change for that patch was to return a non-zero exec value (previously returning a null exec even when an exception is thrown). The code that accesses the uninitialized (not fully initialized) frame is in the caller of stack_check(). The use of exec = exec->callerFrame() existed since the introduction of the LLINT (see http://trac.webkit.org/changeset/108444). If all the tests passes with this patch, we have high confidence that it is good. However, to be safe, I'd look into what frame values are left to be initialized in the function prologue (i.e. the frame fields that remain uninitialized at this point), and either initialized them to some sane value, or at least make sure that they won't ever be used in the exception handling / unwinding code. I'm nervous that we are setting loose a partially initialized frame on the VM. 1108267 3 256484 ggaren 2015-07-09 11:42:30 -0700 Comment on attachment 256484 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256484&action=review r=me It seems like >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-478 >> - // > > The original patch that added this comment is http://trac.webkit.org/changeset/161084 (see https://bugs.webkit.org/show_bug.cgi?id=126174). It was added to fix a test failure: jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-cjit. However, the key change for that patch was to return a non-zero exec value (previously returning a null exec even when an exception is thrown). The code that accesses the uninitialized (not fully initialized) frame is in the caller of stack_check(). The use of exec = exec->callerFrame() existed since the introduction of the LLINT (see http://trac.webkit.org/changeset/108444). > > If all the tests passes with this patch, we have high confidence that it is good. However, to be safe, I'd look into what frame values are left to be initialized in the function prologue (i.e. the frame fields that remain uninitialized at this point), and either initialized them to some sane value, or at least make sure that they won't ever be used in the exception handling / unwinding code. I'm nervous that we are setting loose a partially initialized frame on the VM. It looks like the patch that added this code was about activation tear-off, which doesn't exist anymore. Perhaps we need to remove the two other uses of "exec = exec->callerFrame" preceding CommonSlowPaths::interpreterThrowInCaller as well. > LayoutTests/http/tests/misc/large-js-program.php:33 > +for ($i = 0; $i < 1000000; $i++) { > + if ($i != 0) > + echo ",\n"; > + echo "[0, $i]"; This could be our awesomest test yet! 1108274 4 mark.lam 2015-07-09 11:53:32 -0700 Michael has convinced me that the call frame is indeed properly set up by the time we get to the prologue where we do the stack check. So, there is no cause for concern. r=me too. 1108297 5 msaboff 2015-07-09 12:45:22 -0700 (In reply to comment #3) > > If all the tests passes with this patch, we have high confidence that it is good. However, to be safe, I'd look into what frame values are left to be initialized in the function prologue (i.e. the frame fields that remain uninitialized at this point), and either initialized them to some sane value, or at least make sure that they won't ever be used in the exception handling / unwinding code. I'm nervous that we are setting loose a partially initialized frame on the VM. As Mark replied, we looked at the code together and believe that the call frame is properly initialized for exception handling and unwinding before we make this check. > It looks like the patch that added this code was about activation tear-off, > which doesn't exist anymore. Perhaps we need to remove the two other uses of > "exec = exec->callerFrame" preceding > CommonSlowPaths::interpreterThrowInCaller as well. Those are for the case that we have a stack overflow during arity check. It actually makes sense that the exception is thrown from the caller and not the callee, as a call with proper arity could not be made due to stack overflow. 1108316 6 msaboff 2015-07-09 13:25:42 -0700 Committed r186606: <http://trac.webkit.org/changeset/186606> 1109113 7 bfulgham 2015-07-13 09:30:40 -0700 This new test fails repeatably on Windows. Can you please take a look? 1109115 8 msaboff 2015-07-13 09:39:21 -0700 (In reply to comment #7) > This new test fails repeatably on Windows. Can you please take a look? Looking at it now. 1111644 9 mark.lam 2015-07-23 08:32:45 -0700 *** Bug 141671 has been marked as a duplicate of this bug. *** 256484 2015-07-09 10:27:32 -0700 2015-07-09 11:42:30 -0700 Patch 146767.patch text/plain 4848 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTg2NTkzKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE1LTA3LTA5ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIFJFR1JFU1NJT04gKHIxODAyNDgpOiBSZXBybyBDcmFzaDogY29tLmFwcGxlLldlYktpdC5X ZWJDb250ZW50IGF0IGNvbS5hcHBsZS5KYXZhU2NyaXB0Q29yZTogSlNDOjpjcmVhdGVSYW5nZUVy cm9yICsgMjAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTE0Njc2NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IElmIHRoZSBzdGFjayBjaGVjayBmYWlscyBhdCB0aGUgdG9wIG1vc3QgZnJhbWUsIHdlIG11c3Qg dXNlIHRoYXQgZnJhbWUgdG8KKyAgICAgICAgZ2VuZXJhdGUgdGhlIGV4Y2VwdGlvbi4gIFJldmVy dGVkIHRoZSBjb2RlIHRvIGFsd2F5cyB1c2UgdGhlIGN1cnJlbnQgZnJhbWUgdG8KKyAgICAgICAg dGhyb3cgYW4gb3V0IG9mIHN0YWNrIGV4Y2VwdGlvbi4KKworICAgICAgICAqIGxsaW50L0xMSW50 U2xvd1BhdGhzLmNwcDoKKyAgICAgICAgKEpTQzo6TExJbnQ6OkxMSU5UX1NMT1dfUEFUSF9ERUNM KToKKwogMjAxNS0wNy0wOCAgSm9zZXBoIFBlY29yYXJvICA8cGVjb3Jhcm9AYXBwbGUuY29tPgog CiAgICAgICAgIEZpeCBncmFtbWFyIGlzc3VlIGluIFR5cGVFcnJvciBhdHRlbXB0aW5nIHRvIGNo YW5nZSBhbiB1bmNvbmZpZ3VyYWJsZSBwcm9wZXJ0eQpJbmRleDogU291cmNlL0phdmFTY3JpcHRD b3JlL2xsaW50L0xMSW50U2xvd1BhdGhzLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNj cmlwdENvcmUvbGxpbnQvTExJbnRTbG93UGF0aHMuY3BwCShyZXZpc2lvbiAxODY1OTMpCisrKyBT b3VyY2UvSmF2YVNjcmlwdENvcmUvbGxpbnQvTExJbnRTbG93UGF0aHMuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC00NjgsMTQgKzQ2OCw2IEBAIExMSU5UX1NMT1dfUEFUSF9ERUNMKHN0YWNrX2NoZWNr KQogI2VuZGlmCiAKICNlbmRpZgotICAgIC8vIFRoaXMgc3RhY2sgY2hlY2sgaXMgZG9uZSBpbiB0 aGUgcHJvbG9ndWUgZm9yIGEgZnVuY3Rpb24gY2FsbCwgYW5kIHRoZQotICAgIC8vIENhbGxGcmFt ZSBpcyBub3QgY29tcGxldGVseSBzZXQgdXAgeWV0LiBGb3IgZXhhbXBsZSwgaWYgdGhlIGZyYW1l IG5lZWRzCi0gICAgLy8gYSBsZXhpY2FsIGVudmlyb25tZW50IG9iamVjdCwgdGhlIGxleGljYWwg ZW52aXJvbm1lbnQgb2JqZWN0IHdpbGwgb25seSBiZQotICAgIC8vIHNldCB1cCBhZnRlciB3ZSBz dGFydCBleGVjdXRpbmcgdGhlIGZ1bmN0aW9uLiBJZiB3ZSBuZWVkIHRvIHRocm93IGEKLSAgICAv LyBTdGFja092ZXJmbG93RXJyb3IgaGVyZSwgdGhlbiB3ZSBuZWVkIHRvIHRlbGwgdGhlIHByb2xv Z3VlIHRvIHN0YXJ0IHRoZQotICAgIC8vIHN0YWNrIHVud2luZGluZyBmcm9tIHRoZSBjYWxsZXIg ZnJhbWUgKHdoaWNoIGlzIGZ1bGx5IHNldCB1cCkgaW5zdGVhZC4KLSAgICAvLyBUbyBkbyB0aGF0 LCB3ZSByZXR1cm4gdGhlIGNhbGxlcidzIENhbGxGcmFtZSBpbiB0aGUgc2Vjb25kIHJldHVybiB2 YWx1ZS4KLSAgICAvLwogICAgIC8vIElmIHRoZSBzdGFjayBjaGVjayBzdWNjZWVkcyBhbmQgd2Ug ZG9uJ3QgbmVlZCB0byB0aHJvdyB0aGUgZXJyb3IsIHRoZW4KICAgICAvLyB3ZSdsbCByZXR1cm4g MCBpbnN0ZWFkLiBUaGUgcHJvbG9ndWUgd2lsbCBjaGVjayBmb3IgYSBub24temVybyB2YWx1ZQog ICAgIC8vIHdoZW4gZGV0ZXJtaW5pbmcgd2hldGhlciB0byBzZXQgdGhlIGNhbGxGcmFtZSBvciBu b3QuCkBAIC00ODksNyArNDgxLDYgQEAgTExJTlRfU0xPV19QQVRIX0RFQ0woc3RhY2tfY2hlY2sp CiAgICAgICAgIExMSU5UX1JFVFVSTl9UV08ocGMsIDApOwogI2VuZGlmCiAKLSAgICBleGVjID0g ZXhlYy0+Y2FsbGVyRnJhbWUodm0udG9wVk1FbnRyeUZyYW1lKTsKICAgICB2bS50b3BDYWxsRnJh bWUgPSBleGVjOwogICAgIEVycm9ySGFuZGxpbmdTY29wZSBlcnJvclNjb3BlKHZtKTsKICAgICBD b21tb25TbG93UGF0aHM6OmludGVycHJldGVyVGhyb3dJbkNhbGxlcihleGVjLCBjcmVhdGVTdGFj a092ZXJmbG93RXJyb3IoZXhlYykpOwpJbmRleDogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIExheW91dFRlc3RzL0NoYW5nZUxvZwkocmV2aXNpb24gMTg2NTkzKQorKysgTGF5 b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTUt MDctMDkgIE1pY2hhZWwgU2Fib2ZmICA8bXNhYm9mZkBhcHBsZS5jb20+CisKKyAgICAgICAgUkVH UkVTU0lPTiAocjE4MDI0OCk6IFJlcHJvIENyYXNoOiBjb20uYXBwbGUuV2ViS2l0LldlYkNvbnRl bnQgYXQgY29tLmFwcGxlLkphdmFTY3JpcHRDb3JlOiBKU0M6OmNyZWF0ZVJhbmdlRXJyb3IgKyAy MAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ2NzY3 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTmV3IHRl c3QgdGhhdCBnZW5lcmF0ZXMgYSBjYWxsIHRvIGEgZnVuY3Rpb24gdGhhdCBpbnZvbHZlcyBjcmVh dGluZyBhIGh1Z2UKKyAgICAgICAgb2JqZWN0IGxpdGVyYWwgdGhhdCBleGNlZWRzIHRoZSBhdmFp bGFibGUgc3RhY2sgc3BhY2UuCisKKyAgICAgICAgKiBodHRwL3Rlc3RzL21pc2MvbGFyZ2UtanMt cHJvZ3JhbS1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAgICAqIGh0dHAvdGVzdHMvbWlzYy9s YXJnZS1qcy1wcm9ncmFtLnBocDogQWRkZWQuCisKIDIwMTUtMDctMDkgIENocmlzIEZsZWl6YWNo ICA8Y2ZsZWl6YWNoQGFwcGxlLmNvbT4KIAogICAgICAgICBBWDogVm9pY2VPdmVyIGNhbm5vdCBn ZXQgdG8gYW55IGNvbnRlbnQgaW4gWWFob28gTWFpbCBtZXNzYWdlcyB0YWJsZQpJbmRleDogTGF5 b3V0VGVzdHMvaHR0cC90ZXN0cy9taXNjL2xhcmdlLWpzLXByb2dyYW0tZXhwZWN0ZWQudHh0Cj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIExheW91dFRlc3RzL2h0dHAvdGVzdHMvbWlzYy9sYXJnZS1qcy1wcm9ncmFt LWV4cGVjdGVkLnR4dAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3RzL2h0dHAvdGVzdHMvbWlz Yy9sYXJnZS1qcy1wcm9ncmFtLWV4cGVjdGVkLnR4dAkod29ya2luZyBjb3B5KQpAQCAtMCwwICsx LDUgQEAKK0NPTlNPTEUgTUVTU0FHRTogbGluZSAyNzogUmFuZ2VFcnJvcjogTWF4aW11bSBjYWxs IHN0YWNrIHNpemUgZXhjZWVkZWQuCitUaGlzIHRlc3RzIHZlcmlmaWVzIHRoYXQgYSBsYXJnZSBw cm9ncmFtIGRvZXNuJ3QgY3Jhc2ggSmF2YVNjcmlwdC4KKworVGhpcyB0ZXN0IHNob3VsZCBnZW5l cmF0ZSBhbiBvdXQgb2Ygc3RhY2sgZXhjZXB0aW9uLCBidXQgaGF2ZSBubyBvdGhlciBvdXRwdXQu IAorCkluZGV4OiBMYXlvdXRUZXN0cy9odHRwL3Rlc3RzL21pc2MvbGFyZ2UtanMtcHJvZ3JhbS5w aHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9taXNjL2xhcmdlLWpzLXBy b2dyYW0ucGhwCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9taXNjL2xh cmdlLWpzLXByb2dyYW0ucGhwCSh3b3JraW5nIGNvcHkpCkBAIC0wLDAgKzEsMzkgQEAKKzxodG1s PgorPGhlYWQ+Cis8c2NyaXB0PgoraWYgKHdpbmRvdy50ZXN0UnVubmVyKQorICAgIHRlc3RSdW5u ZXIuZHVtcEFzVGV4dCgpOworPC9zY3JpcHQ+Cis8L2hlYWQ+Cis8Ym9keT4KKzxoMT5UaGlzIHRl c3RzIHZlcmlmaWVzIHRoYXQgYSBsYXJnZSBwcm9ncmFtIGRvZXNuJiMzOTt0IGNyYXNoIEphdmFT Y3JpcHQuPC9oMT4KKzxwPlRoaXMgdGVzdCBzaG91bGQgZ2VuZXJhdGUgYW4gb3V0IG9mIHN0YWNr IGV4Y2VwdGlvbiwgYnV0IGhhdmUgbm8gb3RoZXIgb3V0cHV0LgorPGJyPgorPHByZSBpZD0iY29u c29sZSI+PC9wcmU+Cis8c2NyaXB0IHNyYz0iL2pzLXRlc3QtcmVzb3VyY2VzL2pzLXRlc3QtcHJl LmpzIj48L3NjcmlwdD4KKzxzY3JpcHQ+CitmdW5jdGlvbiBwcmludChtKQoreworICAgIGRvY3Vt ZW50LmdldEVsZW1lbnRCeUlkKCJjb25zb2xlIikuaW5uZXJIVE1MICs9IG0gKyAiPGJyPiI7Cit9 CisKK2Z1bmN0aW9uIGZvbyhvKQoreworICAgIC8vIFdlIHNob3VsZCBub3QgZ2V0IHRvIHRoaXMg Y29kZSwgd2Ugc2hvdWxkIHRocm93IGFuIG91dCBvZiBzdGFjayBleGNlcHRpb24gY2FsbGluZyBm b28oKS4KKyAgICB0ZXN0RmFpbGVkKCJXZSBzaG91bGQgbmV2ZXIgZ2V0IGhlcmUhIik7Cit9CisK KworZm9vKHsieCI6IDEsCisgICAgICJhIjogWworPD9waHAKK2ZvciAoJGkgPSAwOyAkaSA8IDEw MDAwMDA7ICRpKyspIHsKKyAgICBpZiAoJGkgIT0gMCkKKyAgICAgICAgZWNobyAiLFxuIjsKKyAg ICBlY2hvICJbMCwgJGldIjsKK30KKz8+CitdfSk7Cis8L3NjcmlwdD4KKzwvYm9keT4KKzwvaHRt bD4K