Bug 31626 – [Qt] layoutTestController.notifyDone() not working when the frame was reloaded.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

Changing frameElement.src to refer an actual file, actually works, so it seems like an issue with javascript: navigations.

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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);

[Kenneth Rohde Christiansen]

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);

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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).

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

(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).

[Kenneth Rohde Christiansen]

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 :-)

[Kenneth Rohde Christiansen]

(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.

[Gabor Loki]

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;
+    }

[Gabor Loki]

(In reply to comment #12)
> what we did for bool FrameLoader::requestFrame.
Err, FrameLoader::submitForm

[Kenneth Rohde Christiansen]

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?

[Antonio Gomes]

@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;
> +    }

[Kenneth Rohde Christiansen]

(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.

[Andras Becsi]

Involved failing tests skipped in r51635.

[Robert Hogan]

(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

[Robert Hogan]

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.

[Robert Hogan]

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.

[Robert Hogan]

The affected test was fixed by http://trac.webkit.org/changeset/61062

OK to close?

[Kenneth Rohde Christiansen]

Thanks Robert

[Simon Hausmann]

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