149027 2015-09-09 18:48:03 -0700 JSInternalPromiseDeferred should inherit JSPromiseDeferred 2015-09-10 11:05:08 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ysuzuki ysuzuki darin fpizlo ggaren mark.lam msaboff saam oldest_to_newest 1124798 0 ysuzuki 2015-09-09 18:48:03 -0700 JSInternalPromiseDeferred should inherit JSPromiseDeferred 1124799 1 260902 ysuzuki 2015-09-09 18:50:46 -0700 Created attachment 260902 Patch 1124800 2 260902 ysuzuki 2015-09-09 18:52:27 -0700 Comment on attachment 260902 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260902&action=review > Source/JavaScriptCore/runtime/JSInternalPromiseDeferred.cpp:41 > +const ClassInfo JSInternalPromiseDeferred::s_info = { "JSInternalPromiseDeferred", &Base::s_info, nullptr, CREATE_METHOD_TABLE(JSInternalPromiseDeferred) }; While JSPromiseDeferred does not inherit anything, JSInternalPromiseDeferred inherits JSPromiseDeferred. 1124829 3 ysuzuki 2015-09-09 21:18:50 -0700 *** Bug 149028 has been marked as a duplicate of this bug. *** 1124830 4 ysuzuki 2015-09-09 21:33:38 -0700 When visitChildren is called, since we perform jsCast<JSPromiseDeferred*>(cell), it causes the crash. But it is difficult to reproduce this situation stably in the JSC shell because JSInternalPromiseDeferred is not held by the reachable objects. The situation is the following, (1): When JSInternalPromiseDeferred is created and resides on the C stack, (2): GC starts (3): GC calls visitChildren of JSInternalPromiseDeferred object (4): perform jsCast<JSPromiseDeferred*>(cell) and crash. So this patch does not contain the tests. 1124834 5 ysuzuki 2015-09-09 21:43:18 -0700 These faults are figured out during the ES6 Module WebCore integration. 1124902 6 260902 darin 2015-09-10 08:45:10 -0700 Comment on attachment 260902 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260902&action=review >> Source/JavaScriptCore/runtime/JSInternalPromiseDeferred.cpp:41 >> +const ClassInfo JSInternalPromiseDeferred::s_info = { "JSInternalPromiseDeferred", &Base::s_info, nullptr, CREATE_METHOD_TABLE(JSInternalPromiseDeferred) }; > > While JSPromiseDeferred does not inherit anything, JSInternalPromiseDeferred inherits JSPromiseDeferred. I am confused about test coverage. Is there a reason we don’t have a test that demonstrates the problem this lack of inheritance causes? 1124945 7 ysuzuki 2015-09-10 10:56:13 -0700 (In reply to comment #6) > Comment on attachment 260902 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=260902&action=review > > >> Source/JavaScriptCore/runtime/JSInternalPromiseDeferred.cpp:41 > >> +const ClassInfo JSInternalPromiseDeferred::s_info = { "JSInternalPromiseDeferred", &Base::s_info, nullptr, CREATE_METHOD_TABLE(JSInternalPromiseDeferred) }; > > > > While JSPromiseDeferred does not inherit anything, JSInternalPromiseDeferred inherits JSPromiseDeferred. > > I am confused about test coverage. Is there a reason we don’t have a test > that demonstrates the problem this lack of inheritance causes? This case is a little bit edge case I think. For the usual classes, I think we will see the assertion failure with the existing test cases. But, JSInternalPromiseDeferred does not cause it before prototyping Module Loader in WebCore. This is because, 1. This fault causes error when executing JSInternalPromiseDeferred's visitChildren. Since it utilizes JSPromiseDeferred's one, this function attempt to perform jsCast<JSPromiseDeferred*>(cell). So, if we forgot to inherit JSPromiseDeferred::classInfo, it causes assertion failure. 2. But visitChildren is only called when the target object is live and GC occurs 3. JSInternalPromiseDeferred is only used in C++ runtime layer now. And it will be discarded relatively soon. 4. So, the problem only occurs when JSInternalPromiseDeferred is live in the C stack and GC occurs. 5. In addition to this limited situation, JSInternalPromiseDeferred is now only used for the module loader. So it only used when loading modules. The above reasons make testing harder. Hopefully, I this we will cover this bug. I found this issue while prototyping the module loader in WebCore, this is because, 1. In JSC shell module loader, we fetch the module source code from the local file system. So the source code is immediately fetched. In this case, we don't need to hold JSInternalPromiseDeferred because we can resolve the promise with the fetched source immediately. So JSInternalPromiseDeferred will be discarded soon. 2. But in WebCore side, the source code will be fetched over the network. So until we fetched the source code, we need to strongly hold the JSInternalPromiseDeferred instance to resolve the promise when the source is fetched. 3. So, we keep JSInternalPromiseDeferred alive for a while. If the GC occurs during this, we can see this assertion failure. So I think we can cover the problem solved here when adding the tests for the WebCore integrated module loader (I'm now implementing it). 1124949 8 ysuzuki 2015-09-10 11:05:08 -0700 Committed r189577: <http://trac.webkit.org/changeset/189577> 260902 2015-09-09 18:50:46 -0700 2015-09-10 08:45:10 -0700 Patch bug-149027-20150909185039.patch text/plain 1684 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMTg5NTYzCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAz ZDc3MDI3NWI4M2M3ZTI0OWZjMTYzYzkwMmVkODg2NDQwM2Y0M2U3Li4xZGFhYzE0YzBiYzAzYTYx MDdjYTI0ZmZjOWI0NWQ2MWUxMmU1NDk4IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNSBAQAorMjAxNS0wOS0wOSAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWls LmNvbT4KKworICAgICAgICBKU0ludGVybmFsUHJvbWlzZURlZmVycmVkIHNob3VsZCBpbmhlcml0 IEpTUHJvbWlzZURlZmVycmVkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xNDkwMjcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBKU0ludGVybmFsUHJvbWlzZURlZmVycmVkIGlzIGNvbnN0cnVjdGVkIGJ5IHVz aW5nIEpTUHJvbWlzZURlZmVycmVkIGltcGxlbWVudGF0aW9uLgorICAgICAgICBTbyB0aGUgY2xh c3MgaW5mbyBvZiBKU0ludGVybmFsUHJvbWlzZURlZmVycmVkIHNob3VsZCBpbmhlcml0IEpTUHJv bWlzZURlZmVycmVkLgorCisgICAgICAgICogcnVudGltZS9KU0ludGVybmFsUHJvbWlzZURlZmVy cmVkLmNwcDoKKwogMjAxNS0wOS0wOSAgU3Vrb2xzYWsgU2Frc2h1d29uZyAgPHN1a29sc2FrQGdt YWlsLmNvbT4KIAogICAgICAgICBJbXBsZW1lbnQgaW50ZXJuYWwgY2FsbHMgaW4gV2ViQXNzZW1i bHkKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTSW50ZXJuYWxQ cm9taXNlRGVmZXJyZWQuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNJbnRl cm5hbFByb21pc2VEZWZlcnJlZC5jcHAKaW5kZXggNGU2N2E4YzdkOTE3NDhkOTVjYjg5NjRmMGM0 ODg0NDllYWYwNzZkMS4uZmVmZGVjZGYwM2I2NGU2ZGYzYjQxMDBkNjFiNmE0NDdjZmFmMmQ4YiAx MDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNJbnRlcm5hbFByb21p c2VEZWZlcnJlZC5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNJbnRl cm5hbFByb21pc2VEZWZlcnJlZC5jcHAKQEAgLTM4LDcgKzM4LDcgQEAKIAogbmFtZXNwYWNlIEpT QyB7CiAKLWNvbnN0IENsYXNzSW5mbyBKU0ludGVybmFsUHJvbWlzZURlZmVycmVkOjpzX2luZm8g PSB7ICJKU0ludGVybmFsUHJvbWlzZURlZmVycmVkIiwgbnVsbHB0ciwgbnVsbHB0ciwgQ1JFQVRF X01FVEhPRF9UQUJMRShKU0ludGVybmFsUHJvbWlzZURlZmVycmVkKSB9OworY29uc3QgQ2xhc3NJ bmZvIEpTSW50ZXJuYWxQcm9taXNlRGVmZXJyZWQ6OnNfaW5mbyA9IHsgIkpTSW50ZXJuYWxQcm9t aXNlRGVmZXJyZWQiLCAmQmFzZTo6c19pbmZvLCBudWxscHRyLCBDUkVBVEVfTUVUSE9EX1RBQkxF KEpTSW50ZXJuYWxQcm9taXNlRGVmZXJyZWQpIH07CiAKIEpTSW50ZXJuYWxQcm9taXNlRGVmZXJy ZWQqIEpTSW50ZXJuYWxQcm9taXNlRGVmZXJyZWQ6OmNyZWF0ZShFeGVjU3RhdGUqIGV4ZWMsIEpT R2xvYmFsT2JqZWN0KiBnbG9iYWxPYmplY3QpCiB7Cg==