Bug 79222 - [chromium] createObjectURL(Blob) throws 'Illegal invocation' error when MEDIA_STREAM is disabled.
Summary: [chromium] createObjectURL(Blob) throws 'Illegal invocation' error when MEDIA...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hao Zheng
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-02-22 04:12 PST by Hao Zheng
Modified: 2012-04-02 09:58 PDT (History)
7 users (show)

See Also:


Attachments
demo patch. not for review. (660 bytes, patch)
2012-03-29 00:32 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Patch (1.41 KB, patch)
2012-03-30 21:03 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hao Zheng 2012-02-22 04:12:23 PST
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 .
Comment 1 Hao Zheng 2012-02-22 04:14:14 PST
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.
Comment 2 Hao Zheng 2012-02-22 04:15:20 PST
One example test you can try is fast/files/create-blob-url-crash.html .
Comment 3 Kaustubh Atrawalkar 2012-02-22 04:25:33 PST
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.
Comment 4 Hao Zheng 2012-02-23 03:58:05 PST
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);
  }
Comment 5 Hao Zheng 2012-02-23 04:07:59 PST
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.
Comment 6 Erik Arvidsson 2012-02-23 09:07:07 PST
(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.
Comment 7 Erik Arvidsson 2012-02-23 09:08:05 PST
(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.
Comment 8 Hao Zheng 2012-02-23 18:43:48 PST
(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.
Comment 9 Erik Arvidsson 2012-02-23 18:48:01 PST
I think we have bugs related to overloaded static methods.
Comment 10 Hao Zheng 2012-02-23 19:00:04 PST
(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?
Comment 11 Hao Zheng 2012-02-23 19:05:35 PST
(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.
Comment 12 Erik Arvidsson 2012-02-24 09:55:54 PST
(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.
Comment 13 Hao Zheng 2012-02-24 17:01:55 PST
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.
Comment 14 Hao Zheng 2012-03-28 05:33:20 PDT
(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;
}
Comment 15 Hao Zheng 2012-03-28 05:34:47 PDT
(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);
Comment 16 Hao Zheng 2012-03-29 00:30:43 PDT
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!
Comment 17 Hao Zheng 2012-03-29 00:32:10 PDT
Created attachment 134517 [details]
demo patch. not for review.
Comment 18 Hao Zheng 2012-03-29 00:48:12 PDT
(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.
Comment 19 Erik Arvidsson 2012-03-30 11:59:19 PDT
(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.
Comment 20 Hao Zheng 2012-03-30 21:03:30 PDT
Created attachment 134938 [details]
Patch
Comment 21 Kentaro Hara 2012-04-01 00:50:07 PDT
Comment on attachment 134938 [details]
Patch

Makes sense.
Comment 22 WebKit Review Bot 2012-04-01 01:23:48 PDT
Comment on attachment 134938 [details]
Patch

Clearing flags on attachment: 134938

Committed r112811: <http://trac.webkit.org/changeset/112811>
Comment 23 WebKit Review Bot 2012-04-01 01:23:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Erik Arvidsson 2012-04-02 09:58:30 PDT
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.