Bug 34435 - [Gtk] libsoup critical warning in media player http cookies injection code
Summary: [Gtk] libsoup critical warning in media player http cookies injection code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-01 09:55 PST by Philippe Normand
Modified: 2010-02-04 00:33 PST (History)
6 users (show)

See Also:


Attachments
refactored HTTP headers injection code (4.95 KB, patch)
2010-02-01 10:20 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
refactored HTTP headers injection code (9.73 KB, patch)
2010-02-02 01:52 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
refactored HTTP headers injection code (9.79 KB, patch)
2010-02-02 02:26 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
refactored HTTP headers injection code (9.97 KB, patch)
2010-02-02 02:30 PST, Philippe Normand
gns: review-
Details | Formatted Diff | Diff
refactored HTTP headers injection code (9.53 KB, patch)
2010-02-02 06:02 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
refactored HTTP headers injection code (9.81 KB, patch)
2010-02-03 01:23 PST, Philippe Normand
gns: review+
gns: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-02-01 09:55:05 PST
Trying to play a simple html file containing:

<video autobuffer controls 
                      src="http://movies.apple.com/movies/paramount/ironman2/ironman2-z7r459-tlr1_480p.mov"/>

raises:

(GtkLauncher:3115): libsoup-CRITICAL **: soup_cookie_jar_get_cookies: assertion 
                      `SOUP_IS_COOKIE_JAR (jar)' failed
Comment 1 Philippe Normand 2010-02-01 10:20:57 PST
Created attachment 47852 [details]
refactored HTTP headers injection code
Comment 2 Holger Freyther 2010-02-01 23:42:42 PST
I think you should really use the GOwnPtr/GRefPtr. It will avoid you using the goto, it will avoid any leaks in the future and the text size will be exactly the same.
Comment 3 Philippe Normand 2010-02-02 01:52:23 PST
Created attachment 47913 [details]
refactored HTTP headers injection code

Now using GOwnPtr and avoiding goto usage.
Comment 4 WebKit Review Bot 2010-02-02 01:59:33 PST
Attachment 47913 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/gtk/GOwnPtr.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Philippe Normand 2010-02-02 02:26:26 PST
Created attachment 47915 [details]
refactored HTTP headers injection code

Fixed style bot warning and removed un-needed new scope.
Comment 6 Philippe Normand 2010-02-02 02:30:25 PST
Created attachment 47916 [details]
refactored HTTP headers injection code

updated Changelog entry
Comment 7 Gustavo Noronha (kov) 2010-02-02 04:16:56 PST
Comment on attachment 47916 [details]
refactored HTTP headers injection code

 8         * GNUmakefile.am: Added libsoup and gstreamer CFLAGS/LDFLAGS to JavascriptCore build.

This is a bad thing we want to avoid. JSC should not need to depend on those. For GRefPtr mrobinson added files to WebCore/platform/gtk. I think this is the way to go for these GOwnPtr types.

 11         not be done neither the User-Agent not Referer were injected.

s/not/nor/

 142     char* location;
 143     g_object_get(element.get(), "location", &location, NULL);
 144 
 145     GOwnPtr<SoupURI> uri(soup_uri_new(location));
 146     g_free(location);

You could use GOwnPtr for char* location, and the other char*'s in this code, by the way!

r- for the addition of unneeded deps on JSC.
Comment 8 Philippe Normand 2010-02-02 06:02:42 PST
Created attachment 47929 [details]
refactored HTTP headers injection code

As asked by Gustavo I moved the GOwnPtr added bits to a new GOwnPtrGtk module.
Comment 9 Gustavo Noronha (kov) 2010-02-02 15:42:07 PST
Comment on attachment 47929 [details]
refactored HTTP headers injection code

Thanks! I'm going to get this in (by setting cq+) so that it is included in the release.
Comment 10 WebKit Commit Bot 2010-02-02 16:03:32 PST
Comment on attachment 47929 [details]
refactored HTTP headers injection code

Clearing flags on attachment: 47929

Committed r54261: <http://trac.webkit.org/changeset/54261>
Comment 11 WebKit Commit Bot 2010-02-02 16:03:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Gustavo Noronha (kov) 2010-02-02 17:21:22 PST
I had to revert the patch, because my release build started crashing, as did tests in the release bot. Reopenning.
Comment 13 Philippe Normand 2010-02-03 01:23:28 PST
Created attachment 48002 [details]
refactored HTTP headers injection code

Limit injection to elements dealing with HTTP(S) uris.
Comment 14 Philippe Normand 2010-02-03 01:29:05 PST
The build failure happened because of the new order. Before this patch we were checking if the element had a cookies property first, so in case of filesrc this was obviously failing. In the patch of last night we were directly building a SoupUri with the location value, but in case of filesrc the location is not a URI, it is an absolute filesystem path, soup_uri_new() returns NULL in that case, hence the build breakage.

In the updated patch I added checks to ensure the SoupUri is built with a valid http(s) uri. Tests pass fine here.

Sorry for the trouble :(
Comment 15 Gustavo Noronha (kov) 2010-02-03 09:49:51 PST
Comment on attachment 48002 [details]
refactored HTTP headers injection code

 151     // Do injection only for http(s) uris.
 152     if (g_ascii_strncasecmp(uri->scheme, "http", 4)
 153         || g_ascii_strncasecmp(uri->scheme, "https", 5))
 154         return;

I think it's better to use SOUP_URI_VALID_FOR_HTTP() here instead.
Comment 16 Philippe Normand 2010-02-04 00:33:16 PST
Commited as r54330. Thanks!