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.
Same problem with e.g. XSLTProcessor.
Interestingly, javascript:alert((new XMLHttpRequest).__proto__ == XMLHttpRequest.prototype) returns true.
Is this a regression?
(In reply to comment #3) > Is this a regression? Not from Safari 3.2.1.
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
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
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
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.
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
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!
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
Assign for landing.
Landed in http://trac.webkit.org/changeset/45938.
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.
Hum... I'm surprised Darin let you get away w/o a test case!
My patch contains: LayoutTests/fast/js/instanceof-operator.html b/LayoutTests/fast/js/instanceof-operator.html Do you mean a different kind of test?
Sorry. I just missed them in the diff http://trac.webkit.org/changeset/45938. I never remember to look for the special green squares.