117801 2013-06-19 10:30:08 -0700 JSC: StackPolicy should use a more reasonable requiredStack value 2013-06-21 09:37:10 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 116661 117862 1 mark.lam mark.lam bdakin bfulgham commit-queue ggaren paroga rniwa oldest_to_newest 901837 0 mark.lam 2013-06-19 10:30:08 -0700 This issue was discovered while trying to apply the patch from https://bugs.webkit.org/show_bug.cgi?id=116661. The testsapi tests fails with an ASSERTION failure if the available stack discovered by StackBounds is only 242K. There are 2 issues found so far: 1. The StackPolicy (in Interpreter.cpp) is asserting that the stack should be greater than 256K. On Windows, the entire stack size (before we discount the guard page) is 256K. Hence, this assertion will always fail on Windows if the StackBounds is computed correctly. 2. If we loosen the restriction in issue (1), we can run jsc far enough until we hit another assertion failure in testapi. The root cause for this assertion failure is not yet known. The 256K stack size requirement in StackPolicy was chosen based on heuristics. However, it seems improbable that we aren't able to run testapi on 242K of stack. Will need to investigate the root cause of issue (2). We also need to reconsider if the 256K requirement in issue (1) is appropriate. 901842 1 mark.lam 2013-06-19 10:33:50 -0700 FYI, if I artificially restrict the stack size to 242K on OSX, I get the same assertion failures. Will continue debugging this issue on OSX. 901894 2 ggaren 2013-06-19 12:32:09 -0700 > We also need to reconsider if the 256K requirement in issue (1) is appropriate. It isn't. 902165 3 mark.lam 2013-06-20 08:46:51 -0700 (In reply to comment #1) > FYI, if I artificially restrict the stack size to 242K on OSX, I get the same assertion failures. Will continue debugging this issue on OSX. Issue (2) is not an issue. I encountered that assertion failure because in the test I ran, I changed the stack size to 242K and the StackPolicy's requiredStack size to 242K as well. Hence, right off the start based on these values, we're out of stack space. The test is invalid. Hence, issue (2) is not an issue. What remains is issue (1) i.e. we need to pick a more sane value for the StackPolicy's requiredStack. I've confirmed that on Windows which has a ~256K stack, changing the requiredStack to 100K (just for testing … not saying that this is the appropriate value to use), the run-javascriptcore-tests ran to completion with 0 errors. 902344 4 205125 mark.lam 2013-06-20 15:10:24 -0700 Created attachment 205125 the patch 902345 5 commit-queue 2013-06-20 15:12:21 -0700 Attachment 205125 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/interpreter/Interpreter.cpp']" exit_code: 1 Source/JavaScriptCore/ChangeLog:27: Line contains tab character. [whitespace/tab] [5] Source/JavaScriptCore/ChangeLog:30: Line contains tab character. [whitespace/tab] [5] Source/JavaScriptCore/ChangeLog:31: Line contains tab character. [whitespace/tab] [5] Total errors found: 3 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 902346 6 205125 ggaren 2013-06-20 15:14:50 -0700 Comment on attachment 205125 the patch r=me 902349 7 mark.lam 2013-06-20 15:18:32 -0700 Landed in r151808: <http://trac.webkit.org/changeset/151808>. 902370 8 bdakin 2013-06-20 17:22:20 -0700 This change caused fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html to start failing. 902421 9 rniwa 2013-06-20 20:55:28 -0700 --- /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/fast/xmlhttprequest/xmlhttprequest-recursive-sync-event-expected.txt +++ /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/fast/xmlhttprequest/xmlhttprequest-recursive-sync-event-actual.txt @@ -1,5 +1,5 @@ -CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded. -CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded. +CONSOLE MESSAGE: +CONSOLE MESSAGE: This tests that having infinite recursion in XMLHttpRequest event handler does not crash. PASS Why are we not seeing any error messages now? 902422 10 rniwa 2013-06-20 20:57:31 -0700 It makes sense if those console message stopped appearing but they are still there just without an useful error message. Does anyone know why we have two empty console messages? 902424 11 rniwa 2013-06-20 20:59:17 -0700 Filed https://bugs.webkit.org/show_bug.cgi?id=117801 to track the tea failure. 902581 12 mark.lam 2013-06-21 09:22:57 -0700 (In reply to comment #9) > -CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded. > -CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded. > +CONSOLE MESSAGE: > +CONSOLE MESSAGE: > > Why are we not seeing any error messages now? The reason is because the interpreter StackPolicy is not used universally in the VM. There are parts of the VM that does stack checks based on the default StackBound capacity requirement. The default StackBound capacity requirement turned out to be larger than the interpreter StackPolicy requirements. So, when handling the StackOverflowError, we were able to re-enter the interpreter (which we need to do to create the message string), but we soon fail in StringRecursionCheck which uses the default StackBound check. This causes the String operation to be aborted. The result is the blank message. The fix is to either increase the interpreter StackPolicy values to match the default StackBounds values, or reduce the StackBounds values to match the interpreter StackPolicy values. For now, to be conservative, I'll increase the interpreter StackPolicy values. The fix will be tracked in https://bugs.webkit.org/show_bug.cgi?id=117862. > Does anyone know why we have two empty console messages? I suspect this is an artifact of the test using even handlers. If an event was already queued while we're handling the StackOverflow, WebCore will invoke another event listener and get a 2nd stack overflow. The only thing that changed after this patch is that we're not able to create the string for the error message. We were getting 2 StackOverflowErrors before, and we're still getting 2 now. 902587 13 ggaren 2013-06-21 09:37:10 -0700 > For now, to be conservative, I'll increase the interpreter StackPolicy values. Why do we have two different ways to check for stack overflow at all. This is way over-engineered. The whole point of the patch that introduced the StackPolicy class was to get all of our code onto a single, shared mechanism for checking for stack overflow. 205125 2013-06-20 15:10:24 -0700 2013-06-20 15:14:50 -0700 the patch bug-117801.patch text/plain 3980 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTUxODAxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDU5IEBA CisyMDEzLTA2LTIwICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBD aGFuZ2Ugc3RhY2sgY2FwYWNpdHkgcmVxdWlyZW1lbnQgdG8gYmUgbW9yZSByZWFzb25hYmxlLgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE3ODAxLgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFByZXZpb3Vz bHksIHRoZSByZXF1aXJlZFN0YWNrIGluIFN0YWNrUG9saWN5OjpTdGFja1BvbGljeSgpIHdhcyBz ZXQgdG8KKyAgICAgICAgdG8gYSBoaWdoIHZhbHVlIGxpa2UgMjU2SyB0byByZWR1Y2UgdGhlIGNo YW5jZXMgb2YgZW5jb3VudGVyaW5nIGFuCisgICAgICAgIHVuZGV0ZWN0ZWQgc3RhY2sgb3ZlcmZs b3cgaW4gYSBzY2VuYXJpbyB3aGVyZSB3ZSBoYXZlIGEgY29tYmluYXRpb24gb2YKKyAgICAgICAg ZGVlcGx5IG5lc3RlZCBkaXZzIGFuZCBhIGxhcmdlIGFtb3VudCByZWN1cnNpdmUgcmUtZW50cmll cyBpbnRvIHRoZSBWTS4KKworICAgICAgICBIb3dldmVyLCB0aGlzIGhpZ2ggdmFsdWUgb2YgcmVx dWlyZWRTdGFjayBzdGlsbCBkb2VzIG5vdCBjb21wbGV0ZWx5CisgICAgICAgIGVuc3VyZSB0aGF0 IHdlIHdpbGwgbmV2ZXIgZW5jb3VudGVyIGFuIHVuZGV0ZWN0ZWQgc3RhY2sgb3ZlcmZsb3cuIEl0 CisgICAgICAgIG9ubHkgbGVzc2VucyB0aGUgcHJvYmFiaWxpdHkgb2YgZW5jb3VudGVyaW5nIGl0 LgorCisgICAgICAgIFNlY29uZGx5LCBvbiBzb21lIHBsYXRmb3JtcywgdGhlIHRvdGFsIHN0YWNr IHNpemUgY2FuIGJlIGxlc3MgdGhhbiAyNTZLCisgICAgICAgIHRvIHN0YXJ0IHdpdGguIEhlbmNl LCB0aGlzIGhpZ2ggdmFsdWUgcmVxdWlyZWRTdGFjayByZW5kZXJzIHRoZSBWTQorICAgICAgICB1 bnVzZWFibGUgb24gdGhvc2UgcGxhdGZvcm1zLgorCisgICAgICAgIFRoaXMgcGF0Y2ggd2lsbCBm aXggdGhlIHJlcXVpcmVkU3RhY2sgdG8gYmUgbW9yZSByZWFzb25hYmxlIGJhc2VkIG9uCisgICAg ICAgIHJlYWwgd29ybGQgc3RhY2sgdXNhZ2UgYnkgdGhlIFZNLiBXZSB3b24ndCAoYW5kIGNhbm5v dCkgdHJ5IHRvIHByZXZlbnQKKyAgICAgICAgdW5kZXRlY3RlZCBzdGFjayBvdmVyZmxvd3Mgb3V0 c2lkZSBvZiBKU0MgYXMgd2VsbC4gRXh0ZXJuYWwgY29kZSB0aGF0CisgICAgICAgIGRvIGRlZXAg cmVjdXJzaW9uIChlLmcuIERvY3VtbmV0Ojp1cGRhdGVMYXlvdXQoKSkgc2hvdWxkIGRvIHRoZWly IG93bgorICAgICAgICBzdGFjayBjaGVja3MuCisKKwlGcm9tIGEgcHJldmlvdXMgZXhwZXJpbWVu dCwgd2UgbWVhc3VyZWQgdGhlIGZvbGxvd2luZzoKKworICAgICAgICBPbiBhIGRlYnVnIGJ1aWxk IG9uIE9TWDoKKwkxLiBTdGFjayB1c2FnZSBkaWZmZXJlbnQgYmV0d2VlbiByZWN1cnNpdmUgY2Fs bHMgdG8gaW50ZXJwcmV0ZXIgZW50cnk6CisJICAgNzc0NCBieXRlcworICAgICAgICBPbiBhIHJl bGVhc2UgYnVpbGQgb24gT1NYOgorICAgICAgICAyLiBTdGFjayB1c2FnZSBkaWZmZXJlbmNlIGJl dHdlZW4gcmVjdXJzaXZlIGNhbGxzIHRvIGludGVycHJldGVyIGVudHJ5OgorICAgICAgICAgICA2 MzUyIGJ5dGVzCisKKyAgICAgICAgVXNpbmcgdGhlc2UgYXMgYSBndWlkZSwgd2UnbGwgcGljayB0 aGUgZm9sbG93aW5nIHZhbHVlcyBmb3IgdGhlCisgICAgICAgIFN0YWNrUG9saWN5OgorICAgICAg ICAgICByZXF1aXJlZFN0YWNrOiAzMksKKyAgICAgICAgICAgZXJyb3JNb2RlUmVxdWlyZWRTdGFj azogMTZLCisKKyAgICAgICAgVGhlIHJlcXVpcmVkU3RhY2sgaXMgY2hvc2VuIHRvIGJlIDR4IHRo ZSBtZWFzdXJlZCB1c2FnZSBhYm92ZS4gVGhlCisgICAgICAgIGFkZGl0aW9uYWwgM3ggaXMgYSBj b25zZXJ2YXRpdmUgZXN0aW1hdGUgdG8gYWNjb3VudCBmb3Igc3RhY2sgc3BhY2UKKyAgICAgICAg dGhhdCBtYXkgYmUgbmVlZGVkIGJ5IG90aGVyIG5hdGl2ZSBmdW5jdGlvbnMgY2FsbGVkIHdoaWxl IGluIHRoZQorICAgICAgICBpbnRlcnByZXRlci4KKworICAgICAgICBUaGUgZXJyb3JNb2RlUmVx dWlyZWRTdGFjayBoYXMgdG8gYmUgbGVzcyB0aGFuIHRoZSByZXF1aXJlZFN0YWNrIG9yIHdlCisg ICAgICAgIHdvbid0IGJlIGFibGUgdG8gcmVlbnRlciB0aGUgaW50ZXJwcmV0ZXIgdG8gZG8gZXJy b3IgaGFuZGxpbmcgd29yayB3aGVuCisgICAgICAgIGFuIGltbWluZW50IHN0YWNrIG92ZXJmbG93 IGlzIGRldGVjdGVkLiBJdCBpcyBhc3N1bWVkIHRoYXQgdGhlIGVycm9yCisgICAgICAgIGhhbmRs aW5nIGNvZGUgd2lsbCBvbmx5IGRvIG1pbmltYWwgd29yayB0byBhbGxvY2F0ZSBhbiBleGNlcHRp b24gYW5kIGl0cworICAgICAgICBzdGFjayB0cmFjZSwgYW5kIG5vdCBydW4gYW55IGFyYml0cmFy eSBKUyBjb2RlLiBBcyBzdWNoLCBpdCBpcyBzYWZlIHRvCisgICAgICAgIGFsbG93IHJlLWVudHJ5 IGludG8gdGhlIGludGVycHJldGVyIHdpdGggb25seSAyeCB0aGUgbWVhc3VyZWQgdXNhZ2UgaW4K KyAgICAgICAgdGhpcyBjYXNlLgorCisgICAgICAgICogaW50ZXJwcmV0ZXIvSW50ZXJwcmV0ZXIu Y3BwOgorICAgICAgICAoSlNDOjpJbnRlcnByZXRlcjo6U3RhY2tQb2xpY3k6OlN0YWNrUG9saWN5 KToKKwogMjAxMy0wNi0yMCAgTWlraGFpbCBQb3pkbnlha292ICA8bWlraGFpbC5wb3pkbnlha292 QGludGVsLmNvbT4KIAogICAgICAgICBIYXNoU2V0OiByZXZlcnNlIHRoZSBvcmRlciBvZiB0aGUg dGVtcGxhdGUgYXJndW1lbnRzIGF0IGFsdGVybmF0ZSAnZmluZCcsICdjb250YWlucycgYW5kICdh ZGQnIG1ldGhvZHMKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9pbnRlcnByZXRlci9JbnRl cnByZXRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2ludGVycHJl dGVyL0ludGVycHJldGVyLmNwcAkocmV2aXNpb24gMTUxNzg3KQorKysgU291cmNlL0phdmFTY3Jp cHRDb3JlL2ludGVycHJldGVyL0ludGVycHJldGVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTI2 LDEyICsxMjYsMTIgQEAgSW50ZXJwcmV0ZXI6OlN0YWNrUG9saWN5OjpTdGFja1BvbGljeShJbgog ICAgIC8vCiAgICAgLy8gVGhlc2Ugc2l6ZXMgd2VyZSBkZXJpdmVkIGZyb20gdGhlIHN0YWNrIHVz YWdlIG9mIGEgbnVtYmVyIG9mIHNpdGVzIHdoZW4KICAgICAvLyBsYXlvdXQgb2NjdXJzIHdoZW4g d2UndmUgYWxyZWFkeSBjb25zdW1lZCBtb3N0IG9mIHRoZSBDIHN0YWNrLgotICAgIGNvbnN0IHNp emVfdCByZXF1aXJlZFN0YWNrID0gMjU2ICogS0I7Ci0gICAgY29uc3Qgc2l6ZV90IGVycm9yTW9k ZVJlcXVpcmVkU3RhY2sgPSA2NCAqIEtCOworICAgIGNvbnN0IHNpemVfdCByZXF1aXJlZFN0YWNr ID0gMzIgKiBLQjsKKyAgICBjb25zdCBzaXplX3QgZXJyb3JNb2RlUmVxdWlyZWRTdGFjayA9IDE2 ICogS0I7CiAKICAgICBzaXplX3QgcmVxdWlyZWRDYXBhY2l0eSA9IG1faW50ZXJwcmV0ZXIubV9l cnJvckhhbmRsaW5nTW9kZVJlZW50cnkgPyBlcnJvck1vZGVSZXF1aXJlZFN0YWNrIDogcmVxdWly ZWRTdGFjazsKIAotICAgIFJFTEVBU0VfQVNTRVJUKHNpemUgPiByZXF1aXJlZENhcGFjaXR5KTsK KyAgICBSRUxFQVNFX0FTU0VSVChzaXplID49IHJlcXVpcmVkQ2FwYWNpdHkpOwogICAgIAogICAg IG1fcmVxdWlyZWRDYXBhY2l0eSA9IHJlcXVpcmVkQ2FwYWNpdHk7ICAgIAogfQo=