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
I added fast/dom/nodesFromRect-basic.html to the Skipped list again until fix this bug: http://trac.webkit.org/changeset/71028
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.
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.
Created attachment 72549 [details] Patch
Comment on attachment 72549 [details] Patch Nice fix. I am surprised! :)
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!
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?
Attachment 72549 [details] did not build on qt: Build output: http://queues.webkit.org/results/4895001
Created attachment 72559 [details] Patch
(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.
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.
Created attachment 72851 [details] Patch
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.
Created attachment 72853 [details] Patch
Comment on attachment 72853 [details] Patch My comments from the original patch still apply.
Created attachment 72855 [details] Patch
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
Created attachment 72969 [details] Patch
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 on attachment 72969 [details] Patch Clearing flags on attachment: 72969 Committed r71354: <http://trac.webkit.org/changeset/71354>
All reviewed patches have been landed. Closing bug.