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.
Changing frameElement.src to refer an actual file, actually works, so it seems like an issue with javascript: navigations.
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?
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);
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);
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.
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?
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).
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.
(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).
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 :-)
(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.
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; + }
(In reply to comment #12) > what we did for bool FrameLoader::requestFrame. Err, FrameLoader::submitForm
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?
@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; > + }
(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.
Involved failing tests skipped in r51635.
(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
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.
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.
The affected test was fixed by http://trac.webkit.org/changeset/61062 OK to close?
Thanks Robert
*** This bug has been marked as a duplicate of bug 37725 ***