The default bug view has changed. See this FAQ.

foo instanceof XMLHttpRequest always throws

RESOLVED FIXED in Firefox 14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({regression})

unspecified
mozilla15
x86
Mac OS X
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14+ fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Created attachment 618459 [details] [diff] [review]
Fix the instanceof behavior for new bindings in situations where we don't need a custom hasInstance hook.
Attachment #618459 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Created attachment 618500 [details] [diff] [review]
With some asserts fixed
Attachment #618500 - Flags: review?(peterv)
Attachment #618459 - Attachment is obsolete: true
Attachment #618459 - Flags: review?(peterv)

Updated

5 years ago
tracking-firefox14: ? → +
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
Attachment #618500 - Flags: review?(peterv) → review+
> 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.
Whiteboard: [need review] → [need landing]
> 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.
Created attachment 620422 [details] [diff] [review]
Updated to comments
Attachment #618500 - Attachment is obsolete: true
Created attachment 620423 [details] [diff] [review]
Really updated to comments
Attachment #620422 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/5368d38c9f8f
Flags: in-testsuite+
Whiteboard: [need landing]
Target Milestone: --- → mozilla15
Created attachment 620587 [details] [diff] [review]
Aurora version of patch
Attachment #620423 - Attachment is obsolete: true
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.
Attachment #620587 - Flags: approval-mozilla-aurora?
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.
Attachment #620587 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #620423 - Attachment is obsolete: false
http://hg.mozilla.org/releases/mozilla-aurora/rev/faaebf77bbc8
status-firefox14: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/5368d38c9f8f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.