35757 2010-03-04 11:26:12 -0800 [v8] Remove wrong assert in GC callback 2010-03-05 19:05:58 -0800 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 dimich dimich abarth antonm levin oldest_to_newest 196151 0 dimich 2010-03-04 11:26:12 -0800 The Workers in debug Chromuim crash when v8 decides to GC. It is a regression caused by http://trac.webkit.org/changeset/50598. It assumed V8 only runs on main thread, while workers in worker process run on a separate worker thread. This code needs more cleanup (there are chunks of unnecessary (although probably benign) code stuck in there since old attempts to run workers on separate threads using v8 Locker. We don't do this anymore so some cleanup is in order. For now, I'll remove assert to fix the crash. 196152 1 50039 dimich 2010-03-04 11:28:50 -0800 Created attachment 50039 Patch. 196307 2 abarth 2010-03-04 19:09:28 -0800 +antonm for an opinion 196339 3 dimich 2010-03-04 21:03:55 -0800 More info: Jian (mostly) and I initially created the "delayed deref" maps because we were trying to use v8 in a renderer process on separate threads with v8 Lockers. That didn't really work because of GC issues and because it essentially serialized JS execution which was bad for UI thread and isn't very efficient. So we abandoned this in favor of one worker per process model, for now. In the Worker process, the worker is running not on a main thread, but on a separate worker thread. V8 is not used on main thread in Worker process. But DOM Objects (like Event) are created and they have wrappers. Most of our worker tests are not long enough to make v8 to GC. But when it happens, it happens on a worker thread. Hence the crash in debug builds that manifests itself as flakiness of worker tests. I'm preparing a cleanup patch to remove all the delayed deref code. It isn't used now and we'll not likely to use it in the future. 196427 4 antonm 2010-03-05 07:59:33 -0800 (In reply to comment #3) > More info: Jian (mostly) and I initially created the "delayed deref" maps > because we were trying to use v8 in a renderer process on separate threads with > v8 Lockers. That didn't really work because of GC issues and because it > essentially serialized JS execution which was bad for UI thread and isn't very > efficient. So we abandoned this in favor of one worker per process model, for > now. > > In the Worker process, the worker is running not on a main thread, but on a > separate worker thread. V8 is not used on main thread in Worker process. But > DOM Objects (like Event) are created and they have wrappers. Most of our worker > tests are not long enough to make v8 to GC. But when it happens, it happens on > a worker thread. Hence the crash in debug builds that manifests itself as > flakiness of worker tests. > > I'm preparing a cleanup patch to remove all the delayed deref code. It isn't > used now and we'll not likely to use it in the future. Dmitry, just to check I understand it correctly. You say that in Worker process V8 never runs on main thread. And I assume that at least currently it always runs on some dedicated thread (otherwise things go hairy). If it's the case, could we introduce notion of V8 thread and keep the ASSERT turning it into ASSERT(v8Thread() == WTF::currentThread())? 196546 5 dimich 2010-03-05 10:47:04 -0800 (In reply to comment #4) > Dmitry, just to check I understand it correctly. > > You say that in Worker process V8 never runs on main thread. And I assume that > at least currently it always runs on some dedicated thread (otherwise things go > hairy). That's exactly right. > If it's the case, could we introduce notion of V8 thread and keep the ASSERT > turning it into ASSERT(v8Thread() == WTF::currentThread())? There is an assert a couple lines below in the same function that checks we are on the same thread as the DOMData object: ASSERT(store->domData()->owningThread() == WTF::currentThread()); If we pull it couple of lines higher, it seems it could serve the purpose here since the important thing here is to check that we are on the same thread as we were when wrappers were created... Updated patch coming. Having a notion of a V8 thread is an interesting idea (something similar to ScritpExecutionContext::isContextThread()). I can add that in a separate patch (since it would involve replacement of various checks for main thread and DOMData::owningThread() in a few places). Also, if we ever get more then one V8 instance in the process (it might happen sooner then a teleporter, which is another secret project :-), then perhaps it should not be a global function... But we can solve this later. 196560 6 antonm 2010-03-05 11:13:02 -0800 I am fine with any version. Just an observation regarding ASSERT(store->domData()->owningThread() == WTF::currentThread()), pedantic one. That doesn't protect as from multithreading---we might be lucky to grab the right object on the right thread, but without proper synchronization it's russian roulette. But anyway, as I said above, I am fine with the change, esp. as you're going to work on this area. 196562 7 50039 dimich 2010-03-05 11:27:04 -0800 Comment on attachment 50039 Patch. Obsoleting the patch for now, another one is comign (I'm trying to add a specific test) 196590 8 50111 dimich 2010-03-05 12:45:56 -0800 Created attachment 50111 Patch with a test. Added test and moved another ASSERT up as per discussion. 196844 9 dimich 2010-03-05 19:05:12 -0800 Landed: http://trac.webkit.org/changeset/55608 196845 10 dimich 2010-03-05 19:05:58 -0800 Removed r+ from last patch so it doesn't show in landing queue. 50039 2010-03-04 11:28:50 -0800 2010-03-05 11:27:04 -0800 Patch. patch.txt text/plain 1206 dimich ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg M2E4NmJhNy4uNWJmNjdiMiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAxMC0wMy0wNCAgRG1pdHJ5IFRpdG92 ICA8ZGltaWNoQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBbdjhdIFJlbW92ZSB3cm9uZyBhc3NlcnQgaW4gR0MgY2FsbGJhY2sK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM1NzU3CisK KyAgICAgICAgVGhpcyBpcyBjb3ZlcmVkIGJ5IGxvbmctcnVubmluZyB3b3JrZXIgdGVzdHMgbGlr ZSB3b3JrZXItY2xvbmVwb3J0Lmh0bWwsIHdoaWNoIGFyZSBjdXJyZW50bHkgZmxha3kgb24gQ2hy b21pdW0uCisKKyAgICAgICAgKiBiaW5kaW5ncy92OC9ET01EYXRhLmg6CisgICAgICAgIChXZWJD b3JlOjpET01EYXRhOjpoYW5kbGVXZWFrT2JqZWN0KToKKwogMjAxMC0wMy0wNCAgUGF2ZWwgRmVs ZG1hbiAgPHBmZWxkbWFuQGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBUaW1v dGh5IEhhdGNoZXIuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2JpbmRpbmdzL3Y4L0RPTURhdGEuaCBi L1dlYkNvcmUvYmluZGluZ3MvdjgvRE9NRGF0YS5oCmluZGV4IDdmYTllN2QuLjJhYzViNjcgMTAw NjQ0Ci0tLSBhL1dlYkNvcmUvYmluZGluZ3MvdjgvRE9NRGF0YS5oCisrKyBiL1dlYkNvcmUvYmlu ZGluZ3MvdjgvRE9NRGF0YS5oCkBAIC05OCw3ICs5OCw2IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsK ICAgICB0ZW1wbGF0ZTx0eXBlbmFtZSBUPgogICAgIHZvaWQgRE9NRGF0YTo6aGFuZGxlV2Vha09i amVjdChET01EYXRhU3RvcmU6OkRPTVdyYXBwZXJNYXBUeXBlIG1hcFR5cGUsIHY4OjpQZXJzaXN0 ZW50PHY4OjpPYmplY3Q+IHY4T2JqZWN0LCBUKiBkb21PYmplY3QpCiAgICAgewotICAgICAgICBB U1NFUlQoV1RGOjppc01haW5UaHJlYWQoKSk7CiAgICAgICAgIERPTURhdGFMaXN0JiBsaXN0ID0g RE9NRGF0YVN0b3JlOjphbGxTdG9yZXMoKTsKICAgICAgICAgZm9yIChzaXplX3QgaSA9IDA7IGkg PCBsaXN0LnNpemUoKTsgKytpKSB7CiAgICAgICAgICAgICBET01EYXRhU3RvcmUqIHN0b3JlID0g bGlzdFtpXTsK 50111 2010-03-05 12:45:56 -0800 2010-03-05 19:05:27 -0800 Patch with a test. patch.txt text/plain 4249 dimich ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBhZmVkYTM5Li45YzdmMjRhIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTAtMDMt MDUgIERtaXRyeSBUaXRvdiAgPGRpbWljaEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW3Y4XSBSZW1vdmUgd3JvbmcgYXNzZXJ0 IGluIEdDIGNhbGxiYWNrCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0zNTc1NworCisgICAgICAgICogZmFzdC93b3JrZXJzL3Jlc291cmNlcy93cmFwcGVy LW1hcC1nYy5qczogQWRkZWQuCisgICAgICAgICogZmFzdC93b3JrZXJzL3dyYXBwZXItbWFwLWdj Lmh0bWw6IEFkZGVkLgorCiAyMDEwLTAzLTA1ICBFcmljIENhcmxzb24gIDxlcmljLmNhcmxzb25A YXBwbGUuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIGJ1aWxkIGZpeC4KZGlmZiAtLWdpdCBh L0xheW91dFRlc3RzL2Zhc3Qvd29ya2Vycy9yZXNvdXJjZXMvd3JhcHBlci1tYXAtZ2MuanMgYi9M YXlvdXRUZXN0cy9mYXN0L3dvcmtlcnMvcmVzb3VyY2VzL3dyYXBwZXItbWFwLWdjLmpzCm5ldyBm aWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAuLmQyZjU5ZmYKLS0tIC9kZXYvbnVsbAorKysg Yi9MYXlvdXRUZXN0cy9mYXN0L3dvcmtlcnMvcmVzb3VyY2VzL3dyYXBwZXItbWFwLWdjLmpzCkBA IC0wLDAgKzEsNyBAQAorb25tZXNzYWdlID0gZnVuY3Rpb24oZXZlbnQpIHsKKyAgICAvLyBUaGlz IHF1aWNrbHkgY3JlYXRlcyBhIG5lZWQgZm9yIEdDLgorICAgIHBvc3RNZXNzYWdlKGV2ZW50LmRh dGEgKyBldmVudC5kYXRhKTsKK30KKworLy8gVHJpZ2dlciB0aGUgcGluZy1wb25nLgorcG9zdE1l c3NhZ2UoImthYm9vbT8iKTsKXCBObyBuZXdsaW5lIGF0IGVuZCBvZiBmaWxlCmRpZmYgLS1naXQg YS9MYXlvdXRUZXN0cy9mYXN0L3dvcmtlcnMvd3JhcHBlci1tYXAtZ2MuaHRtbCBiL0xheW91dFRl c3RzL2Zhc3Qvd29ya2Vycy93cmFwcGVyLW1hcC1nYy5odG1sCm5ldyBmaWxlIG1vZGUgMTAwNjQ0 CmluZGV4IDAwMDAwMDAuLjg0YTFjNjUKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9m YXN0L3dvcmtlcnMvd3JhcHBlci1tYXAtZ2MuaHRtbApAQCAtMCwwICsxLDMyIEBACis8c2NyaXB0 PgorZnVuY3Rpb24gdGVzdCgpCit7CisgICAgaWYgKHdpbmRvdy5sYXlvdXRUZXN0Q29udHJvbGxl cikgeworICAgICAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5kdW1wQXNUZXh0KCk7CisgICAgICAg IGxheW91dFRlc3RDb250cm9sbGVyLndhaXRVbnRpbERvbmUoKTsKKyAgICB9CisgICAgCisgICAg dmFyIGNvdW50ZXIgPSAwOworICAgIHZhciB3b3JrZXIgPSBuZXcgV29ya2VyKCJyZXNvdXJjZXMv c2VhLW9mLXdyYXBwZXJzLWdjLmpzIik7CisgICAgCisgICAgLy8gUG9zdCB0aGUgbWVzc2FnZSBm cm9tIHdvcmtlciBiYWNrIHRvIHRoZSB3b3JrZXIuCisgICAgLy8gVGhpcyB3aWxsIGZpcmUgYW5v dGhlciBvbm1lc3NhZ2UgZXZlbnQgaW4gdGhlIFdvcmtlciBhbmQgYWxsb2NhdGUgbW9yZSBkYXRh LgorICAgIHdvcmtlci5vbm1lc3NhZ2UgPSBmdW5jdGlvbihldnQpCisgICAgeworICAgICAgICB3 b3JrZXIucG9zdE1lc3NhZ2UoZXZ0LmRhdGEpOworICAgICAgICAvLyBUaGlzIGFwcGVhcnMgdG8g YmUgZW5vdWdoIHRvIHJlbGlhYmx5IHRyaWdnZXIgR0MgaW4gYSBXb3JrZXIgKGFib3V0IDYwTWIg c3RyaW5ncyBhbGxvY2F0ZWQpLgorICAgICAgICBpZiAoKytjb3VudGVyID4gMjEpIHsKKyAgICAg ICAgICAgIGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKCJyZXN1bHQiKS5pbm5lclRleHQgPSAiUEFT UyI7CisgICAgICAgICAgICB3b3JrZXIudGVybWluYXRlKCk7CisgICAgICAgICAgICBpZiAod2lu ZG93LmxheW91dFRlc3RDb250cm9sbGVyKSB7CisgICAgICAgICAgICAgICAgbGF5b3V0VGVzdENv bnRyb2xsZXIubm90aWZ5RG9uZSgpOworICAgICAgICAgICAgfQorICAgICAgICB9CisgICAgfQor fQorPC9zY3JpcHQ+Cis8Ym9keSBvbmxvYWQ9dGVzdCgpPgorPHA+VGhpcyB0ZXN0IHRyaWVzIHRv IGNhdXNlIEdDIGluIFdvcmtlciBjb250ZXh0LiBJdCBhbHNvIGZpcmVzIGV2ZW50cyBpbiB0aGUg V29ya2VyLCB3aGljaCBhbGxvY2F0ZXMgSlMgRE9NIFdyYXBwZXJzIGZvciBFdmVudCBvYmplY3Qu IEFzIGEgcmVzdWx0IG9mIEdDLCB0aGUgbWFwcyB0aGF0IG1hcCB3cmFwcGVycyB0byBET00gT2Jq ZWN0cyB3aWxsIGJlIGNsZWFuZWQgdXAuIFRlc3Qgc3VjY2VlZHMgaWYgaXQgZG9lcyBub3QgY3Jh c2guPC9wPgorPHByZSBpZD0icmVzdWx0Ij48L3ByZT4KKzwvYm9keT4KKzwvaHRtbD4KZGlmZiAt LWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDAxZmEz My4uZDVlMTQ3ZSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2ViQ29yZS9D aGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAxMC0wMy0wNSAgRG1pdHJ5IFRpdG92ICA8ZGlt aWNoQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBbdjhdIFJlbW92ZSB3cm9uZyBhc3NlcnQgaW4gR0MgY2FsbGJhY2sKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM1NzU3CisKKyAgICAg ICAgVGVzdDogZmFzdC93b3JrZXJzL3dyYXBwZXItbWFwLWdjLmh0bWwKKworICAgICAgICAqIGJp bmRpbmdzL3Y4L0RPTURhdGEuaDoKKyAgICAgICAgKFdlYkNvcmU6OkRPTURhdGE6OmhhbmRsZVdl YWtPYmplY3QpOiByZW1vdmUgQVNTRVJUKGlzTWFpblRocmVhZCgpKSwgbW92ZSBhbm90aGVyIGFz c2VydCB1cCB0byB2ZXJpZnkgd2UgYXJlIG9uIHRoZSByaWdodCB0aHJlYWQuCisKIDIwMTAtMDMt MDQgIEVyaWMgQ2FybHNvbiAgPGVyaWMuY2FybHNvbkBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2 aWV3ZWQgYnkgQWRlbGUgUGV0ZXJzb24uCmRpZmYgLS1naXQgYS9XZWJDb3JlL2JpbmRpbmdzL3Y4 L0RPTURhdGEuaCBiL1dlYkNvcmUvYmluZGluZ3MvdjgvRE9NRGF0YS5oCmluZGV4IDdmYTllN2Qu LjNjYTI5YmYgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvYmluZGluZ3MvdjgvRE9NRGF0YS5oCisrKyBi L1dlYkNvcmUvYmluZGluZ3MvdjgvRE9NRGF0YS5oCkBAIC05OCwxNyArOTgsMTQgQEAgbmFtZXNw YWNlIFdlYkNvcmUgewogICAgIHRlbXBsYXRlPHR5cGVuYW1lIFQ+CiAgICAgdm9pZCBET01EYXRh OjpoYW5kbGVXZWFrT2JqZWN0KERPTURhdGFTdG9yZTo6RE9NV3JhcHBlck1hcFR5cGUgbWFwVHlw ZSwgdjg6OlBlcnNpc3RlbnQ8djg6Ok9iamVjdD4gdjhPYmplY3QsIFQqIGRvbU9iamVjdCkKICAg ICB7Ci0gICAgICAgIEFTU0VSVChXVEY6OmlzTWFpblRocmVhZCgpKTsKICAgICAgICAgRE9NRGF0 YUxpc3QmIGxpc3QgPSBET01EYXRhU3RvcmU6OmFsbFN0b3JlcygpOwogICAgICAgICBmb3IgKHNp emVfdCBpID0gMDsgaSA8IGxpc3Quc2l6ZSgpOyArK2kpIHsKICAgICAgICAgICAgIERPTURhdGFT dG9yZSogc3RvcmUgPSBsaXN0W2ldOworICAgICAgICAgICAgQVNTRVJUKHN0b3JlLT5kb21EYXRh KCktPm93bmluZ1RocmVhZCgpID09IFdURjo6Y3VycmVudFRocmVhZCgpKTsKIAogICAgICAgICAg ICAgRE9NRGF0YVN0b3JlOjpJbnRlcm5hbERPTVdyYXBwZXJNYXA8VD4qIGRvbU1hcCA9IHN0YXRp Y19jYXN0PERPTURhdGFTdG9yZTo6SW50ZXJuYWxET01XcmFwcGVyTWFwPFQ+Kj4oc3RvcmUtPmdl dERPTVdyYXBwZXJNYXAobWFwVHlwZSkpOwotCi0gICAgICAgICAgICBpZiAoZG9tTWFwLT5yZW1v dmVJZlByZXNlbnQoZG9tT2JqZWN0LCB2OE9iamVjdCkpIHsKLSAgICAgICAgICAgICAgICBBU1NF UlQoc3RvcmUtPmRvbURhdGEoKS0+b3duaW5nVGhyZWFkKCkgPT0gV1RGOjpjdXJyZW50VGhyZWFk KCkpOworICAgICAgICAgICAgaWYgKGRvbU1hcC0+cmVtb3ZlSWZQcmVzZW50KGRvbU9iamVjdCwg djhPYmplY3QpKQogICAgICAgICAgICAgICAgIHN0b3JlLT5kb21EYXRhKCktPmRlcmVmT2JqZWN0 KFY4RE9NV3JhcHBlcjo6ZG9tV3JhcHBlclR5cGUodjhPYmplY3QpLCBkb21PYmplY3QpOwotICAg ICAgICAgICAgfQogICAgICAgICB9CiAgICAgfQogCg==