748983 - foo instanceof XMLHttpRequest always throws

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We only generate a hasInstance hook in some cases.  In the cases when we do NOT generate it, we should just be using a Function, not something of our interface object class.  Otherwise hasInstance just throws.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the instanceof behavior for new bindings in situations where we don't need a custom hasInstance hook.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: With some asserts fixed

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 618500 [details] [diff] [review]
With some asserts fixed

Review of attachment 618500 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Utils.cpp
@@ +38,5 @@
> +    }
> +    constructor = JS_NewObject(cx, constructorClass, functionProto, global);
> +  } else {
> +    MOZ_ASSERT(constructorNative);
> +    // XXXbz should the name for the function be "name" or NULL?

Can we clear this up? My gut says "name", but I don't see this defined anywhere.

@@ +108,5 @@
>                         ConstantSpec *constants, JSFunctionSpec *staticMethods,
>                         const char* name)
>  {
> +  MOZ_ASSERT(protoClass || constructorClass || constructor,
> +             "Need at least one class!");

... or a constructor

@@ +263,5 @@
> +JSBool
> +ThrowingConstructorWorkers(JSContext* cx, unsigned argc, JS::Value* vp)
> +{
> +  return Throw<false>(cx, NS_ERROR_FAILURE);
> +}

Could also templatize ThrowingConstructor.

::: dom/bindings/Utils.h
@@ +229,5 @@
> + *                  if it should be a function object.
> + * constructor is the JSNative to use as a constructor.  If this is non-null, it
> + *             should be used as a JSNative to back the interface object, which
> + *             should be a Function.  If this is null, then we should create an
> + *             object of constructorClass, unles that's also null, in which case

unless

@@ +230,5 @@
> + * constructor is the JSNative to use as a constructor.  If this is non-null, it
> + *             should be used as a JSNative to back the interface object, which
> + *             should be a Function.  If this is null, then we should create an
> + *             object of constructorClass, unles that's also null, in which case
> + *             we shold not create an interface object at all.

should

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Can we clear this up? My gut says "name", but I don't see this defined anywhere.

Looks like .name on function is not actually standard.  ;)  I think having this come up as |function XMLHttpRequest() {}| makes a lot of sense.  I'll just remove the XXX comment.

Fixed the other issues.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Could also templatize ThrowingConstructor.

Can I put a pointer to a template function in the JSClass?  Seems a bit chancy; I've left this part as is.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Really updated to comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5368d38c9f8f

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Aurora version of patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 620587 [details] [diff] [review]
Aurora version of patch

[Approval Request Comment]
Regression caused by (bug #): bug 740069
User impact if declined: Incorrect instanceof behavior for XMLHttpRequest; 
  likely web content fallout.
Testing completed (on m-c, etc.): Passes automated tests
Risk to taking this patch (and alternatives if risky): Low risk: makes something
   that throws right now return a boolen instead.  Another option is disabling
   the Paris bindings for XHR if we think this is too risky but don't want
   to ship the bug.   Yet another option is to ship the bug.
String changes made by this patch: None.

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 620587 [details] [diff] [review]
Aurora version of patch

[Triage Comment]
Early enough in the cycle to take a low-risk fix for this regression. Approving for Aurora 14.

Comment 12

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/faaebf77bbc8

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5368d38c9f8f