Bug 130492

Summary: [GTK] Running minibrowser with url crashes in debug build
Product: WebKit Reporter: Jae Hyun Park <jaepark>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, commit-queue, fvallee, gns, mario, mrobinson, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch proposal
none
Patch proposal mrobinson: review+, mario: commit-queue-

Description Jae Hyun Park 2014-03-19 18:49:47 PDT
#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()
Comment 1 Mario Sanchez Prada 2014-03-28 08:13:09 PDT
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
Comment 2 Mario Sanchez Prada 2014-03-28 08:13:49 PDT
*** Bug 129606 has been marked as a duplicate of this bug. ***
Comment 3 WebKit Commit Bot 2014-03-28 08:15:56 PDT
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
Comment 4 Mario Sanchez Prada 2014-03-28 09:11:29 PDT
Comment on attachment 228051 [details]
Patch proposal

Thanks!
Comment 5 WebKit Commit Bot 2014-03-28 09:42:29 PDT
Comment on attachment 228051 [details]
Patch proposal

Clearing flags on attachment: 228051

Committed r166410: <http://trac.webkit.org/changeset/166410>
Comment 6 WebKit Commit Bot 2014-03-28 09:42:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Carlos Garcia Campos 2014-03-31 01:30:00 PDT
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")
Comment 8 Mario Sanchez Prada 2014-03-31 06:39:12 PDT
(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.
Comment 9 Carlos Garcia Campos 2014-03-31 06:41:25 PDT
(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
Comment 10 Mario Sanchez Prada 2014-03-31 06:43:05 PDT
Created attachment 228177 [details]
Patch proposal

Patch updating the test case as mentioned in the previous comment. Please review
Comment 11 Mario Sanchez Prada 2014-03-31 09:04:00 PDT
Comment on attachment 228177 [details]
Patch proposal

Thanks for the review. Now setting the cq+ flag
Comment 12 Mario Sanchez Prada 2014-04-01 05:36:08 PDT
Comment on attachment 228177 [details]
Patch proposal

This cq+ did not work. Applying manually...
Comment 13 Mario Sanchez Prada 2014-04-01 05:37:20 PDT
Committed r166577: <http://trac.webkit.org/changeset/166577>