Bug 25205 – XMLHttpRequest instance is not an instanceof XMLHttpRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Michael A. Puls II]

WebKit r42344

The following alerts true in FF, Opera and IE8, but alerts false in Safari.

javascript:alert((new XMLHttpRequest()) instanceof XMLHttpRequest)%3B

This causes (ProgressEvent_instance.target instanceof XMLHttpRequest) to evaluate to false in the load listener function for XHR.

[Alexey Proskuryakov]

Same problem with e.g. XSLTProcessor.

[Sam Weinig]

Interestingly, javascript:alert((new XMLHttpRequest).__proto__ == XMLHttpRequest.prototype) returns true.

[Sam Weinig]

Is this a regression?

[Alexey Proskuryakov]

(In reply to comment #3)
> Is this a regression?

Not from Safari 3.2.1.

[Yuzo Fujishima]

Created attachment 32370 [details]
Fix for "instanceof XMLHttpRequest"

Hi,

Can you review the attached patch?
It attempts to fix this bug by setting JSC::ImplementsHasInstance flag for JSXMLHttpRequestConstructor.

Yuzo

[Yuzo Fujishima]

If the proposed patch is the right fix, the same should be applicable to:

WebCore/bindings/js/JSAudioConstructor.cpp 
WebCore/bindings/js/JSImageConstructor.cpp 
WebCore/bindings/js/JSMessageChannelConstructor.cpp 
WebCore/bindings/js/JSOptionConstructor.cpp 
WebCore/bindings/js/JSWebKitCSSMatrixConstructor.cpp 
WebCore/bindings/js/JSWebKitPointConstructor.cpp 
WebCore/bindings/js/JSWorkerConstructor.cpp 
# WebCore/bindings/js/JSXMLHttpRequestConstructor.cpp 
WebCore/bindings/js/JSXSLTProcessorConstructor.cpp 

In that case, we should perhaps add:

static PassRefPtr<Structure> createStructureForConstructor(JSValue prototype);

to JavaScriptCore/runtime/JSObject.h
and call the method from the above classes.

Yuzo

[Yuzo Fujishima]

Created attachment 32434 [details]
This also fixes for XSTLProcessor, Audio, etc.

Can anyone review this, please?

I've updated the patch to fix for XSLTProcessor, Audio, etc.

Yuzo

[Darin Adler]

Comment on attachment 32434 [details]
This also fixes for XSTLProcessor, Audio, etc.

This seems OK, but not great. The design here is supposed to be that derived classes override createStructure if the base class version doesn't work right. Leaving behind a createStructure function that does the wrong thing and relying on the callers to call the correct one seems unnecessarily fragile to me. To share code, we could create a new base class, derived from DOMObject, and have all the constructor classes derive from this, and it would override createStructure to work properly.

Would you be willing to try that approach?

review- for now, but I'm open to landing this version if you don't like my suggestion and can convince me.

[Yuzo Fujishima]

Created attachment 32499 [details]
DOMConstructorObject approach.

Thank you for your review.

I agree.

I've revised the patch such that all the classes in question derive from DOMConstructorObject, which in turn derives from DOMObject.

Can you take another look?

Yuzo

[Darin Adler]

Comment on attachment 32499 [details]
DOMConstructorObject approach.

> +#ifndef NDEBUG
> +        virtual ~DOMConstructorObject();
> +#endif

What's the idea here? I don't see any definition of this destructor, so it seems like the change won't link. Have you tested yet?

Otherwise, everything looks great!

[Yuzo Fujishima]

Created attachment 32547 [details]
Removed virtual destructor declaration of DOMConstructorObject.

Hi,

Thank you for reviewing this.

I've removed the virtual destructor declaration of DOMConstructorObject
and confirmed that the layout test succeeds for both Debug and Release.

Yuzo

[Brent Fulgham]

Assign for landing.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45938.

[Eric Seidel (no email)]

This doesn't fix all constructors though.  This diff is still missing from the change:

@@ -2027,10 +2042,10 @@ sub constructorFor
     my $canConstruct = shift;
 
 my $implContent = << "EOF";
-class ${className}Constructor : public DOMObject {
+class ${className}Constructor : public DOMConstructorObject {
 public:
     ${className}Constructor(ExecState* exec, JSDOMGlobalObject* globalObject)
-        : DOMObject(${className}Constructor::createStructure(globalObject->objectPrototype()))
+        : DOMConstructorObject(${className}Constructor::createStructure(globalObject->objectPrototype()), globalObject)
     {
         putDirect(exec->propertyNames().prototype, ${protoClassName}::self(exec, globalObject), None);
     }

I guess I'll find a way to write a test and file a new bug for that change.

[Eric Seidel (no email)]

Hum... I'm surprised Darin let you get away w/o a test case!

[Yuzo Fujishima]

My patch contains:
LayoutTests/fast/js/instanceof-operator.html b/LayoutTests/fast/js/instanceof-operator.html

Do you mean a different kind of test?

[Eric Seidel (no email)]

Sorry. I just missed them in the diff http://trac.webkit.org/changeset/45938.  I never remember to look for the special green squares.