Bug 126130 - [GTK] Test /webkit2/WebKitWebContext/languages crashes with network process enabled
Summary: [GTK] Test /webkit2/WebKitWebContext/languages crashes with network process e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 126208 126966
Blocks: 108832
  Show dependency treegraph
 
Reported: 2013-12-22 03:08 PST by Carlos Garcia Campos
Modified: 2014-01-14 09:26 PST (History)
2 users (show)

See Also:


Attachments
Patch (9.47 KB, patch)
2013-12-24 06:37 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (10.13 KB, patch)
2013-12-28 02:26 PST, Carlos Garcia Campos
andersca: review-
Details | Formatted Diff | Diff
Updated patch just removing the confusing comment (10.02 KB, patch)
2014-01-10 01:57 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (7.76 KB, patch)
2014-01-13 10:33 PST, Carlos Garcia Campos
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2013-12-22 03:08:50 PST
$ Programs/WebKit2APITests/TestWebKitWebContext -p /webkit2/WebKitWebContext/languages
/webkit2/WebKitWebContext/languages: OK

$ WEBKIT_USE_NETWORK_PROCESS=1 Programs/WebKit2APITests/TestWebKitWebContext -p /webkit2/WebKitWebContext/languages
/webkit2/WebKitWebContext/languages: Segmentation fault
Comment 1 Carlos Garcia Campos 2013-12-24 06:37:01 PST
Created attachment 219969 [details]
Patch

This patch applies on top of patch attached to bug #126208. It's specific to soup because I don't know if mac needs to also notify the network process about user preferred languages change. Anders, could you confirm it?

$ WEBKIT_USE_NETWORK_PROCESS=1 Programs/WebKit2APITests/TestWebKitWebContext -p /webkit2/WebKitWebContext/languages
/webkit2/WebKitWebContext/languages: OK
Comment 2 Carlos Garcia Campos 2013-12-28 02:26:24 PST
Created attachment 220072 [details]
Updated patch

Just rebased to apply on current git master
Comment 3 Anders Carlsson 2014-01-09 09:28:48 PST
Comment on attachment 220072 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220072&action=review

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:80
> +// This function is based on Epiphany code in ephy-embed-prefs.c.
> +static CString buildAcceptLanguages(Vector<String> languages)

Looking around, it seems that ephy-embed-prefs.c is GPL which is a big no-no. Maybe you can get Xan to relicense it since he seems to be the only copyright holder.
Comment 4 Carlos Garcia Campos 2014-01-09 09:44:23 PST
(In reply to comment #3)
> (From update of attachment 220072 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220072&action=review
> 
> > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:80
> > +// This function is based on Epiphany code in ephy-embed-prefs.c.
> > +static CString buildAcceptLanguages(Vector<String> languages)
> 
> Looking around, it seems that ephy-embed-prefs.c is GPL which is a big no-no. Maybe you can get Xan to relicense it since he seems to be the only copyright holder.

It's true that the code was copied from ephy and then adapted to wk coding style, but the original code in ephy (which is not part of ephy anymore since it's wk2 only) was copied from libsoup, that is LGPL. Dan, could you confirm that piece of code is LGPL? I'll update the comment (or just remove it) to avoid confusions.
Comment 5 Carlos Garcia Campos 2014-01-10 01:57:04 PST
Created attachment 220824 [details]
Updated patch just removing the confusing comment

Removed the comment. The code is actually based on accept_languages_from_system() from libsoup, see:

https://git.gnome.org/browse/libsoup/tree/libsoup/soup-session.c?id=2.45.3#n390
Comment 6 Dan Winship 2014-01-13 10:28:12 PST
yeah, that code was copied into epiphany from libsoup, so it should be fine
Comment 7 Carlos Garcia Campos 2014-01-13 10:31:06 PST
(In reply to comment #6)
> yeah, that code was copied into epiphany from libsoup, so it should be fine

Thanks for confirming it. I've moved that code to a common file in WebCore in another patch so that it's shared by web and network processes instead of having it duplicated. I'll submit a rebased patch.
Comment 8 Carlos Garcia Campos 2014-01-13 10:33:46 PST
Created attachment 221061 [details]
Rebased patch

This is simpler patch now that doesn't need the build accept languages code.
Comment 9 Anders Carlsson 2014-01-13 11:03:57 PST
Comment on attachment 221061 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221061&action=review

> Source/WebKit2/NetworkProcess/NetworkProcess.h:118
> +    void userPreferredLanguagesChanged(Vector<String>);

Should be const Vector<String>&.
Comment 10 Carlos Garcia Campos 2014-01-14 09:26:21 PST
Committed r161976: <http://trac.webkit.org/changeset/161976>