Bug 43930 – Apparent error in FrameLoaderClientQt::createFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Leo Yang]

frameData.frame->loader()->loadURLIntoChildFrame(frameData.url, frameData.referrer, frameData.frame.get());

Apparent error: calling loadURLIntoChildFrame with child frame's loader.

[Leo Yang]

Created attachment 64262 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 64262 [details]
Patch

How come this having been breaking anything?

[Leo Yang]

(In reply to comment #2)
> (From update of attachment 64262 [details])
> How come this having been breaking anything?

Some logic in loadURLIntoChildFrame is unreachable. We can't run into the following if statement. And if we add some operations which are related with parent frame in loadURLIntoChildFrame in the future, it's buggy for qt porting.

 917     if (parentItem && parentItem->children().size() && isBackForwardLoadType(loadType)) {
 918         HistoryItem* childItem = parentItem->childItemWithTarget(childFrame->tree()->name());
 919         if (childItem) {
 920             // Use the original URL to ensure we get all the side-effects, such as
 921             // onLoad handlers, of any redirects that happened. An example of where
 922             // this is needed is Radar 3213556.
 923             workingURL = KURL(ParsedURLString, childItem->originalURLString());
 924             childLoadType = loadType;
 925             childFrame->loader()->history()->setProvisionalItem(childItem);
 926         }
 927     }

[WebKit Commit Bot]

Comment on attachment 64262 [details]
Patch

Rejecting patch 64262 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20894 test cases.
media/video-preload.html -> crashed

Exiting early after 1 failures. 17255 tests run.
651.04s total testing time

17254 test cases (99%) succeeded
1 test case (<1%) crashed
36 test cases (<1%) had stderr output

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

[Leo Yang]

(In reply to comment #4)
> (From update of attachment 64262 [details])
> Rejecting patch 64262 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
> Running build-dumprendertree
> Compiling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Testing 20894 test cases.
> media/video-preload.html -> crashed
> 
> Exiting early after 1 failures. 17255 tests run.
> 651.04s total testing time
> 
> 17254 test cases (99%) succeeded
> 1 test case (<1%) crashed
> 36 test cases (<1%) had stderr output
> 
> Full output: http://queues.webkit.org/results/3753576

Code changes in the patch are only related to QT port. I tested using QtTestBrowser, it works fine for media/video-preload.html. So is it false positive?

[Eric Seidel (no email)]

Comment on attachment 64262 [details]
Patch

The media tests a very flaky on leopard due to a CoreVideo bug.

[Leo Yang]

(In reply to comment #6)
> (From update of attachment 64262 [details])
> The media tests a very flaky on leopard due to a CoreVideo bug.

Can someone who has commit access help land this patch?

[Eric Seidel (no email)]

The cq will land it shortly.

[WebKit Commit Bot]

Comment on attachment 64262 [details]
Patch

Clearing flags on attachment: 64262

Committed r66030: <http://trac.webkit.org/changeset/66030>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

-    frameData.frame->loader()->loadURLIntoChildFrame(frameData.url, frameData.referrer, frameData.frame.get());
+    m_frame->loader()->loadURLIntoChildFrame(frameData.url, frameData.referrer, frameData.frame.get());
 

That is a great catch!

Did not it fixed anything? If not, no unit tests or regression tests added? :-(

It could happen to be broken again...