Bug 79222

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.

One example test you can try is fast/files/create-blob-url-crash.html .

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.

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); }

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.

(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.

(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.

(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.

I think we have bugs related to overloaded static methods.

(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?

(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.

(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.

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.

(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; }

(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);

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!

Created attachment 134517 [details] demo patch. not for review.

(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.

(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.

Created attachment 134938 [details] Patch

Comment on attachment 134938 [details] Patch Makes sense.

Comment on attachment 134938 [details] Patch Clearing flags on attachment: 134938 Committed r112811: <http://trac.webkit.org/changeset/112811>

All reviewed patches have been landed. Closing bug.

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.