280742 - ignore_host GCONF key is not read in firefox proxy settings

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Ganesh]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041109 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041109 Firefox/1.0

ignore_host GCONF key is not read in firefox proxy settings

Reproducible: Always

Steps to Reproduce:
1.Set somethign in ignore_hosts key in gconf-editor
2.Open firefox preferences
3.

Actual Results:  
Values are not read

Expected Results:  
Values should be read.

Comment 1

[Ganesh]

Attachment: patch for this (aviary)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is this just for Aviary? Because the trunk seems to handle no_proxies_on
already, but in a different way. Is the trunk correct?

I don't think we want this for the Aviary 1.0.x branch; that branch is mainly
for security work.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is this just for Aviary? Because the trunk seems to handle no_proxies_on
already, but in a different way. Is the trunk correct?

I don't think we want this for the Aviary 1.0.x branch; that branch is mainly
for security work.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

hmm, Ganesh may know better. But for me it seems the patch on the trunk was
incomplete (and wrong?) in that respect.
One question is if "/system/proxy/ignore_hosts" or
"/system/http_proxy/ignore_hosts" is correct.

Comment 5

[Ganesh]

Attachment: updated diff (aviary-branch)

The patch in the trunk doesn't seem to handle the ignore_hosts well. Firest of
all the ignore_hosts key location isn't correct and secondly it doesn't seem to
convert the list GCONF preference to sting.
The current patch does it. I am including a new diff as I have missed out to
take a diff of nsSystemPref.cpp in the earlier one.
This diff I took form firefox 1.0 source I would pull the latest mozilla trunk
and attach one if you like.

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

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

Comment 7

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch for trunk

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

Attachment: next try for trunk

first part of the prior patch isn't needed on trunk

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

+        gchar *tmp=(gchar *)gslist->next->data;
+        str=(gchar *)g_realloc(str,strlen(str)+strlen(tmp+1));
+	strcat(str,",");
+	strcat(str,tmp);
+	gslist=gslist->next;

can we use some string class here? I mean, even glib offers string helper
functions, does it not
(http://developer.gnome.org/doc/API/2.0/glib/glib-Strings.html)? although using
mozilla's C++ string classes is probably better.

(I the calculation here is too short. it forgets the terminating zero, the
comma, and due to tmp+1 the length is one further character too short)

Comment 10

[timeless]

Comment on attachment 173243 [details] [diff] [review]
next try for trunk

or
+	 str=(gchar *)g_realloc(str,strlen(str)+strlen(tmp+1));
+	strcat(str,",");

g_realloc just fails and you strcat onto NULL and *CRASH*.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, we should definitely use a real string class here, preferably Mozilla's.

Also the code needs to be reindented. Thanks...

Comment 12

[Ganesh]

Attachment: using gstring function (trunk patch)

This patch uses the gstring functions. 
I don't see we are converting the string to mozilla strings(in
nsSystemPrefService.cpp) in case of other char prefs.

Comment 13

[timeless]

Comment on attachment 173341 [details] [diff] [review]
using gstring function (trunk patch)

>+      GString *g_string=g_string_new((gchar *)gslist->data);
>+      while(gslist->next)
>+           {
>+           g_string_append_c(g_string,',');

this will *still* crash when g_string_new fails.

Comment 14

[Ganesh]

Attachment: Patch based on review comments

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That creates no-proxy strings with a leading comma, e.g.

,blah.com,foo.com

That might work but it'd be cleaner to not add a comma in the first iteration.

Also, could you indent properly here?

+    else
+    {
+    GSList* gslist =
GConfClientGetList(mGConfClient,MozKey2GConfKey(aMozKey),GCONF_VALUE_STRING,NULL);

thanks

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 173587 [details] [diff] [review]
Patch based on review comments

it looks like the style of the file is to put the braces on the same line of
the if and else...

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah but "GSList* gslist = ..." still needs to be indented.

Comment 18

[Ganesh]

Roc, I don't think the proxy string would get created with leading comma. Say if
there is only one entry int eh list it won't even bother about get into the
while loop to append.
I will look into the indentation stuff. I see int eh file I attached it is fine
but when I copy and paste it here it get garbled.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My mistake. Sorry about that. It looks good.

Make sure that your editor inserts spaces and never tabs. Tabs are evil...

Comment 20

[Ganesh]

Attachment: corrected indentation

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 173721 [details] [diff] [review]
corrected indentation

Not sure who should review this ... blizzard?

Comment 22

[Christopher Blizzard (:blizzard)]

Comment on attachment 173721 [details] [diff] [review]
corrected indentation

>+    if (strcmp (aMozKey, "network.proxy.no_proxies_on") != 0) { 
>     gchar *str = GConfClientGetString(mGConfClient,
>                                       MozKey2GConfKey(aMozKey), NULL);
>     if (str) {
>         *retval = PL_strdup(str);
>         g_free(str);
>     }
>+    }
>+    else
>+    {

[ code removed ]

>+        }
>+      }
>+    }
>     return NS_OK;
> }

The indentation for the if is wrong here.  You need to indent the code.  You've
also got an else before return.  Good pattern to use in this function is:

if (foo) {
    // do stuff
    return NS_OK;
}

// else code as it exists now
return NS_OK;

Comment 23

[Ganesh]

Attachment: second try based on comments

Comment 24

[Wolfgang Rosenauer [:wolfiR]]

Ganesh, please ask for review again to get it approved.

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 173926 [details] [diff] [review]
second try based on comments

Sorry, I thought I sent this to Chris weeks ago. =/

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 173926 [details] [diff] [review]
second try based on comments

Please also attach a diff without the -w flag

>@@ -697,6 +705,24 @@
>     }
>     return NS_OK;
> }
>+    GSList* gslist = GConfClientGetList(mGConfClient,MozKey2GConfKey(aMozKey),GCONF_VALUE_STRING,NULL);
>+    if(gslist){ 
>+      GString *g_string=g_string_new((gchar *)gslist->data);

Please assert that gslist->data is in fact a GCONF_VALUE_STRING?

>+      if (g_string == nsnull)
>+            return NS_ERROR_OUT_OF_MEMORY;
>+      while(gslist->next)
>+           {

I realize this is a diff -w, but the whitespace is still extremely goofy here. 
Please fix it to use K&R style instead of what you are using (whitesmiths?)

>+           g_string_append_c(g_string,',');
>+           g_string_append(g_string,(gchar *)gslist->next->data);
>+           gslist=gslist->next;
>+           }
>+      if (g_string) {

We returned early already if g_string was null.  No need to check again here.

>+         *retval = PL_strdup(g_string->str);
>+         g_string_free(g_string,TRUE);
>+      }
>+    }

You're leaking a bunch of stuff.  Everything gchar member in the GSList must be
freed with g_free().  The GSList itself must then be freed with g_slist_free().

Comment 27

[Alfred Peng]

Attachment: patch for trunk

This patch has fixed the similar bug in our internal Mozilla1.7 build.
Since we connect to the internet through internal proxy, I don't really know how to reproduce the bug on Firefox. If you have the exact reproduce steps for this bug, I can do some further test for this patch.

Comment 28

[Alfred Peng]

Attachment: patch v2 for trunk

Improve on the previous patch. Build and verify on Solaris platform.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 206288 [details] [diff] [review]
patch v2 for trunk

I should let you know that I have a new gconf prefs backend that fixes a lot of problems in the current code and adds a lot more preference mappings.

Comment 30

[Alfred Peng]

That will be great. I'm also thinking about adding some system prefs function for some other platforms. Where can I get the new gconf prefs backend? Or is there a bug for it?
timeless, could you help me check the patch into trunk? Thanks.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Created bug 321315. Please let any Sun people know who might be affected.

Comment 32

[timeless]

Comment on attachment 206288 [details] [diff] [review]
patch v2 for trunk

mozilla/extensions/pref/system-pref/src/gconf/nsSystemPrefService.cpp 	1.9
mozilla/extensions/pref/system-pref/src/gconf/gconf_pref_list.inc 	1.4