135143 2014-07-21 19:30:04 -0700 JSLock release should only modify the AtomicStringTable if it modified in acquire 2014-07-24 16:57:10 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 135192 1 joepeck webkit-unassigned barraclough commit-queue ggaren joepeck mhahnenberg thorton oldest_to_newest 1023862 0 joepeck 2014-07-21 19:30:04 -0700 JSLock could have an imbalanced and unexpected change to the global thread data's AtomicStringTable. The expected balance between JSLock::didAcquireLock and JSLock::willReleaseLock wouldn't happen if the VM was not set (during destruction of a VM). 1023863 1 235264 joepeck 2014-07-21 19:34:48 -0700 Created attachment 235264 [PATCH] Proposed Fix I have been trying to create a test for this but it is proving difficult. My plan is to create a JSContext on a non-main thread (so a non-main AtomicStringTable), delete the JSContext on the main thread (such that JSLock would have unbalanced lock and release leaving the wrong AtomicStringTable). But it seems I'm missing some complexity. In any case, the reproducible case I have (a larger application) reproduced the issue reliably. 1023867 2 joepeck 2014-07-21 19:48:23 -0700 <rdar://problem/17041912> 1023977 3 235264 psolanki 2014-07-22 10:25:14 -0700 Comment on attachment 235264 [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=235264&action=review Patch looks fine to me but would be good if Geoff took a look as well. > Source/JavaScriptCore/runtime/JSLock.cpp:170 > + if (m_vm) { Early return? 1024083 4 joepeck 2014-07-22 15:10:41 -0700 r171367: <http://trac.webkit.org/changeset/171367> 1024205 5 commit-queue 2014-07-23 03:28:57 -0700 Re-opened since this is blocked by bug 135192 1024207 6 thorton 2014-07-23 03:29:23 -0700 Stealing ap's message from the wrong bug: "This change broke three API tests, so I'm going to roll out. Tests that failed: WKUserContentController.ScriptMessageHandlerWithNavigation WebKit2.RestoreSessionStateContainingFormData Tests that timed out: DeviceScaleFactorOnBack.WebKit2" The UI process is stuck in a HashTable lock adding a string to the AtomicString table from inside decidePolicyForResponse 1024272 7 joepeck 2014-07-23 11:08:11 -0700 (In reply to comment #6) > Stealing ap's message from the wrong bug: > > "This change broke three API tests, so I'm going to roll out. > > Tests that failed: > WKUserContentController.ScriptMessageHandlerWithNavigation > WebKit2.RestoreSessionStateContainingFormData > Tests that timed out: > DeviceScaleFactorOnBack.WebKit2" I can reproduce. Thinking about things, now we might incorrectly balance in the opposite way after this change. We need to handle what will happen if: - we acquire the lock with a vm (set the atomic string table) - m_vm gets cleared - we release the lock without a vm => we need to restore the atomic string table. Making the appropriate changes the tests all pass. I need to test the original case and then I'll put up another patch. 1024320 8 235376 joepeck 2014-07-23 14:15:36 -0700 Created attachment 235376 [PATCH] Proposed Fix 1024324 9 235376 ggaren 2014-07-23 14:20:25 -0700 Comment on attachment 235376 [PATCH] Proposed Fix What if we set the table on entry, and it was null on entry? How will it get back to null? 1024366 10 joepeck 2014-07-23 15:56:01 -0700 (In reply to comment #9) > (From update of attachment 235376 [details]) > What if we set the table on entry, and it was null on entry? How will it get back to null? Good point. My understanding is that the WTFThreadData's AtomicStringTable can (and should) never be null. I could ASSERT that now, and perhaps ultimately we could move to references. Alternatively we could have a bool alongside the pointer. 1024526 11 ggaren 2014-07-24 10:06:53 -0700 (In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 235376 [details] [details]) > > What if we set the table on entry, and it was null on entry? How will it get back to null? > > Good point. My understanding is that the WTFThreadData's AtomicStringTable can (and should) never be null. I could ASSERT that now, and perhaps ultimately we could move to references. Alternatively we could have a bool alongside the pointer. It looks like WTFThreadData always sets a default table, so I think you're right. Let's ASSERT. 1024681 12 235474 joepeck 2014-07-24 16:55:54 -0700 Created attachment 235474 [PATCH] For Landing Added ASSERTs. Re-ran my tests without a hitch. 1024682 13 joepeck 2014-07-24 16:57:10 -0700 r171558: <http://trac.webkit.org/changeset/171558> 235264 2014-07-21 19:34:48 -0700 2014-07-23 14:15:36 -0700 [PATCH] Proposed Fix jslock-1.patch text/plain 1622 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDc1NTJlZTQuLjQ0MjM1ZmQgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE0LTA3LTIxICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CisKKyAgICAgICAgSlNMb2NrIHJlbGVhc2Ugc2hvdWxk IG9ubHkgbW9kaWZ5IHRoZSBBdG9taWNTdHJpbmdUYWJsZSBpZiBpdCBtb2RpZmllZCBpbiBhY3F1 aXJlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzUx NDMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHJ1 bnRpbWUvSlNMb2NrLmNwcDoKKyAgICAgICAgKEpTQzo6SlNMb2NrOjp3aWxsRGVzdHJveVZNKToK KyAgICAgICAgKEpTQzo6SlNMb2NrOjp3aWxsUmVsZWFzZUxvY2spOgorICAgICAgICBPbmx5IHNl dCB0aGUgQXRvbWljU3RyaW5nVGFibGUgd2hlbiB0aGVyZSB3YXMgYSBWTSwgdG8gYmFsYW5jZSBK U0xvY2s6OmRpZEFjcXVpcmVMb2NrLgorCiAyMDE0LTA3LTE3ICBKb3NlcGggUGVjb3Jhcm8gIDxw ZWNvcmFyb0BhcHBsZS5jb20+CiAKICAgICAgICAgRm9sbG93LXVwIGZpeCB0byByMTcxMTk1IHRv IHByZXZlbnQgQVNTRVJUIGluIGZhc3QvcHJvZmlsZXIvcHJvZmlsZS13aXRoLW5vLXRpdGxlLmh0 bWwKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTTG9jay5jcHAg Yi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0xvY2suY3BwCmluZGV4IDIxMTRmMTUu LmE1MzAzZmYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTTG9j ay5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNMb2NrLmNwcApAQCAt OTMsNyArOTMsNyBAQCBKU0xvY2s6On5KU0xvY2soKQogdm9pZCBKU0xvY2s6OndpbGxEZXN0cm95 Vk0oVk0qIHZtKQogewogICAgIEFTU0VSVF9VTlVTRUQodm0sIG1fdm0gPT0gdm0pOwotICAgIG1f dm0gPSAwOworICAgIG1fdm0gPSBudWxscHRyOwogfQogCiB2b2lkIEpTTG9jazo6c2V0RXhjbHVz aXZlVGhyZWFkKHN0ZDo6dGhyZWFkOjppZCB0aHJlYWRJZCkKQEAgLTE2NywxMCArMTY3LDExIEBA IHZvaWQgSlNMb2NrOjp1bmxvY2soaW50cHRyX3QgdW5sb2NrQ291bnQpCiAKIHZvaWQgSlNMb2Nr Ojp3aWxsUmVsZWFzZUxvY2soKQogewotICAgIGlmIChtX3ZtKQorICAgIGlmIChtX3ZtKSB7CiAg ICAgICAgIG1fdm0tPnNldFN0YWNrUG9pbnRlckF0Vk1FbnRyeShudWxscHRyKTsKIAotICAgIHd0 ZlRocmVhZERhdGEoKS5zZXRDdXJyZW50QXRvbWljU3RyaW5nVGFibGUobV9lbnRyeUF0b21pY1N0 cmluZ1RhYmxlKTsKKyAgICAgICAgd3RmVGhyZWFkRGF0YSgpLnNldEN1cnJlbnRBdG9taWNTdHJp bmdUYWJsZShtX2VudHJ5QXRvbWljU3RyaW5nVGFibGUpOworICAgIH0KIH0KIAogdm9pZCBKU0xv Y2s6OmxvY2soRXhlY1N0YXRlKiBleGVjKQo= 235376 2014-07-23 14:15:36 -0700 2014-07-24 16:55:54 -0700 [PATCH] Proposed Fix jslock-2.patch text/plain 2077 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDZiNzEyNjAuLjRjNWNkMzIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDE0LTA3LTIzICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CisKKyAgICAgICAgSlNMb2NrIHJlbGVhc2Ugc2hvdWxk IG9ubHkgbW9kaWZ5IHRoZSBBdG9taWNTdHJpbmdUYWJsZSBpZiBpdCBtb2RpZmllZCBpbiBhY3F1 aXJlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzUx NDMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHJ1 bnRpbWUvSlNMb2NrLmNwcDoKKyAgICAgICAgKEpTQzo6SlNMb2NrOjpKU0xvY2spOgorICAgICAg ICBJbml0aWFsaXplIHRoZSBtZW1iZXIgdmFyaWFibGUgdG8gbnVsbHB0ci4KKworICAgICAgICAo SlNDOjpKU0xvY2s6OndpbGxEZXN0cm95Vk0pOgorICAgICAgICBVcGRhdGUgc3R5bGUgdG8gdXNl IG51bGxwdHIgaW5zdGVhZCBvZiAwLgorCisgICAgICAgIChKU0M6OkpTTG9jazo6d2lsbFJlbGVh c2VMb2NrKToKKyAgICAgICAgV2Ugc2hvdWxkIG9ubHkgcmVzZXQgdGhlIHRocmVhZCBkYXRhJ3Mg YXRvbWljIHN0cmluZyB0YWJsZSBpZgorICAgICAgICBkaWRBY3F1aXJlTG9jayBjaGFuZ2VkIGl0 LiBtX2VudHJ5QXRvbWljU3RyaW5nVGFibGUgd2lsbCBoYXZlCisgICAgICAgIGJlZW4gc2V0IGJ5 IGRpZEFjcXVpcmVMb2NrIGlmIGl0IGNoYW5nZWQsIG9yIG51bGxwdHIgaWYgaXQgZGlkbid0Lgor ICAgICAgICBUaGlzIHdheSB3ZSBhcmUgc3VyZSB3ZSBhcmUgYmFsYW5jZWQsIHJlZ2FyZGxlc3Mg b2YgbV92bSBjaGFuZ2VzLgorCiAyMDE0LTA3LTIzICBCcmVudCBGdWxnaGFtICA8YmZ1bGdoYW1A YXBwbGUuY29tPgogCiAgICAgICAgIEJ1aWxkIGZpeCBhZnRlciByMTcxNDgyLgpkaWZmIC0tZ2l0 IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNMb2NrLmNwcCBiL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL0pTTG9jay5jcHAKaW5kZXggMjExNGYxNS4uNWIxMDk5ZSAxMDA2 NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNMb2NrLmNwcAorKysgYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0xvY2suY3BwCkBAIC04Myw2ICs4Myw3IEBA IEpTTG9jazo6SlNMb2NrKFZNKiB2bSkKICAgICAsIG1fbG9ja0Ryb3BEZXB0aCgwKQogICAgICwg bV9oYXNFeGNsdXNpdmVUaHJlYWQoZmFsc2UpCiAgICAgLCBtX3ZtKHZtKQorICAgICwgbV9lbnRy eUF0b21pY1N0cmluZ1RhYmxlKG51bGxwdHIpCiB7CiB9CiAKQEAgLTkzLDcgKzk0LDcgQEAgSlNM b2NrOjp+SlNMb2NrKCkKIHZvaWQgSlNMb2NrOjp3aWxsRGVzdHJveVZNKFZNKiB2bSkKIHsKICAg ICBBU1NFUlRfVU5VU0VEKHZtLCBtX3ZtID09IHZtKTsKLSAgICBtX3ZtID0gMDsKKyAgICBtX3Zt ID0gbnVsbHB0cjsKIH0KIAogdm9pZCBKU0xvY2s6OnNldEV4Y2x1c2l2ZVRocmVhZChzdGQ6OnRo cmVhZDo6aWQgdGhyZWFkSWQpCkBAIC0xNzAsNyArMTcxLDEwIEBAIHZvaWQgSlNMb2NrOjp3aWxs UmVsZWFzZUxvY2soKQogICAgIGlmIChtX3ZtKQogICAgICAgICBtX3ZtLT5zZXRTdGFja1BvaW50 ZXJBdFZNRW50cnkobnVsbHB0cik7CiAKLSAgICB3dGZUaHJlYWREYXRhKCkuc2V0Q3VycmVudEF0 b21pY1N0cmluZ1RhYmxlKG1fZW50cnlBdG9taWNTdHJpbmdUYWJsZSk7CisgICAgaWYgKG1fZW50 cnlBdG9taWNTdHJpbmdUYWJsZSkgeworICAgICAgICB3dGZUaHJlYWREYXRhKCkuc2V0Q3VycmVu dEF0b21pY1N0cmluZ1RhYmxlKG1fZW50cnlBdG9taWNTdHJpbmdUYWJsZSk7CisgICAgICAgIG1f ZW50cnlBdG9taWNTdHJpbmdUYWJsZSA9IG51bGxwdHI7CisgICAgfQogfQogCiB2b2lkIEpTTG9j azo6bG9jayhFeGVjU3RhdGUqIGV4ZWMpCg== 235474 2014-07-24 16:55:54 -0700 2014-07-24 16:56:35 -0700 [PATCH] For Landing final.patch text/plain 2571 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDhkNWRmZWUuLjIxOTM5MWQgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDE0LTA3LTI0ICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CisKKyAgICAgICAgSlNMb2NrIHJlbGVhc2Ugc2hvdWxk IG9ubHkgbW9kaWZ5IHRoZSBBdG9taWNTdHJpbmdUYWJsZSBpZiBpdCBtb2RpZmllZCBpbiBhY3F1 aXJlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzUx NDMKKworICAgICAgICBSZXZpZXdlZCBieSBEYXJpbiBBZGxlci4KKworICAgICAgICAqIHJ1bnRp bWUvSlNMb2NrLmNwcDoKKyAgICAgICAgKEpTQzo6SlNMb2NrOjpKU0xvY2spOgorICAgICAgICBJ bml0aWFsaXplIHRoZSBtZW1iZXIgdmFyaWFibGUgdG8gbnVsbHB0ci4KKworICAgICAgICAoSlND OjpKU0xvY2s6OndpbGxEZXN0cm95Vk0pOgorICAgICAgICBVcGRhdGUgc3R5bGUgdG8gdXNlIG51 bGxwdHIgaW5zdGVhZCBvZiAwLgorCisgICAgICAgIChKU0M6OkpTTG9jazo6d2lsbFJlbGVhc2VM b2NrKToKKyAgICAgICAgV2Ugc2hvdWxkIG9ubHkgcmVzZXQgdGhlIHRocmVhZCBkYXRhJ3MgYXRv bWljIHN0cmluZyB0YWJsZSBpZgorICAgICAgICBkaWRBY3F1aXJlTG9jayBjaGFuZ2VkIGl0LiBt X2VudHJ5QXRvbWljU3RyaW5nVGFibGUgd2lsbCBoYXZlCisgICAgICAgIGJlZW4gc2V0IGJ5IGRp ZEFjcXVpcmVMb2NrIGlmIGl0IGNoYW5nZWQsIG9yIG51bGxwdHIgaWYgaXQgZGlkbid0LgorICAg ICAgICBUaGlzIHdheSB3ZSBhcmUgc3VyZSB3ZSBhcmUgYmFsYW5jZWQsIHJlZ2FyZGxlc3Mgb2Yg bV92bSBjaGFuZ2VzLgorCiAyMDE0LTA3LTI0ICBQZXl0b24gUmFuZG9scGggIDxwcmFuZG9scGhA YXBwbGUuY29tPgogCiAgICAgICAgIFJlbmFtZSBmZWF0dXJlIGZsYWcgZm9yIGxvbmctcHJlc3Mg Z2VzdHVyZSBvbiBNYWMuICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgIApkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvSlNMb2NrLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pT TG9jay5jcHAKaW5kZXggMjExNGYxNS4uYWY4ZTI3MiAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFT Y3JpcHRDb3JlL3J1bnRpbWUvSlNMb2NrLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUv cnVudGltZS9KU0xvY2suY3BwCkBAIC04Myw2ICs4Myw3IEBAIEpTTG9jazo6SlNMb2NrKFZNKiB2 bSkKICAgICAsIG1fbG9ja0Ryb3BEZXB0aCgwKQogICAgICwgbV9oYXNFeGNsdXNpdmVUaHJlYWQo ZmFsc2UpCiAgICAgLCBtX3ZtKHZtKQorICAgICwgbV9lbnRyeUF0b21pY1N0cmluZ1RhYmxlKG51 bGxwdHIpCiB7CiB9CiAKQEAgLTkzLDcgKzk0LDcgQEAgSlNMb2NrOjp+SlNMb2NrKCkKIHZvaWQg SlNMb2NrOjp3aWxsRGVzdHJveVZNKFZNKiB2bSkKIHsKICAgICBBU1NFUlRfVU5VU0VEKHZtLCBt X3ZtID09IHZtKTsKLSAgICBtX3ZtID0gMDsKKyAgICBtX3ZtID0gbnVsbHB0cjsKIH0KIAogdm9p ZCBKU0xvY2s6OnNldEV4Y2x1c2l2ZVRocmVhZChzdGQ6OnRocmVhZDo6aWQgdGhyZWFkSWQpCkBA IC0xMzksNyArMTQwLDEwIEBAIHZvaWQgSlNMb2NrOjpkaWRBY3F1aXJlTG9jaygpCiAgICAgV1RG VGhyZWFkRGF0YSYgdGhyZWFkRGF0YSA9IHd0ZlRocmVhZERhdGEoKTsKICAgICBtX3ZtLT5zZXRM YXN0U3RhY2tUb3AodGhyZWFkRGF0YS5zYXZlZExhc3RTdGFja1RvcCgpKTsKIAorICAgIEFTU0VS VCghbV9lbnRyeUF0b21pY1N0cmluZ1RhYmxlKTsKICAgICBtX2VudHJ5QXRvbWljU3RyaW5nVGFi bGUgPSB0aHJlYWREYXRhLnNldEN1cnJlbnRBdG9taWNTdHJpbmdUYWJsZShtX3ZtLT5hdG9taWNT dHJpbmdUYWJsZSgpKTsKKyAgICBBU1NFUlQobV9lbnRyeUF0b21pY1N0cmluZ1RhYmxlKTsKKwog ICAgIG1fdm0tPmhlYXAubWFjaGluZVRocmVhZHMoKS5hZGRDdXJyZW50VGhyZWFkKCk7CiB9CiAK QEAgLTE3MCw3ICsxNzQsMTAgQEAgdm9pZCBKU0xvY2s6OndpbGxSZWxlYXNlTG9jaygpCiAgICAg aWYgKG1fdm0pCiAgICAgICAgIG1fdm0tPnNldFN0YWNrUG9pbnRlckF0Vk1FbnRyeShudWxscHRy KTsKIAotICAgIHd0ZlRocmVhZERhdGEoKS5zZXRDdXJyZW50QXRvbWljU3RyaW5nVGFibGUobV9l bnRyeUF0b21pY1N0cmluZ1RhYmxlKTsKKyAgICBpZiAobV9lbnRyeUF0b21pY1N0cmluZ1RhYmxl KSB7CisgICAgICAgIHd0ZlRocmVhZERhdGEoKS5zZXRDdXJyZW50QXRvbWljU3RyaW5nVGFibGUo bV9lbnRyeUF0b21pY1N0cmluZ1RhYmxlKTsKKyAgICAgICAgbV9lbnRyeUF0b21pY1N0cmluZ1Rh YmxlID0gbnVsbHB0cjsKKyAgICB9CiB9CiAKIHZvaWQgSlNMb2NrOjpsb2NrKEV4ZWNTdGF0ZSog ZXhlYykK