[chromium] createObjectURL(Blob) throws 'Illegal invocation' error when MEDIA_STREAM is disabled.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Hao Zheng]

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 .

[Hao Zheng]

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.

[Hao Zheng]

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

[Kaustubh Atrawalkar]

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.

[Hao Zheng]

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

[Hao Zheng]

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.

[Erik Arvidsson]

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

[Erik Arvidsson]

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

[Hao Zheng]

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

[Erik Arvidsson]

I think we have bugs related to overloaded static methods.

[Hao Zheng]

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

[Hao Zheng]

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

[Erik Arvidsson]

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

[Hao Zheng]

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.

[Hao Zheng]

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

[Hao Zheng]

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

[Hao Zheng]

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!

[Hao Zheng]

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

[Hao Zheng]

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

[Erik Arvidsson]

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

[Hao Zheng]

Created attachment 134938 [details]
Patch

[Kentaro Hara]

Comment on attachment 134938 [details]
Patch

Makes sense.

[WebKit Review Bot]

Comment on attachment 134938 [details]
Patch

Clearing flags on attachment: 134938

Committed r112811: <http://trac.webkit.org/changeset/112811>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Erik Arvidsson]

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.