Bug 130492 – [GTK] Running minibrowser with url crashes in debug build

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jae Hyun Park]

#0  0x00007ffff2549fdb in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:333
#1  0x00007ffff2f7f508 in WebCore::URL::URL (this=0x7fffffffcb30, url=...) at ../../Source/WebCore/platform/URL.cpp:331
#2  0x00007ffff247329b in WebCore::ResourceRequest::ResourceRequest (this=0x7fffffffcbc0, url=...) at ../../Source/WebCore/platform/network/soup/ResourceRequest.h:42
#3  0x00007ffff2482337 in webkit_web_view_load_uri (webView=0x6cfd30, uri=0x704730 "http://www.naver.com") at ../../Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2002
#4  0x000000000040ef63 in browser_window_load_uri (window=0x70c2a0, uri=0x6beec0 "http://www.naver.com") at ../../Tools/MiniBrowser/gtk/BrowserWindow.c:801
#5  0x000000000040f09e in createBrowserWindow (uri=0x6c0e50 "http://www.naver.com", webkitSettings=0x681310) at ../../Tools/MiniBrowser/gtk/main.c:74
#6  0x000000000040fa25 in main (argc=1, argv=0x7fffffffce88) at ../../Tools/MiniBrowser/gtk/main.c:286

This crash appeared as of r163037.

Test Scenario.
1. Run Debug MiniBrowser
 > Tools/Script/run-launcher --debug --gtk -2 http://www.google.com
2. Crashes at WebCore::URL::URL() because "url = http://www.google.com" but "m_string = http://www.google.com/"

If we always put "/" at the end of URL, there won't be assertion failure at URL::URL()

[Mario Sanchez Prada]

Created attachment 228051 [details]
Patch proposal

This patch fixes the issue by using SoupURI API to create a URL object to be passed to loadRequest()

Please review

[Mario Sanchez Prada]

*** Bug 129606 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Mario Sanchez Prada]

Comment on attachment 228051 [details]
Patch proposal

Thanks!

[WebKit Commit Bot]

Comment on attachment 228051 [details]
Patch proposal

Clearing flags on attachment: 228051

Committed r166410: <http://trac.webkit.org/changeset/166410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

This broke /webkit2/WebKitWebContext/uri-scheme

/webkit2/WebKitWebContext/uri-scheme: **
ERROR:../../Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:30:void loadChangedCallback(WebKitWebView*, WebKitLoadEvent, LoadTrackingTest*): assertion failed (test->m_activeURI.data() == webkit_web_view_get_uri(webView)): ("echo:hello world" == "echo:hello%20world")

[Mario Sanchez Prada]

(In reply to comment #7)
> This broke /webkit2/WebKitWebContext/uri-scheme
> 
> /webkit2/WebKitWebContext/uri-scheme: **
> ERROR:../../Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:30:void loadChangedCallback(WebKitWebView*, WebKitLoadEvent, LoadTrackingTest*): assertion failed (test->m_activeURI.data() == webkit_web_view_get_uri(webView)): ("echo:hello world" == "echo:hello%20world")

I think the problem here is in the test, not in the code, since "hello world" is not a valid URI (it should be normalized to "hello%20world" to become one) and so that's why the WebView, after this patch landed, is returning a mismatch in comparison to what the test class has stored.

So, unless there's a good reason to support URIs such as "hello world" or "foo bar" in this test, I'd suggest to just update the test to use "hello-world" or "foo-bar" in these two cases, so that we do not need to either artificially unnormalize whatever valid URI comes from webkit_web_view_get_uri() nor to normalize the URIs used in these test cases.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > This broke /webkit2/WebKitWebContext/uri-scheme
> > 
> > /webkit2/WebKitWebContext/uri-scheme: **
> > ERROR:../../Tools/TestWebKitAPI/gtk/WebKit2Gtk/LoadTrackingTest.cpp:30:void loadChangedCallback(WebKitWebView*, WebKitLoadEvent, LoadTrackingTest*): assertion failed (test->m_activeURI.data() == webkit_web_view_get_uri(webView)): ("echo:hello world" == "echo:hello%20world")
> 
> I think the problem here is in the test, not in the code, since "hello world" is not a valid URI (it should be normalized to "hello%20world" to become one) and so that's why the WebView, after this patch landed, is returning a mismatch in comparison to what the test class has stored.

Yes, I think you are right.

> So, unless there's a good reason to support URIs such as "hello world" or "foo bar" in this test, I'd suggest to just update the test to use "hello-world" or "foo-bar" in these two cases, so that we do not need to either artificially unnormalize whatever valid URI comes from webkit_web_view_get_uri() nor to normalize the URIs used in these test cases.

Sounds good to me

[Mario Sanchez Prada]

Created attachment 228177 [details]
Patch proposal

Patch updating the test case as mentioned in the previous comment. Please review

[Mario Sanchez Prada]

Comment on attachment 228177 [details]
Patch proposal

Thanks for the review. Now setting the cq+ flag

[Mario Sanchez Prada]

Comment on attachment 228177 [details]
Patch proposal

This cq+ did not work. Applying manually...

[Mario Sanchez Prada]

Committed r166577: <http://trac.webkit.org/changeset/166577>