31626 2009-11-18 05:24:37 -0800 [Qt] layoutTestController.notifyDone() not working when the frame was reloaded. 2010-06-17 02:59:06 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) PC All RESOLVED DUPLICATE 37725 Qt P3 Normal --- 1 kenneth webkit-qt-unassigned abarth abecsi darin hausmann jturcotte laszlo.gombos loki ossy robert sam tonikitoo oldest_to_newest 164548 0 kenneth 2009-11-18 05:24:37 -0800 So let's see if I can explain this easily. Lets focus on one of the layout tests fast/dom/javascript-url-crash-function.html resulting in timeout. This test calls waitUntilDone from the mainframe and creates an iframe, calling a function and then calling notifyDone. 9 if (window.layoutTestController) { 10 layoutTestController.dumpAsText(); 11 layoutTestController.dumpChildFramesAsText(); 12 layoutTestController.waitUntilDone(); 13 } This above part works perfectly fine. Now looking at the code exporting our QObject based LayoutTestController to JS, initJSObjects is called 3 times, which might seem strange. The last two are called for the iframe. It is not that strange if you look at the code of the iframe 1 <script> 2 function test() 3 { 4 (function () 5 { 6 (function () 7 { 8 (function () 9 { 10 frameElement.src = "javascript:'<pre>PASS: You didn\\'t crash.</pre>'"; 11 })() 12 })() 13 })() 14 } 15 16 setTimeout(function () 17 { 18 test(); 19 if (window.layoutTestController) 20 layoutTestController.notifyDone(); 21 22 }, 0); 23 </script> As you notice, the frameElement.src reloads the frame, so the javaScriptWindowObjectCleared () signal will be called again, resulting in initJSObjects being called. OK, let's get to the problem. 19 if (window.layoutTestController) 20 layoutTestController.notifyDone(); is not working! window.layoutTestController is true, but layoutTestController.notifyDone() does nothing. Replacing it with window.layoutTestController.notifyDone() make the test pass, but isn't right. The frameElement.src = is the result of this. Out commenting that instead of adding the window. makes the test not timeout. 164630 1 kenneth 2009-11-18 10:43:47 -0800 Changing frameElement.src to refer an actual file, actually works, so it seems like an issue with javascript: navigations. 164638 2 kenneth 2009-11-18 11:30:12 -0800 I started to follow the flow when frameElement.src = "javascript:..." is executed. The flow is: scheduleLocationChange -> schedule -> startTimer (case ScheduledRedirection::redirection:) -> frameLoader->clientRedirected() -> FrameLoaderClientQt::dispatchWillPerformClientRedirect -> Which is not implemented. Would this be what is causing this? I saw that Darin implemented that method for Mac. Darin do you have any idea to what is going on, or how I can implement this for Qt. What am I supposed to do in this method? 164646 3 kenneth 2009-11-18 12:02:06 -0800 This is being called by FrameLoader::requestFrame btw. > scheduleLocationChange -> > schedule -> > startTimer (case ScheduledRedirection::redirection:) -> > frameLoader->clientRedirected() -> > FrameLoaderClientQt::dispatchWillPerformClientRedirect -> Which after all the above will call frame->script()->executeIfJavaScriptURL(scriptURL); 164648 4 kenneth 2009-11-18 12:08:30 -0800 So apparently the replaceDocument is the culprit, that maybe somehow removes the layoutTestController that we added in DRT::initJSObjects. 79 // FIXME: We should always replace the document, but doing so 80 // synchronously can cause crashes: 81 // http://bugs.webkit.org/show_bug.cgi?id=16782 82 if (replaceDocument)· 83 m_frame->loader()->replaceDocument(scriptResult); 164662 5 kenneth 2009-11-18 12:29:17 -0800 the root of the cause is 806 clear(resetScripting, resetScripting); in FrameLoader, called from FrameLoader::begin() (called from replaceDocument()). Changing this to clear(false, resetScripting); makes Qt work. The first argument is "bool clearWindowProperties", so that is where the problem lies. Now we just need to find the right fix. 164669 6 kenneth 2009-11-18 12:38:59 -0800 After clearning the window properties, we are calling our void QWebFrame::addToJavaScriptWindowObject ( const QString & name, QObject * object ) but apparently it is not succeeding in adding the properties? We have the following code in the DRT: void DumpRenderTree::initJSObjects() { QWebFrame *frame = qobject_cast<QWebFrame*>(sender()); Q_ASSERT(frame); frame->addToJavaScriptWindowObject(QLatin1String("layoutTestController"), m_controller); frame->addToJavaScriptWindowObject(QLatin1String("eventSender"), m_eventSender); frame->addToJavaScriptWindowObject(QLatin1String("textInputController"), m_textInputController); frame->addToJavaScriptWindowObject(QLatin1String("GCController"), m_gcController); } so I guess that the layoutTestController should have been exported, and it was in the other cases. Any ideas? 164672 7 kenneth 2009-11-18 12:44:22 -0800 For reference this is out implementation of addToJavaScriptWindowObject. Does anyone spot any obvious bug? 446 void QWebFrame::addToJavaScriptWindowObject(const QString &name, QObject *object, QScriptEngine::ValueOwnership ownership) 447 { 448 if (!page()->settings()->testAttribute(QWebSettings::JavascriptEnabled)) 449 return; 450 451 JSC::JSLock lock(JSC::SilenceAssertionsOnly); 452 JSDOMWindow* window = toJSDOMWindow(d->frame, mainThreadNormalWorld()); 453 JSC::Bindings::RootObject* root = d->frame->script()->bindingRootObject(); 454 if (!window) { 455 qDebug() << "Warning: couldn't get window object"; 456 return; 457 } 458 459 JSC::ExecState* exec = window->globalExec(); 460 461 JSC::JSObject* runtimeObject = 462 JSC::Bindings::QtInstance::getQtInstance(object, root, ownership)->createRuntimeObject(exec); 463 464 JSC::PutPropertySlot slot; 465 window->put(exec, JSC::Identifier(exec, (const UChar *) name.constData(), name.length()), runtimeObject, slot); 466 } It seems to get the window from the WebCore::Frame and then to add the properties (line 464-465). 165024 8 kenneth 2009-11-19 10:54:57 -0800 More info: All the window->put(exec, JSC::Identifier(exec, (const UChar *) name.constData(), name.length()), runtimeObject, slot); executed ends up in: void JSObject::put(ExecState* exec, const Identifier& propertyName, JSValue value, PutPropertySlot& slot) { ... // Check if there are any setters or getters in the prototype chain JSValue prototype; for (JSObject* obj = this; !obj->structure()->hasGetterSetterProperties(); obj = asObject(prototype)) { prototype = obj->prototype(); if (prototype.isNull()) { putDirectInternal(exec->globalData(), propertyName, value, 0, true, slot); return; } } ... } And end up in the return there. I have checked the propertyName, and they are are correct and the value is an object (checked with isObject()). putDirectInternal is thus called. I'm debugging that method right now, but I would like to know if if is the right thing to do. 165027 9 kenneth 2009-11-19 11:07:48 -0800 (In reply to comment #8) > putDirectInternal is thus called. I'm debugging that method right now, but I > would like to know if if is the right thing to do. in putDirectInternal, all gets to the end: if (!specificFunction) return; slot.setNewProperty(this, offset); and slot.setNewProperty(this, offset); is executed (specificFunction is 0 in all cases). 165056 10 kenneth 2009-11-19 12:32:40 -0800 I looked at the clearWindowProperties again, and outcommenting windowShell->setWindow(m_frame->domWindow()); in ScriptController::clearWindowShell() make it work. so why? I can confirm that windowShell->window() is different from m_frame->domWindow() Actually one is a JSDOMWindow* and the other is a DOMWindow. Shouldn't we set a JSDOMWindow? As a test (not knowing this code well) I tried: windowShell->setWindow(toJSDOMWindow(m_frame, mainThreadCurrentWorld())); and that actually worked. So, now I need an expert to chime in :-) 165083 11 kenneth 2009-11-19 13:56:39 -0800 (In reply to comment #10) > I looked at the clearWindowProperties again, and outcommenting > > windowShell->setWindow(m_frame->domWindow()); > > in ScriptController::clearWindowShell() make it work. so why? > > I can confirm that windowShell->window() is different from m_frame->domWindow() > > Actually one is a JSDOMWindow* and the other is a DOMWindow. Shouldn't we set a > JSDOMWindow? > > As a test (not knowing this code well) I tried: > > windowShell->setWindow(toJSDOMWindow(m_frame, mainThreadCurrentWorld())); > > and that actually worked. > > So, now I need an expert to chime in :-) Disregard this. When windowShell->setWindow(m_frame->domWindow()); is called a new JSDOMWindow is created and this is the window we get in addToJavaScriptWindowObject. The above confirms that if we create a new one, it breaks, if we reuse the old one, it works. Nothing more than that. 165326 12 loki 2009-11-20 09:17:15 -0800 The root of the problem is that the QtInstance is destroyed before execution of main JS is finished. The following backtrace is produced before cti_op_get_by_id_method_check is called for notifyDone(). backtrace: #0 ~QtInstance (this=0x8159910) at ../../../WebCore/bridge/qt/qt_instance.cpp:93 #1 0xb697edbd in WTF::RefCounted<JSC::Bindings::Instance>::deref (this=0x8159914) at ../../../JavaScriptCore/wtf/RefCounted.h:109 #2 0xb69aa4de in WTF::RefPtr<JSC::Bindings::Instance>::operator= (this=0xb1e456ec, optr=0x0) at ../../../JavaScriptCore/wtf/RefPtr.h:132 #3 0xb69ab27c in JSC::RuntimeObjectImp::invalidate (this=0xb1e456c0) at ../../../WebCore/bridge/runtime_object.cpp:67 #4 0xb69b2c21 in JSC::Bindings::RootObject::invalidate (this=0x8159128) at ../../../WebCore/bridge/runtime_root.cpp:102 #5 0xb69893a5 in WebCore::ScriptController::clearScriptObjects (this=0x80f8f1c) at ../../../WebCore/bindings/js/ScriptController.cpp:437 #6 0xb6d76683 in WebCore::FrameLoader::clear (this=0x80f8cac, clearWindowProperties=true, clearScriptObjects=true, clearFrameView=true) at ../../../WebCore/loader/FrameLoader.cpp:723 #7 0xb6d76f99 in WebCore::FrameLoader::begin (this=0x80f8cac, url=@0x80f8dbc, dispatch=true, origin=0x810b460) at ../../../WebCore/loader/FrameLoader.cpp:803 #8 0xb6d77b82 in WebCore::FrameLoader::replaceDocument (this=0x80f8cac, html=@0xbfb717cc) at ../../../WebCore/loader/FrameLoader.cpp:678 #9 0xb699f599 in WebCore::ScriptController::executeIfJavaScriptURL (this=0x80f8f1c, url=@0xbfb71888, userGesture=false, replaceDocument=true) at ../../../WebCore/bindings/ScriptControllerBase.cpp:83 #10 0xb6d8448d in WebCore::FrameLoader::requestFrame (this=0x80ba994, ownerElement=0x80f8290, urlString=@0x80f82d0, frameName=@0x80f82d4) at ../../../WebCore/loader/FrameLoader.cpp:371 If the Instance is destroyed, we will not able to do the lookup in JSC::RuntimeObjectImp::getOwnPropertySlot. So, we should do something similar in bool FrameLoader::requestFrame what we did for bool FrameLoader::requestFrame. - if (!scriptURL.isEmpty()) - frame->script()->executeIfJavaScriptURL(scriptURL); + if (!scriptURL.isEmpty()) { + m_isExecutingJavaScriptFormAction = true; + frame->script()->executeIfJavaScriptURL(scriptURL, false, false); + m_isExecutingJavaScriptFormAction = false; + } 165327 13 loki 2009-11-20 09:19:42 -0800 (In reply to comment #12) > what we did for bool FrameLoader::requestFrame. Err, FrameLoader::submitForm 165332 14 kenneth 2009-11-20 09:30:08 -0800 So it seems that 16 setTimeout(function () 17 { 18 test(); 19 if (window.layoutTestController) 20 layoutTestController.notifyDone(); 21 22 }, 0); is execute on the old DOMWindow, which is being replaced by test() (frameElement.src = ). Right loki? 165555 15 tonikitoo 2009-11-20 19:35:00 -0800 @keneth, nice investigation. @gabor, good catch. @andras, this is the exactly root of the CONSOLE MESSAGE error we were seeing ... (In reply to comment #12) > The root of the problem is that the QtInstance is destroyed before execution of > main JS is finished. > > The following backtrace is produced before cti_op_get_by_id_method_check is > called for notifyDone(). > > backtrace: > #0 ~QtInstance (this=0x8159910) at > ../../../WebCore/bridge/qt/qt_instance.cpp:93 > #1 0xb697edbd in WTF::RefCounted<JSC::Bindings::Instance>::deref > (this=0x8159914) at ../../../JavaScriptCore/wtf/RefCounted.h:109 > #2 0xb69aa4de in WTF::RefPtr<JSC::Bindings::Instance>::operator= > (this=0xb1e456ec, optr=0x0) at ../../../JavaScriptCore/wtf/RefPtr.h:132 > #3 0xb69ab27c in JSC::RuntimeObjectImp::invalidate (this=0xb1e456c0) at > ../../../WebCore/bridge/runtime_object.cpp:67 > #4 0xb69b2c21 in JSC::Bindings::RootObject::invalidate (this=0x8159128) at > ../../../WebCore/bridge/runtime_root.cpp:102 > #5 0xb69893a5 in WebCore::ScriptController::clearScriptObjects > (this=0x80f8f1c) at ../../../WebCore/bindings/js/ScriptController.cpp:437 > #6 0xb6d76683 in WebCore::FrameLoader::clear (this=0x80f8cac, > clearWindowProperties=true, clearScriptObjects=true, clearFrameView=true) at > ../../../WebCore/loader/FrameLoader.cpp:723 > #7 0xb6d76f99 in WebCore::FrameLoader::begin (this=0x80f8cac, url=@0x80f8dbc, > dispatch=true, origin=0x810b460) at ../../../WebCore/loader/FrameLoader.cpp:803 > #8 0xb6d77b82 in WebCore::FrameLoader::replaceDocument (this=0x80f8cac, > html=@0xbfb717cc) at ../../../WebCore/loader/FrameLoader.cpp:678 > #9 0xb699f599 in WebCore::ScriptController::executeIfJavaScriptURL > (this=0x80f8f1c, url=@0xbfb71888, userGesture=false, replaceDocument=true) at > ../../../WebCore/bindings/ScriptControllerBase.cpp:83 > #10 0xb6d8448d in WebCore::FrameLoader::requestFrame (this=0x80ba994, > ownerElement=0x80f8290, urlString=@0x80f82d0, frameName=@0x80f82d4) at > ../../../WebCore/loader/FrameLoader.cpp:371 > > If the Instance is destroyed, we will not able to do the lookup in > JSC::RuntimeObjectImp::getOwnPropertySlot. > > So, we should do something similar in bool FrameLoader::requestFrame what we > did for bool FrameLoader::requestFrame. > > - if (!scriptURL.isEmpty()) > - frame->script()->executeIfJavaScriptURL(scriptURL); > + if (!scriptURL.isEmpty()) { > + m_isExecutingJavaScriptFormAction = true; > + frame->script()->executeIfJavaScriptURL(scriptURL, false, false); > + m_isExecutingJavaScriptFormAction = false; > + } 166026 16 kenneth 2009-11-24 02:54:27 -0800 (In reply to comment #12) > The root of the problem is that the QtInstance is destroyed before execution of > main JS is finished. > So, we should do something similar in bool FrameLoader::requestFrame what we > did for bool FrameLoader::requestFrame. > > - if (!scriptURL.isEmpty()) > - frame->script()->executeIfJavaScriptURL(scriptURL); > + if (!scriptURL.isEmpty()) { > + m_isExecutingJavaScriptFormAction = true; > + frame->script()->executeIfJavaScriptURL(scriptURL, false, false); > + m_isExecutingJavaScriptFormAction = false; > + } Actually the problem is a bit bigger than this. Adding the frame->script()->executeIfJavaScriptURL(scriptURL, false, false); instead, will just make it not replace the frame at all, meaning that the test would break. The right behaviour is to replace the frame, but the thing is that since we are doing that from javascript, we loose the context from where we are executing it, as I believe it is tied to the frame. I guess the right fix would be to not destroy that context until we are finished executing javascript, but how to find out that is probably a bit complicated. 168387 17 abecsi 2009-12-03 06:32:54 -0800 Involved failing tests skipped in r51635. 174792 18 robert 2009-12-24 11:32:30 -0800 (In reply to comment #17) > Involved failing tests skipped in r51635. Also affects: fast/events/pageshow-pagehide-on-back-cached-with-frames.html fast/events/pageshow-pagehide-on-back-cached.html 174966 19 robert 2009-12-26 12:31:26 -0800 This looks like a slightly related one: LayoutTests/http/tests/misc/set-window-opener-to-null.html It fails at: if (window.layoutTestController) { child_window_opened = layoutTestController.globalFlag; That is, window.layoutTestController.globalFlag remains false but w.layoutTestController.globalFlag is true (opening http://127.0.0.1:8000/misc/resources/content-iframe.html sets it to true). Changing the above lines to: if (w.layoutTestController) { child_window_opened = w.layoutTestController.globalFlag; allows the test to pass. Full text of test below: -- <html> <script> if (window.layoutTestController) { layoutTestController.setCanOpenWindows(); layoutTestController.dumpAsText(); layoutTestController.waitUntilDone(); layoutTestController.globalFlag = false; } var w = window.open("http://127.0.0.1:8000/misc/resources/content-iframe.html"); w.opener = null; function wait_to_start() { var child_window_opened; // polling layoutTestController.globalFlag or w.document if (window.layoutTestController) { child_window_opened = layoutTestController.globalFlag; } else { // polling w.document is not very reliable cross browsers, // it works in chrome for manual testsing. // We might want to change it to using postMessage when it // is supported. child_window_opened = !w.document; } if (!child_window_opened) { setTimeout(wait_to_start, 30); return; } var e = document.getElementById('console'); e.innerHTML = w.opener == null ? 'PASS' : 'FAIL'; w.close(); if (window.layoutTestController) layoutTestController.notifyDone(); } window.onload = wait_to_start; </script> <body> This tests that following code works in Chrome: <pre> var w = window.open(...); w.opener = null; </pre> After new page finishes loading, its opener should stay as null. 235945 20 robert 2010-06-09 12:43:49 -0700 There is a potential solution to this bug posted for review at: https://bugs.webkit.org/show_bug.cgi?id=37725 Both bugs are caused by the fact that ScriptController currently destroys a page's Qt bindings objects when navigating away from it. The solution proposed is to make a special case for such objects by tracking them in a root object that persists between navigations within a frame and only gets destroyed with the frame. 237304 21 robert 2010-06-12 10:06:18 -0700 The affected test was fixed by http://trac.webkit.org/changeset/61062 OK to close? 237322 22 kenneth 2010-06-12 11:26:34 -0700 Thanks Robert 239312 23 hausmann 2010-06-17 02:59:06 -0700 *** This bug has been marked as a duplicate of bug 37725 ***