Bug 31626

Summary: [Qt] layoutTestController.notifyDone() not working when the frame was reloaded.
Product: WebKit Reporter: Kenneth Rohde Christiansen <kenneth>
Component: New BugsAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: abarth, abecsi, darin, hausmann, jturcotte, laszlo.gombos, loki, ossy, robert, sam, tonikitoo
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   

Description Kenneth Rohde Christiansen 2009-11-18 05:24:37 PST
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.
Comment 1 Kenneth Rohde Christiansen 2009-11-18 10:43:47 PST
Changing frameElement.src to refer an actual file, actually works, so it seems like an issue with javascript: navigations.
Comment 2 Kenneth Rohde Christiansen 2009-11-18 11:30:12 PST
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?
Comment 3 Kenneth Rohde Christiansen 2009-11-18 12:02:06 PST
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);
Comment 4 Kenneth Rohde Christiansen 2009-11-18 12:08:30 PST
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);
Comment 5 Kenneth Rohde Christiansen 2009-11-18 12:29:17 PST
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.
Comment 6 Kenneth Rohde Christiansen 2009-11-18 12:38:59 PST
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?
Comment 7 Kenneth Rohde Christiansen 2009-11-18 12:44:22 PST
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).
Comment 8 Kenneth Rohde Christiansen 2009-11-19 10:54:57 PST
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.
Comment 9 Kenneth Rohde Christiansen 2009-11-19 11:07:48 PST
(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).
Comment 10 Kenneth Rohde Christiansen 2009-11-19 12:32:40 PST
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 :-)
Comment 11 Kenneth Rohde Christiansen 2009-11-19 13:56:39 PST
(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.
Comment 12 Gabor Loki 2009-11-20 09:17:15 PST
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;
+    }
Comment 13 Gabor Loki 2009-11-20 09:19:42 PST
(In reply to comment #12)
> what we did for bool FrameLoader::requestFrame.
Err, FrameLoader::submitForm
Comment 14 Kenneth Rohde Christiansen 2009-11-20 09:30:08 PST
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?
Comment 15 Antonio Gomes 2009-11-20 19:35:00 PST
@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;
> +    }
Comment 16 Kenneth Rohde Christiansen 2009-11-24 02:54:27 PST
(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.
Comment 17 Andras Becsi 2009-12-03 06:32:54 PST
Involved failing tests skipped in r51635.
Comment 18 Robert Hogan 2009-12-24 11:32:30 PST
(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
Comment 19 Robert Hogan 2009-12-26 12:31:26 PST
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.
Comment 20 Robert Hogan 2010-06-09 12:43:49 PDT
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.
Comment 21 Robert Hogan 2010-06-12 10:06:18 PDT
The affected test was fixed by http://trac.webkit.org/changeset/61062

OK to close?
Comment 22 Kenneth Rohde Christiansen 2010-06-12 11:26:34 PDT
Thanks Robert
Comment 23 Simon Hausmann 2010-06-17 02:59:06 PDT

*** This bug has been marked as a duplicate of bug 37725 ***