[Gtk] libsoup critical warning in media player http cookies injection code

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

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

[Philippe Normand]

Created attachment 47852 [details]
refactored HTTP headers injection code

[Holger Freyther]

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.

[Philippe Normand]

Created attachment 47913 [details]
refactored HTTP headers injection code

Now using GOwnPtr and avoiding goto usage.

[WebKit Review Bot]

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.

[Philippe Normand]

Created attachment 47915 [details]
refactored HTTP headers injection code

Fixed style bot warning and removed un-needed new scope.

[Philippe Normand]

Created attachment 47916 [details]
refactored HTTP headers injection code

updated Changelog entry

[Gustavo Noronha (kov)]

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.

[Philippe Normand]

Created attachment 47929 [details]
refactored HTTP headers injection code

As asked by Gustavo I moved the GOwnPtr added bits to a new GOwnPtrGtk module.

[Gustavo Noronha (kov)]

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.

[WebKit Commit Bot]

Comment on attachment 47929 [details]
refactored HTTP headers injection code

Clearing flags on attachment: 47929

Committed r54261: <http://trac.webkit.org/changeset/54261>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gustavo Noronha (kov)]

I had to revert the patch, because my release build started crashing, as did tests in the release bot. Reopenning.

[Philippe Normand]

Created attachment 48002 [details]
refactored HTTP headers injection code

Limit injection to elements dealing with HTTP(S) uris.

[Philippe Normand]

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 :(

[Gustavo Noronha (kov)]

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.

[Philippe Normand]

Commited as r54330. Thanks!