Last Comment Bug 748983 - foo instanceof XMLHttpRequest always throws
: foo instanceof XMLHttpRequest always throws
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 740069
  Show dependency treegraph
 
Reported: 2012-04-25 14:42 PDT by Boris Zbarsky [:bz]
Modified: 2012-05-04 02:29 PDT (History)
3 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Fix the instanceof behavior for new bindings in situations where we don't need a custom hasInstance hook. (13.24 KB, patch)
2012-04-25 15:40 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
With some asserts fixed (13.47 KB, patch)
2012-04-25 18:25 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Splinter Review
Updated to comments (13.64 KB, patch)
2012-05-02 12:21 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Really updated to comments (13.66 KB, patch)
2012-05-02 12:24 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Aurora version of patch (13.50 KB, patch)
2012-05-02 22:23 PDT, Boris Zbarsky [:bz]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-04-25 14:42:49 PDT
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 [:bz] 2012-04-25 15:40:29 PDT
Created attachment 618459 [details] [diff] [review]
Fix the instanceof behavior for new bindings in situations where we don't need a custom hasInstance hook.
Comment 2 Boris Zbarsky [:bz] 2012-04-25 18:25:02 PDT
Created attachment 618500 [details] [diff] [review]
With some asserts fixed
Comment 3 Peter Van der Beken [:peterv] 2012-05-02 07:15:17 PDT
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 [:bz] 2012-05-02 11:57:21 PDT
> 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 [:bz] 2012-05-02 12:04:53 PDT
> 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 [:bz] 2012-05-02 12:21:22 PDT
Created attachment 620422 [details] [diff] [review]
Updated to comments
Comment 7 Boris Zbarsky [:bz] 2012-05-02 12:24:47 PDT
Created attachment 620423 [details] [diff] [review]
Really updated to comments
Comment 9 Boris Zbarsky [:bz] 2012-05-02 22:23:53 PDT
Created attachment 620587 [details] [diff] [review]
Aurora version of patch
Comment 10 Boris Zbarsky [:bz] 2012-05-02 22:26:21 PDT
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] 2012-05-03 09:22:28 PDT
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 13 Ed Morley [:emorley] 2012-05-04 02:29:50 PDT
https://hg.mozilla.org/mozilla-central/rev/5368d38c9f8f

Note You need to log in before you can comment on or make changes to this bug.