128840 2014-02-14 13:46:55 -0800 ASSERT(isValidAllocation(bytes)) when ObjC API creates custom errors 2014-02-14 14:39:49 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 joepeck mhahnenberg ggaren joepeck mhahnenberg oldest_to_newest 980952 0 joepeck 2014-02-14 13:46:55 -0800 * TEST: JSContext *context = [[[JSContext alloc] init] autorelease]; [[JSValue valueWithInt32:42 inContext:context] toDictionary]; * ASSERT ASSERTION FAILED: isValidAllocation(bytes) /Volumes/Data/Code/safari/OpenSource/Source/JavaScriptCore/heap/Heap.h(467) : void *JSC::Heap::allocateWithoutDestructor(size_t) 1 0x10a94ce50 WTFCrash 2 0x10a1ce54b JSC::Heap::allocateWithoutDestructor(unsigned long) 3 0x10a4ea107 void* JSC::allocateCell<JSC::ErrorInstance>(JSC::Heap&, unsigned long) 4 0x10a4e986f void* JSC::allocateCell<JSC::ErrorInstance>(JSC::Heap&) 5 0x10a4e8ffb JSC::ErrorInstance::create(JSC::VM&, JSC::Structure*, WTF::String const&, WTF::Vector<JSC::StackFrame, 0ul, WTF::CrashOnOverflow>) 6 0x10a4e88f7 JSC::createTypeError(JSC::JSGlobalObject*, WTF::String const&) 7 0x10a4e8b35 JSC::createTypeError(JSC::ExecState*, WTF::String const&) 8 0x10a6d5af5 valueToDictionary(OpaqueJSContext*, OpaqueJSValue const*, OpaqueJSValue const**) 9 0x10a6d5921 -[JSValue toDictionary] 10 0x10a1b5f04 main 11 0x7fff8cbbf5f1 start Specifically, vm->identifierTable != wtfThreadData().currentIdentifierTable. My guess is that the ObjC API has to grab the APIEntryShim before it calls into JSC (via JSC::createTypeError). Most of the ObjC API uses the C API which already does this implicitly. 980953 1 joepeck 2014-02-14 13:48:01 -0800 There are a number of create calls throughout the ObjC API. They will all need to be audited: shell> ack 'create.*?Error' API/*.mm API/JSValue.mm 784: *exception = toRef(JSC::createTypeError(toJS(context), "Cannot convert primitive to NSArray")); 800: *exception = toRef(JSC::createTypeError(toJS(context), "Cannot convert primitive to NSDictionary")); API/ObjCCallbackFunction.mm 132: *exception = toRef(JSC::createTypeError(toJS(contextRef), "Argument does not match Objective-C Class")); 456: // (2) We're calling some JSC internals that require us to be on the 'inside' - e.g. createTypeError. 499: *exception = toRef(JSC::createTypeError(toJS(contextRef), "Objective-C blocks called as constructors must return an object.")); 565: *exception = toRef(JSC::createTypeError(toJS(contextRef), "self type check failed for Objective-C instance method")); 575: *exception = toRef(JSC::createTypeError(toJS(contextRef), "self type check failed for Objective-C instance method")); 980968 2 ggaren 2014-02-14 14:06:19 -0800 > My guess is that the ObjC API has to grab the APIEntryShim before it calls into JSC (via JSC::createTypeError). Right. 980970 3 224250 mhahnenberg 2014-02-14 14:09:12 -0800 Created attachment 224250 Patch 980971 4 mhahnenberg 2014-02-14 14:09:52 -0800 (In reply to comment #1) > There are a number of create calls throughout the ObjC API. They will all need to be audited: > > shell> ack 'create.*?Error' API/*.mm > API/JSValue.mm > 784: *exception = toRef(JSC::createTypeError(toJS(context), "Cannot convert primitive to NSArray")); > 800: *exception = toRef(JSC::createTypeError(toJS(context), "Cannot convert primitive to NSDictionary")); > > API/ObjCCallbackFunction.mm > 132: *exception = toRef(JSC::createTypeError(toJS(contextRef), "Argument does not match Objective-C Class")); > 456: // (2) We're calling some JSC internals that require us to be on the 'inside' - e.g. createTypeError. > 499: *exception = toRef(JSC::createTypeError(toJS(contextRef), "Objective-C blocks called as constructors must return an object.")); > 565: *exception = toRef(JSC::createTypeError(toJS(contextRef), "self type check failed for Objective-C instance method")); > 575: *exception = toRef(JSC::createTypeError(toJS(contextRef), "self type check failed for Objective-C instance method")); I audited the other call sites and other than these two they are all in places where we already hold the API lock. 980990 5 224250 joepeck 2014-02-14 14:23:44 -0800 Comment on attachment 224250 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224250&action=review r=me > Source/JavaScriptCore/API/JSValue.mm:785 > *exception = toRef(JSC::createTypeError(toJS(context), "Cannot convert primitive to NSArray")); Nit: You could ASCIILiteral(...) the string being passed to createTypeError, since it is a string literal turning into a WTF::String. Is the lock needed by the "tryUnwrapObjcObject" call at the top of these functions? Otherwise this is identical to the local fix I had. 980994 6 mhahnenberg 2014-02-14 14:28:18 -0800 (In reply to comment #5) > (From update of attachment 224250 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224250&action=review > > r=me > > > Source/JavaScriptCore/API/JSValue.mm:785 > > *exception = toRef(JSC::createTypeError(toJS(context), "Cannot convert primitive to NSArray")); > > Nit: You could ASCIILiteral(...) the string being passed to createTypeError, since it is a string literal turning into a WTF::String. > Will do. > Is the lock needed by the "tryUnwrapObjcObject" call at the top of these functions? > I don't think so. We don't create any objects in it, and the object is always reachable from the stack. If I missed something and we do need to take the shim for things that happen inside tryUnwrapObjcObject then the shim should probably go in there instead of out here. 981001 7 mhahnenberg 2014-02-14 14:39:49 -0800 Committed r164136: <http://trac.webkit.org/changeset/164136> 224250 2014-02-14 14:09:12 -0800 2014-02-14 14:23:43 -0800 Patch bug-128840-20140214140905.patch text/plain 2732 mhahnenberg SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTY0MTMyKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE0LTAyLTE0ICBNYXJrIEhhaG5lbmJlcmcgIDxtaGFobmVuYmVyZ0BhcHBsZS5jb20+CisK KyAgICAgICAgQVNTRVJUKGlzVmFsaWRBbGxvY2F0aW9uKGJ5dGVzKSkgd2hlbiBPYmpDIEFQSSBj cmVhdGVzIGN1c3RvbSBlcnJvcnMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTEyODg0MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIFdlIG5lZWQgdG8gYWRkIEFQSUVudHJ5U2hpbXMgYXJvdW5kIHBsYWNlcyB3 aGVyZSB3ZSBhbGxvY2F0ZSBlcnJvcnMgaW4gSlNDLgorCisgICAgICAgICogQVBJL0pTVmFsdWUu bW06CisgICAgICAgICh2YWx1ZVRvQXJyYXkpOgorICAgICAgICAodmFsdWVUb0RpY3Rpb25hcnkp OgorICAgICAgICAqIEFQSS90ZXN0cy90ZXN0YXBpLm1tOgorCiAyMDE0LTAyLTE0ICBNYXJrIEhh aG5lbmJlcmcgIDxtaGFobmVuYmVyZ0BhcHBsZS5jb20+CiAKICAgICAgICAgQmFzZWxpbmUgSklU IHNob3VsZCBoYXZlIGEgZmFzdCBwYXRoIHRvIGJ5cGFzcyB0aGUgd3JpdGUgYmFycmllciBvbiBv cF9lbnRlcgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL0FQSS9KU1ZhbHVlLm1tCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvSlNWYWx1ZS5tbQkocmV2aXNpb24g MTY0MTMxKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL0FQSS9KU1ZhbHVlLm1tCSh3b3JraW5n IGNvcHkpCkBAIC03ODAsNiArNzgwLDcgQEAgaWQgdmFsdWVUb0FycmF5KEpTR2xvYmFsQ29udGV4 dFJlZiBjb250ZQogICAgIGlmIChKU1ZhbHVlSXNPYmplY3QoY29udGV4dCwgdmFsdWUpKQogICAg ICAgICByZXR1cm4gY29udGFpbmVyVmFsdWVUb09iamVjdChjb250ZXh0LCAoSlNDb250YWluZXJD b252ZXJ0b3I6OlRhc2speyB2YWx1ZSwgW05TTXV0YWJsZUFycmF5IGFycmF5XSwgQ29udGFpbmVy QXJyYXl9KTsKIAorICAgIEpTQzo6QVBJRW50cnlTaGltIHNoaW0odG9KUyhjb250ZXh0KSk7CiAg ICAgaWYgKCEoSlNWYWx1ZUlzTnVsbChjb250ZXh0LCB2YWx1ZSkgfHwgSlNWYWx1ZUlzVW5kZWZp bmVkKGNvbnRleHQsIHZhbHVlKSkpCiAgICAgICAgICpleGNlcHRpb24gPSB0b1JlZihKU0M6OmNy ZWF0ZVR5cGVFcnJvcih0b0pTKGNvbnRleHQpLCAiQ2Fubm90IGNvbnZlcnQgcHJpbWl0aXZlIHRv IE5TQXJyYXkiKSk7CiAgICAgcmV0dXJuIG5pbDsKQEAgLTc5Niw2ICs3OTcsNyBAQCBpZCB2YWx1 ZVRvRGljdGlvbmFyeShKU0dsb2JhbENvbnRleHRSZWYgCiAgICAgaWYgKEpTVmFsdWVJc09iamVj dChjb250ZXh0LCB2YWx1ZSkpCiAgICAgICAgIHJldHVybiBjb250YWluZXJWYWx1ZVRvT2JqZWN0 KGNvbnRleHQsIChKU0NvbnRhaW5lckNvbnZlcnRvcjo6VGFzayl7IHZhbHVlLCBbTlNNdXRhYmxl RGljdGlvbmFyeSBkaWN0aW9uYXJ5XSwgQ29udGFpbmVyRGljdGlvbmFyeX0pOwogCisgICAgSlND OjpBUElFbnRyeVNoaW0gc2hpbSh0b0pTKGNvbnRleHQpKTsKICAgICBpZiAoIShKU1ZhbHVlSXNO dWxsKGNvbnRleHQsIHZhbHVlKSB8fCBKU1ZhbHVlSXNVbmRlZmluZWQoY29udGV4dCwgdmFsdWUp KSkKICAgICAgICAgKmV4Y2VwdGlvbiA9IHRvUmVmKEpTQzo6Y3JlYXRlVHlwZUVycm9yKHRvSlMo Y29udGV4dCksICJDYW5ub3QgY29udmVydCBwcmltaXRpdmUgdG8gTlNEaWN0aW9uYXJ5IikpOwog ICAgIHJldHVybiBuaWw7CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL3Rlc3RzL3Rl c3RhcGkubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL0FQSS90ZXN0cy90 ZXN0YXBpLm1tCShyZXZpc2lvbiAxNjQxMzEpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJ L3Rlc3RzL3Rlc3RhcGkubW0JKHdvcmtpbmcgY29weSkKQEAgLTEyNzIsNiArMTI3MiwxMiBAQCB2 b2lkIHRlc3RPYmplY3RpdmVDQVBJKCkKICAgICAgICAgY2hlY2tSZXN1bHQoQCJtYWtlT2JqZWN0 KCkgaW5zdGFuY2VvZiBVbmV4cG9ydGVkT2JqZWN0IiwgW3Jlc3VsdCBpc0Jvb2xlYW5dICYmIFty ZXN1bHQgdG9Cb29sXSk7CiAgICAgfQogCisgICAgQGF1dG9yZWxlYXNlcG9vbCB7CisgICAgICAg IEpTQ29udGV4dCAqY29udGV4dCA9IFtbSlNDb250ZXh0IGFsbG9jXSBpbml0XTsKKyAgICAgICAg W1tKU1ZhbHVlIHZhbHVlV2l0aEludDMyOjQyIGluQ29udGV4dDpjb250ZXh0XSB0b0RpY3Rpb25h cnldOworICAgICAgICBbW0pTVmFsdWUgdmFsdWVXaXRoSW50MzI6NDIgaW5Db250ZXh0OmNvbnRl eHRdIHRvQXJyYXldOworICAgIH0KKwogICAgIGN1cnJlbnRUaGlzSW5zaWRlQmxvY2tHZXR0ZXJU ZXN0KCk7CiAgICAgcnVuRGF0ZVRlc3RzKCk7CiAgICAgcnVuSlNFeHBvcnRUZXN0cygpOwo=