Bug 48758 – Stale reference to JSDOMWindow in ScriptController::m_cacheableBindingObject

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

fast/dom/nodesFromRect-basic.html fails after r71004 if you execute run-webkit-tests. 
But it passes if you run only this testcase, so it must be a DRT bug.

http://build.webkit.org/results/Qt%20Linux%20Release/r71025%20%2822924%29/fast/dom/nodesFromRect-basic-pretty-diff.html

[Csaba Osztrogonác]

I added fast/dom/nodesFromRect-basic.html to the Skipped list again
until fix this bug: http://trac.webkit.org/changeset/71028

[Robert Hogan]

Not surprisingly, though I don't understand why, this is fixed by the patch at https://bugs.webkit.org/show_bug.cgi?id=42578.

However, I don't think the problem here is QNAM related like the other failures resolved by 42578. So it would be good to understand why the patch at 42578 fixes it.

Some debugging reveals that the NodeList is getting inspected in DumpRenderTreeSupportQt but for some reason the QVariantList passed back to the runtime object contains null values by the time it is inspected in the test. The QVariantList is there, it has the right length, but the values in it are 'undefined'.

To me this suggests a problem with the state of the LayoutTestController runtime object after a new document has been loaded in the same QWebPage. This may also be a problem during normal navigation so it could be a real bug.

[Robert Hogan]

Yes, this is related to: https://bugs.webkit.org/show_bug.cgi?id=37725

Specifically the code that adds the LayoutTestController object:

void QWebFrame::addToJavaScriptWindowObject(const QString &name, QObject *object, QScriptEngine::ValueOwnership ownership)
{
    if (!page()->settings()->testAttribute(QWebSettings::JavascriptEnabled))
        return;
#if USE(JSC)
    JSC::JSLock lock(JSC::SilenceAssertionsOnly);
    JSDOMWindow* window = toJSDOMWindow(d->frame, mainThreadNormalWorld());
    JSC::Bindings::RootObject* root;
    if (ownership == QScriptEngine::QtOwnership)
        root = d->frame->script()->cacheableBindingRootObject();
    else
        root = d->frame->script()->bindingRootObject();

At the moment LayoutTestController gets added as a cacheableBindingRootObject() so that it survives page navigations. This object will only get destroyed once the page has been destroyed. 

If you change it so that LayoutTestController gets added as a bindingRootObject() instead it will get destroyed on the page navigation implicit in loading a new test and node-from-rect-basic.html passes when run as the following tests.

So there is something wrong with the state of the Qt root object after loading an initial page, then loading a new one. This problem does not arise with a 'fresh' root object.

[Robert Hogan]

Created attachment 72549 [details]
Patch

[Antonio Gomes]

Comment on attachment 72549 [details]
Patch

Nice fix. I am surprised! :)

[Robert Hogan]

As stated in the log, I believe the correct method for getting the current active
document in a JS context is from the context's lexicalGlobalObject()rather than the root's globalObject(). This is the way it's done elsewhere in WebCore.

I don't pretend to fully understand the differences between the two - I'm just copying what is used elsewhere and applying it to fix the regression here.

Would be nice if someone could say 'yes, you're right' with some degree of confidence!

[Robert Hogan]

hi Noam,

You added the line I'm amending here in http://trac.webkit.org/changeset/54775

What do you think? Is my change ok?

[Early Warning System Bot]

Attachment 72549 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4895001

[Robert Hogan]

Created attachment 72559 [details]
Patch

[Robert Hogan]

(In reply to comment #8)
> Attachment 72549 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/4895001

Ach, forgot the include when applying the patch to a clean branch.

[Robert Hogan]

Looking at it a bit more this isn't the real fix.

The problem is that after bug 37725 we need to update the globalObject() (i.e. JSDOMWindow) associated with the cached bindingRootObject every time we clear the frame and load a new page. This allows the runtime objects (e.g. LayoutTestController etc.) to persist between navigations as before but also ensures that when qt_runtime looks at root->globalObject() it's referring to the DOMWindow of the current page.

--- a/WebCore/bindings/js/ScriptController.cpp
+++ b/WebCore/bindings/js/ScriptController.cpp
@@ -197,6 +198,9 @@ void ScriptController::clearWindowShell(bool goingIntoPageCache)
         windowShell->window()->willRemoveFromWindowShell();
         windowShell->setWindow(m_frame->domWindow());
 
+        if (m_cacheableBindingRootObject)
+            m_cacheableBindingRootObject->setGlobalObject(windowShell->window());
+
         if (Page* page = m_frame->page()) {
             attachDebugger(windowShell, page->debugger());
             windowShell->window()->setProfileGroup(page->group().identifier());

Hopefully this will still be right tomorrow - but that's what it looks like now, and it certainly fixes the DRT sideeffect as well as allowing the tests in 37725 to continue to pass.

[Robert Hogan]

Created attachment 72851 [details]
Patch

[Darin Adler]

Comment on attachment 72851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72851&action=review

Change seems OK, but I’m not convinced this is all that’s needed.

> WebCore/ChangeLog:8
> +        associated with the m_cacheablebindingRootObject every time we clear the

The m_cacheableBindingRootObject mechanism seems half-implemented and it’s not clear to me that “cacheable” is a good concept for this.

This fixes one bug, but I think there are going to be other problems as well.

Lets not add this in WebKit2 and maybe we can dump it for good eventually.

> WebCore/bridge/runtime_root.h:72
> +    void updateGlobalObject(JSGlobalObject* globalObject);

No need for the argument name here.

[Robert Hogan]

Created attachment 72853 [details]
Patch

[Darin Adler]

Comment on attachment 72853 [details]
Patch

My comments from the original patch still apply.

[Robert Hogan]

Created attachment 72855 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 72855 [details]
Patch

Rejecting patch 72855 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', 72855]" exit_code: 2
Last 500 characters of output:
rsed 5 diffs from patch file(s).
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 5461.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/bindings/js/ScriptController.cpp
patching file WebCore/bridge/runtime_root.cpp
patching file WebCore/bridge/runtime_root.h

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/5208007

[Robert Hogan]

Created attachment 72969 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 72969 [details]:

http/tests/websocket/tests/close-on-navigate-new-location.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 72969 [details]
Patch

Clearing flags on attachment: 72969

Committed r71354: <http://trac.webkit.org/changeset/71354>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.