Bug 48758

Summary: Stale reference to JSDOMWindow in ScriptController::m_cacheableBindingObject
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, barraclough, commit-queue, ggaren, mjs, noam, robert, timothy, tonikitoo, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Csaba Osztrogonác 2010-11-01 08:20:04 PDT
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
Comment 1 Csaba Osztrogonác 2010-11-01 08:28:42 PDT
I added fast/dom/nodesFromRect-basic.html to the Skipped list again
until fix this bug: http://trac.webkit.org/changeset/71028
Comment 2 Robert Hogan 2010-11-01 12:37:11 PDT
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.
Comment 3 Robert Hogan 2010-11-01 13:12:55 PDT
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.
Comment 4 Robert Hogan 2010-11-01 13:42:22 PDT
Created attachment 72549 [details]
Patch
Comment 5 Antonio Gomes 2010-11-01 13:43:33 PDT
Comment on attachment 72549 [details]
Patch

Nice fix. I am surprised! :)
Comment 6 Robert Hogan 2010-11-01 13:45:48 PDT
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!
Comment 7 Robert Hogan 2010-11-01 13:54:52 PDT
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?
Comment 8 Early Warning System Bot 2010-11-01 13:55:40 PDT
Attachment 72549 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4895001
Comment 9 Robert Hogan 2010-11-01 14:18:31 PDT
Created attachment 72559 [details]
Patch
Comment 10 Robert Hogan 2010-11-01 14:19:00 PDT
(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.
Comment 11 Robert Hogan 2010-11-02 16:50:00 PDT
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.
Comment 12 Robert Hogan 2010-11-03 12:54:26 PDT
Created attachment 72851 [details]
Patch
Comment 13 Darin Adler 2010-11-03 12:58:33 PDT
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.
Comment 14 Robert Hogan 2010-11-03 13:00:30 PDT
Created attachment 72853 [details]
Patch
Comment 15 Darin Adler 2010-11-03 13:03:40 PDT
Comment on attachment 72853 [details]
Patch

My comments from the original patch still apply.
Comment 16 Robert Hogan 2010-11-03 13:05:52 PDT
Created attachment 72855 [details]
Patch
Comment 17 WebKit Commit Bot 2010-11-04 01:24:46 PDT
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
Comment 18 Robert Hogan 2010-11-04 11:56:57 PDT
Created attachment 72969 [details]
Patch
Comment 19 WebKit Commit Bot 2010-11-04 13:39:29 PDT
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 abarth@webkit.org.  The commit-queue is continuing to process your patch.
Comment 20 WebKit Commit Bot 2010-11-04 13:42:05 PDT
Comment on attachment 72969 [details]
Patch

Clearing flags on attachment: 72969

Committed r71354: <http://trac.webkit.org/changeset/71354>
Comment 21 WebKit Commit Bot 2010-11-04 13:42:13 PDT
All reviewed patches have been landed.  Closing bug.