WebCore:
Reviewed by Darin.
<rdar://problem/5491013> REGRESSION: -[WebView windowScriptObject] returns a dummy object or nil if a page hasn't loaded
Calling -[WebView windowScriptObject] before the page loads would give you nil. This behavior didn't match Tiger.
The API behavior in Tiger let you get the window script object once and keep ahold of it as long as you needed it.
The window object would remain valid even after page loads. This change restores the Tiger behavior.
<rdar://problem/5495790> NULL dereference crash beneath Bindings::RootObject::interpreter when saving Dashcode document
The changes to WebScriptObject's _isSafeScript call also fixed the crash in Dashcode.
* bindings/objc/WebScriptObject.mm:
(-[WebScriptObject _setOriginRootObject:andRootObject:]): New method used to update the the root objects,
so the WebScriptObject can still be used after a page load.
(-[WebScriptObject _isSafeScript]): Call [self _rootObject] instead of accessing the data member directly.
DOMNode has an override for the _rootObject method, and it can return 0 when _private->_rootObject
is non-zero. We would return YES here when it wasn't safe and later crash with my modified layout tests.
Checking _rootObject first prevents other calls sites from needing to check for a valid root object,
this fixed the Dashcode crash.
(-[WebScriptObject _imp]): Ditto.
* bindings/objc/WebScriptObjectPrivate.h: Add _setOriginRootObject:andRootObject:.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::clear): Call the renamed clearScriptObjects function.
(WebCore::FrameLoader::dispatchWindowObjectAvailable): Some gratuitous code cleanup.
* page/Frame.cpp:
(WebCore::Frame::clearScriptObjects): Renamed cleanupScriptObject to clearScriptObjects.
Call clearPlatformScriptObjects last so m_bindingRootObject is already NULL.
(WebCore::Frame::windowScriptNPObject): Hold a JSLock before accessing the window. This
change is unrelated to the bug, but should be fixed.
* page/mac/FrameMac.mm:
(WebCore::Frame::windowScriptObject): Return a script object even if the interpreter is NULL.
This resotres the Tiger behavior of always being able to access the window object.
(WebCore::Frame::clearPlatformScriptObjects): Keep the window script object around, and update
the root objects for the window script object.
* page/Frame.h: Rename cleanupScriptObject to clearScriptObjects.
* page/FramePrivate.h: Use a RetainPtr for m_windowScriptObject.
WebKitTools:
Reviewed by Darin.
<rdar://problem/5491013> REGRESSION: -[WebView windowScriptObject] returns a dummy object or nil if a page hasn't loaded (breaks EA Sports Online)
Assert that the -[WebScriptObject JSObject] return value is only NULL for non-window objects.
This is tested by plugins/root-object-premature-delete-crash.html.
* DumpRenderTree/mac/ObjCController.m:
(-[ObjCController accessStoredWebScriptObject]):
LayoutTests:
Reviewed by Darin.
<rdar://problem/5491013> REGRESSION: -[WebView windowScriptObject] returns a dummy object or nil if a page hasn't loaded (breaks EA Sports Online)
Updated results to show the didClearWindowScriptObject delegate call. Tweaked plugins/root-object-premature-delete-crash.html
to test a non-window script object half the time. This change works with a change to DumpRenderTree.
* http/tests/loading/empty-subframe-expected.txt:
* http/tests/loading/simple-subframe-expected.txt:
* plugins/root-object-premature-delete-crash.html:
* webarchive/loading/test-loading-archive-expected.txt:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@25697 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 0a44d42..d3e7ab0 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2007-09-21 Timothy Hatcher <timothy@apple.com>
+
+ Reviewed by Darin.
+
+ <rdar://problem/5491013> REGRESSION: -[WebView windowScriptObject] returns a dummy object or nil if a page hasn't loaded (breaks EA Sports Online)
+
+ Updated results to show the didClearWindowScriptObject delegate call. Tweaked plugins/root-object-premature-delete-crash.html
+ to test a non-window script object half the time. This change works with a change to DumpRenderTree.
+
+ * http/tests/loading/empty-subframe-expected.txt:
+ * http/tests/loading/simple-subframe-expected.txt:
+ * plugins/root-object-premature-delete-crash.html:
+ * webarchive/loading/test-loading-archive-expected.txt:
+
2007-09-20 Rob Buis <buis@kde.org>
Reviewed by Mitz.
diff --git a/LayoutTests/plugins/root-object-premature-delete-crash.html b/LayoutTests/plugins/root-object-premature-delete-crash.html
index 9a1f7c8..75686610 100644
--- a/LayoutTests/plugins/root-object-premature-delete-crash.html
+++ b/LayoutTests/plugins/root-object-premature-delete-crash.html
@@ -28,7 +28,7 @@
queueTest();
return;
}
-
+
log("PASS: You didn't crash.\n");
layoutTestController.notifyDone();
}
@@ -36,10 +36,16 @@
function queueTest()
{
iframe.onload = runTest;
- objCController.storeWebScriptObject(iframe.contentWindow);
+ objCController.storeWebScriptObject(count % 2 == 0 ? iframe.contentWindow : iframe.contentDocument.documentElement);
iframe.contentWindow.location.reload();
}
+function timeoutTest()
+{
+ log("FAIL: Test timed-out after " + count + " runs.\n");
+ layoutTestController.notifyDone();
+}
+
window.onload = function onload()
{
if (!("layoutTestController" in window) || !("objCController" in window)) {
@@ -48,6 +54,8 @@
throw errorMessage;
}
+ timeoutIdentifier = setTimeout(timeoutTest, 30000); // timeout after 30 seconds
+
layoutTestController.dumpAsText();
layoutTestController.waitUntilDone();
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 228bc2f..9b25a22 100644
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,48 @@
+2007-09-21 Timothy Hatcher <timothy@apple.com>
+
+ Reviewed by Darin.
+
+ <rdar://problem/5491013> REGRESSION: -[WebView windowScriptObject] returns a dummy object or nil if a page hasn't loaded
+
+ Calling -[WebView windowScriptObject] before the page loads would give you nil. This behavior didn't match Tiger.
+ The API behavior in Tiger let you get the window script object once and keep ahold of it as long as you needed it.
+ The window object would remain valid even after page loads. This change restores the Tiger behavior.
+
+ <rdar://problem/5495790> NULL dereference crash beneath Bindings::RootObject::interpreter when saving Dashcode document
+
+ The changes to WebScriptObject's _isSafeScript call also fixed the crash in Dashcode.
+
+ * bindings/objc/WebScriptObject.mm:
+ (-[WebScriptObject _setOriginRootObject:andRootObject:]): New method used to update the the root objects,
+ so the WebScriptObject can still be used after a page load.
+ (-[WebScriptObject _isSafeScript]): Call [self _rootObject] instead of accessing the data member directly.
+ DOMNode has an override for the _rootObject method, and it can return 0 when _private->_rootObject
+ is non-zero. We would return YES here when it wasn't safe and later crash with my modified layout tests.
+ Checking _rootObject first prevents other calls sites from needing to check for a valid root object,
+ this fixed the Dashcode crash.
+ (-[WebScriptObject _imp]): Ditto.
+
+ * bindings/objc/WebScriptObjectPrivate.h: Add _setOriginRootObject:andRootObject:.
+
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::clear): Call the renamed clearScriptObjects function.
+ (WebCore::FrameLoader::dispatchWindowObjectAvailable): Some gratuitous code cleanup.
+
+ * page/Frame.cpp:
+ (WebCore::Frame::clearScriptObjects): Renamed cleanupScriptObject to clearScriptObjects.
+ Call clearPlatformScriptObjects last so m_bindingRootObject is already NULL.
+ (WebCore::Frame::windowScriptNPObject): Hold a JSLock before accessing the window. This
+ change is unrelated to the bug, but should be fixed.
+
+ * page/mac/FrameMac.mm:
+ (WebCore::Frame::windowScriptObject): Return a script object even if the interpreter is NULL.
+ This resotres the Tiger behavior of always being able to access the window object.
+ (WebCore::Frame::clearPlatformScriptObjects): Keep the window script object around, and update
+ the root objects for the window script object.
+
+ * page/Frame.h: Rename cleanupScriptObject to clearScriptObjects.
+ * page/FramePrivate.h: Use a RetainPtr for m_windowScriptObject.
+
2007-09-21 Mike Fenton <mike@staikos.net>
Reviewed by George Staikos.
diff --git a/WebCore/bindings/objc/WebScriptObject.mm b/WebCore/bindings/objc/WebScriptObject.mm
index befa76f..ffe5550 100644
--- a/WebCore/bindings/objc/WebScriptObject.mm
+++ b/WebCore/bindings/objc/WebScriptObject.mm
@@ -132,10 +132,30 @@
WebCore::addJSWrapper(self, imp);
- if(_private->rootObject)
+ if (_private->rootObject)
_private->rootObject->gcProtect(imp);
}
+- (void)_setOriginRootObject:(PassRefPtr<RootObject>)originRootObject andRootObject:(PassRefPtr<RootObject>)rootObject
+{
+ ASSERT(_private->imp);
+
+ if (rootObject)
+ rootObject->gcProtect(_private->imp);
+
+ if (_private->rootObject && _private->rootObject->isValid())
+ _private->rootObject->gcUnprotect(_private->imp);
+
+ if (_private->rootObject)
+ _private->rootObject->deref();
+
+ if (_private->originRootObject)
+ _private->originRootObject->deref();
+
+ _private->rootObject = rootObject.releaseRef();
+ _private->originRootObject = originRootObject.releaseRef();
+}
+
- (id)_initWithJSObject:(KJS::JSObject*)imp originRootObject:(PassRefPtr<KJS::Bindings::RootObject>)originRootObject rootObject:(PassRefPtr<KJS::Bindings::RootObject>)rootObject
{
ASSERT(imp);
@@ -153,7 +173,7 @@
// This is done on lazily, on demand.
if (!_private->imp && _private->isCreatedByDOMWrapper)
[self _initializeScriptDOMNodeImp];
- return _private->rootObject && _private->rootObject->isValid() ? _private->imp : 0;
+ return [self _rootObject] ? _private->imp : 0;
}
- (BOOL)_hasImp
@@ -161,6 +181,8 @@
return _private->imp != nil;
}
+// Node that DOMNode overrides this method. So you should almost always
+// use this method call instead of _private->rootObject directly.
- (RootObject*)_rootObject
{
return _private->rootObject && _private->rootObject->isValid() ? _private->rootObject : 0;
@@ -173,13 +195,17 @@
- (BOOL)_isSafeScript
{
+ RootObject *root = [self _rootObject];
+ if (!root)
+ return false;
+
if (!_private->originRootObject)
return true;
- if (!_private->originRootObject->isValid() || !_private->rootObject || !_private->rootObject->isValid())
+ if (!_private->originRootObject->isValid())
return false;
- return _private->originRootObject->interpreter()->isSafeScript(_private->rootObject->interpreter());
+ return _private->originRootObject->interpreter()->isSafeScript(root->interpreter());
}
- (void)dealloc
diff --git a/WebCore/bindings/objc/WebScriptObjectPrivate.h b/WebCore/bindings/objc/WebScriptObjectPrivate.h
index 2e071aa..53982ad 100644
--- a/WebCore/bindings/objc/WebScriptObjectPrivate.h
+++ b/WebCore/bindings/objc/WebScriptObjectPrivate.h
@@ -24,6 +24,7 @@
- (id)_init;
- (id)_initWithJSObject:(KJS::JSObject*)imp originRootObject:(PassRefPtr<KJS::Bindings::RootObject>)originRootObject rootObject:(PassRefPtr<KJS::Bindings::RootObject>)rootObject;
- (void)_setImp:(KJS::JSObject*)imp originRootObject:(PassRefPtr<KJS::Bindings::RootObject>)originRootObject rootObject:(PassRefPtr<KJS::Bindings::RootObject>)rootObject;
+- (void)_setOriginRootObject:(PassRefPtr<KJS::Bindings::RootObject>)originRootObject andRootObject:(PassRefPtr<KJS::Bindings::RootObject>)rootObject;
- (void)_initializeScriptDOMNodeImp;
- (KJS::JSObject *)_imp;
- (BOOL)_hasImp;
diff --git a/WebCore/loader/FrameLoader.cpp b/WebCore/loader/FrameLoader.cpp
index 7172f71..404d118 100644
--- a/WebCore/loader/FrameLoader.cpp
+++ b/WebCore/loader/FrameLoader.cpp
@@ -801,7 +801,7 @@
m_decoder = 0;
m_containsPlugIns = false;
- m_frame->cleanupScriptObjects();
+ m_frame->clearScriptObjects();
m_redirectionTimer.stop();
m_scheduledRedirection.clear();
@@ -4477,13 +4477,14 @@
void FrameLoader::dispatchWindowObjectAvailable()
{
- if (Settings* settings = m_frame->settings())
- if (settings->isJavaScriptEnabled() && m_frame->scriptProxy()->haveInterpreter()) {
- m_client->windowObjectCleared();
- if (Page* page = m_frame->page())
- if (InspectorController* inspector = page->parentInspectorController())
- inspector->windowScriptObjectAvailable();
- }
+ Settings* settings = m_frame->settings();
+ if (!settings || !settings->isJavaScriptEnabled() || !m_frame->scriptProxy()->haveInterpreter())
+ return;
+
+ m_client->windowObjectCleared();
+ if (Page* page = m_frame->page())
+ if (InspectorController* inspector = page->parentInspectorController())
+ inspector->windowScriptObjectAvailable();
}
Widget* FrameLoader::createJavaAppletWidget(const IntSize& size, Element* element, const HashMap<String, String>& args)
diff --git a/WebCore/page/Frame.cpp b/WebCore/page/Frame.cpp
index d9ded76..df0bb84 100644
--- a/WebCore/page/Frame.cpp
+++ b/WebCore/page/Frame.cpp
@@ -1105,6 +1105,7 @@
if (settings && settings->isJavaScriptEnabled()) {
// JavaScript is enabled, so there is a JavaScript window object. Return an NPObject bound to the window
// object.
+ KJS::JSLock lock;
KJS::JSObject* win = KJS::Window::retrieveWindow(this);
ASSERT(win);
KJS::Bindings::RootObject* root = bindingRootObject();
@@ -1143,9 +1144,8 @@
d->m_rootObjects.remove(it);
}
-void Frame::cleanupScriptObjects()
+void Frame::clearScriptObjects()
{
- cleanupPlatformScriptObjects();
JSLock lock;
RootObjectMap::const_iterator end = d->m_rootObjects.end();
@@ -1168,6 +1168,8 @@
d->m_windowScriptNPObject = 0;
}
#endif
+
+ clearPlatformScriptObjects();
}
RenderObject *Frame::renderer() const
diff --git a/WebCore/page/Frame.h b/WebCore/page/Frame.h
index 05bf80e..046b32c 100644
--- a/WebCore/page/Frame.h
+++ b/WebCore/page/Frame.h
@@ -233,11 +233,11 @@
void clearScriptProxy();
void clearDOMWindow();
- void cleanupScriptObjects();
+ void clearScriptObjects();
void cleanupScriptObjectsForPlugin(void*);
private:
- void cleanupPlatformScriptObjects();
+ void clearPlatformScriptObjects();
void lifeSupportTimerFired(Timer<Frame>*);
diff --git a/WebCore/page/FramePrivate.h b/WebCore/page/FramePrivate.h
index 9e8d64a..d684022 100644
--- a/WebCore/page/FramePrivate.h
+++ b/WebCore/page/FramePrivate.h
@@ -124,7 +124,7 @@
RootObjectMap m_rootObjects;
NPObject* m_windowScriptNPObject;
#if PLATFORM(MAC)
- WebScriptObject* m_windowScriptObject;
+ RetainPtr<WebScriptObject> m_windowScriptObject;
WebCoreFrameBridge* m_bridge;
#endif
};
diff --git a/WebCore/page/mac/FrameMac.mm b/WebCore/page/mac/FrameMac.mm
index 1d72520..6b0ff12 100644
--- a/WebCore/page/mac/FrameMac.mm
+++ b/WebCore/page/mac/FrameMac.mm
@@ -648,30 +648,25 @@
WebScriptObject* Frame::windowScriptObject()
{
Settings* settings = this->settings();
- if (!settings || !settings->isJavaScriptEnabled() || !scriptProxy()->haveInterpreter())
+ if (!settings || !settings->isJavaScriptEnabled())
return 0;
if (!d->m_windowScriptObject) {
KJS::JSLock lock;
KJS::JSObject* win = KJS::Window::retrieveWindow(this);
KJS::Bindings::RootObject *root = bindingRootObject();
- d->m_windowScriptObject = HardRetain([WebScriptObject scriptObjectForJSObject:toRef(win) originRootObject:root rootObject:root]);
+ d->m_windowScriptObject = [WebScriptObject scriptObjectForJSObject:toRef(win) originRootObject:root rootObject:root];
}
- return d->m_windowScriptObject;
+ return d->m_windowScriptObject.get();
}
-void Frame::cleanupPlatformScriptObjects()
+void Frame::clearPlatformScriptObjects()
{
- HardRelease(d->m_windowScriptObject);
- // Explicitly remove m_windowScriptObject from the wrapper caches, otherwise
- // the next load might end up with a stale, cached m_windowScriptObject.
- // (This problem is unique to m_windowScriptObject because its JS/DOM counterparts
- // persist across page loads.)
- removeDOMWrapper(reinterpret_cast<DOMObjectInternal*>(d->m_domWindow.get()));
- if (d->m_jscript && d->m_jscript->haveInterpreter())
- removeJSWrapper(KJS::Window::retrieveWindow(this));
- d->m_windowScriptObject = 0;
+ if (d->m_windowScriptObject) {
+ KJS::Bindings::RootObject* root = bindingRootObject();
+ [d->m_windowScriptObject.get() _setOriginRootObject:root andRootObject:root];
+ }
}
} // namespace WebCore
diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
index 40fe8ae..338449d 100644
--- a/WebKitTools/ChangeLog
+++ b/WebKitTools/ChangeLog
@@ -1,3 +1,15 @@
+2007-09-21 Timothy Hatcher <timothy@apple.com>
+
+ Reviewed by Darin.
+
+ <rdar://problem/5491013> REGRESSION: -[WebView windowScriptObject] returns a dummy object or nil if a page hasn't loaded (breaks EA Sports Online)
+
+ Assert that the -[WebScriptObject JSObject] return value is only NULL for non-window objects.
+ This is tested by plugins/root-object-premature-delete-crash.html.
+
+ * DumpRenderTree/mac/ObjCController.m:
+ (-[ObjCController accessStoredWebScriptObject]):
+
2007-09-21 Kevin McCullough <kmccullough@apple.com>
Reviewed by Oliver.
diff --git a/WebKitTools/DumpRenderTree/mac/ObjCController.m b/WebKitTools/DumpRenderTree/mac/ObjCController.m
index c738a2b..c3c04e2 100644
--- a/WebKitTools/DumpRenderTree/mac/ObjCController.m
+++ b/WebKitTools/DumpRenderTree/mac/ObjCController.m
@@ -31,6 +31,7 @@
#import <JavaScriptCore/Assertions.h>
#import <WebKit/WebScriptObject.h>
#import <WebKit/WebView.h>
+#import <WebKit/DOMAbstractView.h>
@implementation ObjCController
@@ -153,8 +154,11 @@
- (void)accessStoredWebScriptObject
{
+#if !ASSERT_DISABLED
+ BOOL isWindowObject = [storedWebScriptObject isKindOfClass:[DOMAbstractView class]];
+#endif
JSObjectRef jsObject = [storedWebScriptObject JSObject];
- ASSERT(!jsObject);
+ ASSERT((jsObject && isWindowObject) || (!jsObject && !isWindowObject));
[storedWebScriptObject callWebScriptMethod:@"" withArguments:nil];
[storedWebScriptObject evaluateWebScript:@""];
