79222 2012-02-22 04:12:23 -0800 [chromium] createObjectURL(Blob) throws 'Illegal invocation' error when MEDIA_STREAM is disabled. 2012-04-02 09:58:30 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 66687 1 zhenghao zhenghao abarth ager arv haraken japhet kaustubh.ra webkit.review.bot oldest_to_newest 561834 0 zhenghao 2012-02-22 04:12:23 -0800 All chromium platforms except Android enable both BLOB and MEDIA_STREAM, so the error is not seen normally. However, if you turn off MediaStream in features.gypi and build DRT, all tests related to createObjectURL(Blob) would throw 'Illegal invocation' error. I'm not sure whether other ports have this error, so classify it under chromium. I don't think it's related to MediaStream. Probably caused by https://bugs.webkit.org/show_bug.cgi?id=74386 . 561835 1 zhenghao 2012-02-22 04:14:14 -0800 Sorry, forgot to mention build without MEDIA_STREAM is broken. You need to apply patch from https://bugs.webkit.org/show_bug.cgi?id=79214 to build. 561838 2 zhenghao 2012-02-22 04:15:20 -0800 One example test you can try is fast/files/create-blob-url-crash.html . 561848 3 kaustubh.ra 2012-02-22 04:25:33 -0800 I had a look in this issue. As far as #ifdefs are concerned for MEDIA_STREAM, they all are proper. Will try to debug this. 563014 4 zhenghao 2012-02-23 03:58:05 -0800 The problem may be caused by the method is changed to static. I'm confused about the static semantics of DOMURL. Let's see Screen.idl. Of course, there is only a single instance of Screen, and we still don't add static to its attributes/methods. And in DOMWindow.idl, it's defined as: attribute [Replaceable] Screen screen; However, for DOMURL, it's defined as: attribute [Conditional=BLOB] DOMURLConstructor webkitURL; And it's much like other new-able elements like: attribute EventConstructor Event; Now that all methods of DOMURL are static, we can treat it as other singleton classes like Screen, Console, etc. Of course, defining it as static is also fine. But obviously something is wrong. I'm not familiar with idl, but webkitURL is defined as a constructor. Is it right to call static methods on a constructor, rather than the class? In debugging, I found the code when the error happens (builtins.cc HandleApiCallHelper()): Object* raw_holder = TypeCheck(heap, args.length(), &args[0], fun_data); // <--- TypeCheck failed. if (raw_holder->IsNull()) { // This function cannot be called with the given receiver. Abort! Handle<Object> obj = isolate->factory()->NewTypeError( "illegal_invocation", HandleVector(&function, 1)); return isolate->Throw(*obj); } 563018 5 zhenghao 2012-02-23 04:07:59 -0800 If there are 2 createBlobURL methods, the generated code is: desc->Set(v8::String::New("createObjectURL"), v8::FunctionTemplate::New(DOMURLInternal::createObjectURLCallback, v8::Handle<v8::Value>(), v8::Local<v8::Signature>())); desc->Set(v8::String::New("revokeObjectURL"), v8::FunctionTemplate::New(DOMURLInternal::revokeObjectURLCallback, v8::Handle<v8::Value>(), v8::Local<v8::Signature>())); If there is only 1 createBlobURL method (after MEDIA_STREAM is disabled), the code is: // Custom Signature 'createObjectURL' const int createObjectURLArgc = 1; v8::Handle<v8::FunctionTemplate> createObjectURLArgv[createObjectURLArgc] = { V8Blob::GetRawTemplate() }; v8::Handle<v8::Signature> createObjectURLSignature = v8::Signature::New(desc, createObjectURLArgc, createObjectURLArgv); desc->Set(v8::String::New("createObjectURL"), v8::FunctionTemplate::New(DOMURLInternal::createObjectURLCallback, v8::Handle<v8::Value>(), createObjectURLSignature)); desc->Set(v8::String::New("revokeObjectURL"), v8::FunctionTemplate::New(DOMURLInternal::revokeObjectURLCallback, v8::Handle<v8::Value>(), v8::Local<v8::Signature>())); It appears to me that signature check is only executed when one method is defined. Not sure if it's related to the bug. 563146 6 arv 2012-02-23 09:07:07 -0800 (In reply to comment #4) > The problem may be caused by the method is changed to static. I'm confused about the static semantics of DOMURL. Let's see Screen.idl. Of course, there is only a single instance of Screen, and we still don't add static to its attributes/methods. And in DOMWindow.idl, it's defined as: > attribute [Replaceable] Screen screen; > However, for DOMURL, it's defined as: > attribute [Conditional=BLOB] DOMURLConstructor webkitURL; > And it's much like other new-able elements like: > attribute EventConstructor Event; URL is going to be constructable real soon. See 74385 > Now that all methods of DOMURL are static, we can treat it as other singleton classes like Screen, Console, etc. No, screen is just an object (instanceof Object). URL is a constructor (instanceof Function). > Of course, defining it as static is also fine. But obviously something is wrong. I'm not familiar with idl, but webkitURL is defined as a constructor. Is it right to call static methods on a constructor, rather than the class? In debugging, I found the code when the error happens (builtins.cc HandleApiCallHelper()): > > Object* raw_holder = TypeCheck(heap, args.length(), &args[0], fun_data); // <--- TypeCheck failed. > > if (raw_holder->IsNull()) { > // This function cannot be called with the given receiver. Abort! > Handle<Object> obj = > isolate->factory()->NewTypeError( > "illegal_invocation", HandleVector(&function, 1)); > return isolate->Throw(*obj); > } This does sound like a bug. More in next reply. 563148 7 arv 2012-02-23 09:08:05 -0800 (In reply to comment #5) > It appears to me that signature check is only executed when one method is defined. Not sure if it's related to the bug. If there are overloaded functions then the bindings check which one to call based on the type check. If there is only one, the same type check is not required. 563813 8 zhenghao 2012-02-23 18:43:48 -0800 (In reply to comment #7) > (In reply to comment #5) > > It appears to me that signature check is only executed when one method is defined. Not sure if it's related to the bug. > > If there are overloaded functions then the bindings check which one to call based on the type check. If there is only one, the same type check is not required. Thanks for your explanation, Eric. Do you mean the type check in v8 builtins.cc should be skipped for non-overloaded methods? But why other non-overloaded methods doesn't throw 'illegal invocation' error? For example, revokeObjectURLCallback doesn't have a signature. 563817 9 arv 2012-02-23 18:48:01 -0800 I think we have bugs related to overloaded static methods. 563821 10 zhenghao 2012-02-23 19:00:04 -0800 (In reply to comment #9) > I think we have bugs related to overloaded static methods. Is it a v8 bug or WebKit bug? Eric, could you help on this, or add someone here? 563826 11 zhenghao 2012-02-23 19:05:35 -0800 (In reply to comment #9) > I think we have bugs related to overloaded static methods. I just tested createBlobURL after removing the other definition in DOMURL.idl: #if defined(ENABLE_MEDIA_STREAM) && ENABLE_MEDIA_STREAM static [CallWith=ScriptExecutionContext,TreatReturnedNullStringAs=Undefined] DOMString createObjectURL(in MediaStream stream); #endif So there is only one static createBlobURL now. However, I still got illegal invocation error... So maybe it's not related to overloaded methods, just related to static methods. But weirdly, revokeObjectURL is also static method. 564402 12 arv 2012-02-24 09:55:54 -0800 (In reply to comment #11) > (In reply to comment #9) > > I think we have bugs related to overloaded static methods. > > I just tested createBlobURL after removing the other definition in DOMURL.idl: > > #if defined(ENABLE_MEDIA_STREAM) && ENABLE_MEDIA_STREAM > static [CallWith=ScriptExecutionContext,TreatReturnedNullStringAs=Undefined] DOMString createObjectURL(in MediaStream stream); > #endif > > So there is only one static createBlobURL now. However, I still got illegal invocation error... So maybe it's not related to overloaded methods, just related to static methods. But weirdly, revokeObjectURL is also static method. Do you have a test? This passes on the LayoutTests so maybe you are doing something strange. 564770 13 zhenghao 2012-02-24 17:01:55 -0800 Have you disabled MEDIASTREAM? fast/files/create-blob-url-crash.html throws error in both config, but a different one (illegal invocation) in this config. And many other tests related to createBlobURL fail. 589876 14 zhenghao 2012-03-28 05:33:20 -0700 (In reply to comment #13) > Have you disabled MEDIASTREAM? fast/files/create-blob-url-crash.html > throws error in both config, but a different one (illegal invocation) > in this config. And many other tests related to createBlobURL fail. The error is in CodeGeneratorV8.pm RequiresCustomSignature. If the function is overloaded, it returns 0 at the 2nd if. Otherwise, as Blob is wrapper type, it returns 1 at the if inside the 2nd foreach. sub RequiresCustomSignature { my $function = shift; # No signature needed for Custom function if ($function->signature->extendedAttributes->{"Custom"} || $function->signature->extendedAttributes->{"V8Custom"}) { return 0; } # No signature needed for overloaded function if (@{$function->{overloads}} > 1) { return 0; } # Type checking is performed in the generated code if ($function->signature->extendedAttributes->{"StrictTypeChecking"}) { return 0; } foreach my $parameter (@{$function->parameters}) { my $optional = $parameter->extendedAttributes->{"Optional"}; if (($optional && $optional ne "DefaultIsUndefined" && $optional ne "DefaultIsNullString") || $parameter->extendedAttributes->{"Callback"}) { return 0; } } foreach my $parameter (@{$function->parameters}) { if (IsWrapperType($parameter->type)) { return 1; } } return 0; } 589878 15 zhenghao 2012-03-28 05:34:47 -0700 (In reply to comment #14) > (In reply to comment #13) > > Have you disabled MEDIASTREAM? fast/files/create-blob-url-crash.html > > throws error in both config, but a different one (illegal invocation) > > in this config. And many other tests related to createBlobURL fail. > > The error is in CodeGeneratorV8.pm RequiresCustomSignature. If the function is overloaded, it returns 0 at the 2nd if. Otherwise, as Blob is wrapper type, it returns 1 at the if inside the 2nd foreach. > > sub RequiresCustomSignature > { > my $function = shift; > # No signature needed for Custom function > if ($function->signature->extendedAttributes->{"Custom"} || > $function->signature->extendedAttributes->{"V8Custom"}) { > return 0; > } > # No signature needed for overloaded function > if (@{$function->{overloads}} > 1) { > return 0; > } > # Type checking is performed in the generated code > if ($function->signature->extendedAttributes->{"StrictTypeChecking"}) { > return 0; > } > foreach my $parameter (@{$function->parameters}) { > my $optional = $parameter->extendedAttributes->{"Optional"}; > if (($optional && $optional ne "DefaultIsUndefined" && $optional ne "DefaultIsNullString") || $parameter->extendedAttributes->{"Callback"}) { > return 0; > } > } > > foreach my $parameter (@{$function->parameters}) { > if (IsWrapperType($parameter->type)) { > return 1; > } > } > return 0; > } revokeObjectURL doesn't require custom signature, because DOMString is not wrapper type. [CallWith=ScriptExecutionContext] static void revokeObjectURL(in DOMString url); 590908 16 zhenghao 2012-03-29 00:30:43 -0700 OK. Now I think it's right that createBlobURL needs a custom signature. But v8 could not handle static function correctly. In builtins.cc TypeCheck: Object* holder = recv; if (!recv_type->IsUndefined()) { for (; holder != heap->null_value(); holder = holder->GetPrototype()) { if (holder->IsInstanceOf(FunctionTemplateInfo::cast(recv_type))) { break; } } if (holder == heap->null_value()) return holder; } It try to verify holder is instance of recv_type. But for static function, holder is actually recv_type itself, not instance of recv_type. Thus, if we omit the step in objects-inl.h IsInstanceOf: bool Object::IsInstanceOf(FunctionTemplateInfo* expected) { // There is a constraint on the object; check. // if (!this->IsJSObject()) return false; // Fetch the constructor function of the object. // Object* cons_obj = JSObject::cast(this)->map()->constructor(); // Omit the above step. Use this directly. Object* cons_obj = this; if (!cons_obj->IsJSFunction()) return false; JSFunction* fun = JSFunction::cast(cons_obj); // Iterate through the chain of inheriting function templates to // see if the required one occurs. for (Object* type = fun->shared()->function_data(); type->IsFunctionTemplateInfo(); type = FunctionTemplateInfo::cast(type)->parent_template()) { if (type == expected) return true; } // Didn't find the required type in the inheritance chain. return false; } Then we can invoke the static function. I got a simple patch to v8, but I'm not confident if it's correct. Erik/Adam, could you add some v8 folks here? Thanks! 590910 17 134517 zhenghao 2012-03-29 00:32:10 -0700 Created attachment 134517 demo patch. not for review. 590918 18 zhenghao 2012-03-29 00:48:12 -0700 (In reply to comment #17) > Created an attachment (id=134517) [details] > demo patch. not for review. When MEDIA_STREAM is enabled, we have overloaded createObjectURL. The bug is hidden, because no custom signature is generated. I'm not sure if it's a v8 bug, or it's WebKit bug about invoking v8 incorrectly. 592366 19 arv 2012-03-30 11:59:19 -0700 (In reply to comment #18) > (In reply to comment #17) > > Created an attachment (id=134517) [details] [details] > > demo patch. not for review. > > When MEDIA_STREAM is enabled, we have overloaded createObjectURL. The bug is hidden, because no custom signature is generated. I'm not sure if it's a v8 bug, or it's WebKit bug about invoking v8 incorrectly. It is a bug in the code generator. 592785 20 134938 zhenghao 2012-03-30 21:03:30 -0700 Created attachment 134938 Patch 592980 21 134938 haraken 2012-04-01 00:50:07 -0700 Comment on attachment 134938 Patch Makes sense. 592994 22 134938 webkit.review.bot 2012-04-01 01:23:48 -0700 Comment on attachment 134938 Patch Clearing flags on attachment: 134938 Committed r112811: <http://trac.webkit.org/changeset/112811> 592995 23 webkit.review.bot 2012-04-01 01:23:53 -0700 All reviewed patches have been landed. Closing bug. 593553 24 arv 2012-04-02 09:58:30 -0700 Sorry for not replying earlier to this. It would have been nice if you added the problematic case to one of the test idl files so we could see how the generated code changed. 134517 2012-03-29 00:32:10 -0700 2012-03-30 21:03:15 -0700 demo patch. not for review. v8.patch text/plain 660 zhenghao SW5kZXg6IHNyYy9vYmplY3RzLWlubC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIHNyYy9vYmplY3RzLWlubC5o CShyZXZpc2lvbiAxMTEwMikKKysrIHNyYy9vYmplY3RzLWlubC5oCSh3b3JraW5nIGNvcHkpCkBA IC0xNDksNyArMTQ5LDcgQEAKICAgLy8gVGhlcmUgaXMgYSBjb25zdHJhaW50IG9uIHRoZSBvYmpl Y3Q7IGNoZWNrLgogICBpZiAoIXRoaXMtPklzSlNPYmplY3QoKSkgcmV0dXJuIGZhbHNlOwogICAv LyBGZXRjaCB0aGUgY29uc3RydWN0b3IgZnVuY3Rpb24gb2YgdGhlIG9iamVjdC4KLSAgT2JqZWN0 KiBjb25zX29iaiA9IEpTT2JqZWN0OjpjYXN0KHRoaXMpLT5tYXAoKS0+Y29uc3RydWN0b3IoKTsK KyAgT2JqZWN0KiBjb25zX29iaiA9IHRoaXMtPklzSlNGdW5jdGlvbigpID8gdGhpcyA6IEpTT2Jq ZWN0OjpjYXN0KHRoaXMpLT5tYXAoKS0+Y29uc3RydWN0b3IoKTsKICAgaWYgKCFjb25zX29iai0+ SXNKU0Z1bmN0aW9uKCkpIHJldHVybiBmYWxzZTsKICAgSlNGdW5jdGlvbiogZnVuID0gSlNGdW5j dGlvbjo6Y2FzdChjb25zX29iaik7CiAgIC8vIEl0ZXJhdGUgdGhyb3VnaCB0aGUgY2hhaW4gb2Yg aW5oZXJpdGluZyBmdW5jdGlvbiB0ZW1wbGF0ZXMgdG8K 134938 2012-03-30 21:03:30 -0700 2012-04-01 01:23:48 -0700 Patch bug-79222-20120331120328.patch text/plain 1448 zhenghao U3VidmVyc2lvbiBSZXZpc2lvbjogMTEyMjQzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTg2YWNjNDlhMGM5ZGRl ODYyNDVmMTE0NDk0NWJmNjcxMTcyYTExZi4uZWVjZjBjOThmMDQzZTQ3NDQ1ZWUwZDdjYTY5Yjgz MjYyYzRjNmEyNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEyLTAzLTMwICBIYW8g WmhlbmcgIDx6aGVuZ2hhb0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW2Nocm9taXVtXSBEbyBu b3QgZ2VuZXJhdGUgY3VzdG9tIHNpZ25hdHVyZSBmb3Igc3RhdGljIG1ldGhvZHMuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03OTIyMgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cy4gQ292 ZXJlZCBieSBleGlzdGluZyB0ZXN0cyB3aGVuIE1FRElBX1NUUkVBTSBpcyBkaXNhYmxlZC4KKwor ICAgICAgICAqIGJpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtOgorICAgICAgICAo UmVxdWlyZXNDdXN0b21TaWduYXR1cmUpOgorCiAyMDEyLTAzLTI3ICBMZXZpIFdlaW50cmF1YiAg PGxldml3QGNocm9taXVtLm9yZz4KIAogICAgICAgICBMYXlvdXRSZXBhaW50ZXI6IFJlbW92ZSB1 bnVzZWQgY29uc3RydWN0b3IgcGFyYW1ldGVyIGFuZCB1cGRhdGUgdG8gTGF5b3V0VW5pdHMKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4 LnBtIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yVjgucG0K aW5kZXggYmRjM2FjOTEzZTEwODk3M2IzY2Q3MDFmNTdjZDZiNzVjM2U3NTc4ZS4uYzVlZjc5OTA5 ZjMxNjk2MmRlNzRiMzEwMzI1ZjZhZDEyODU2N2IxMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yVjgucG0KKysrIGIvU291cmNlL1dlYkNv cmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yVjgucG0KQEAgLTM2MzMsNiArMzYzMyw5 IEBAIHN1YiBSZXF1aXJlc0N1c3RvbVNpZ25hdHVyZQogICAgIGlmIChAeyRmdW5jdGlvbi0+e292 ZXJsb2Fkc319ID4gMSkgewogICAgICAgICByZXR1cm4gMDsKICAgICB9CisgICAgaWYgKCRmdW5j dGlvbi0+aXNTdGF0aWMpIHsKKyAgICAgICAgcmV0dXJuIDA7CisgICAgfQogICAgICMgVHlwZSBj aGVja2luZyBpcyBwZXJmb3JtZWQgaW4gdGhlIGdlbmVyYXRlZCBjb2RlCiAgICAgaWYgKCRmdW5j dGlvbi0+c2lnbmF0dXJlLT5leHRlbmRlZEF0dHJpYnV0ZXMtPnsiU3RyaWN0VHlwZUNoZWNraW5n In0pIHsKICAgICAgIHJldHVybiAwOwo=