Bug 20940 - [GTK] use of confirm dialog (yes/no) causes segfault
Summary: [GTK] use of confirm dialog (yes/no) causes segfault
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: Gtk
Depends on:
Blocks:
 
Reported: 2008-09-19 09:57 PDT by Luke Kenneth Casson Leighton
Modified: 2009-03-15 12:49 PDT (History)
1 user (show)

See Also:


Attachments
gdb trace, disas and $rip (12.03 KB, text/plain)
2008-09-19 09:57 PDT, Luke Kenneth Casson Leighton
no flags Details
we marshal the didConfirm parameter to a boolean not to a pointer (3.70 KB, patch)
2009-03-11 04:13 PDT, Jan Alonzo
no flags Details | Formatted Diff | Diff
change the marshaller instead (1.89 KB, patch)
2009-03-11 12:26 PDT, Jan Alonzo
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Kenneth Casson Leighton 2008-09-19 09:57:03 PDT
the gboolean pointer into which the yes/no result is to be placed
appears to be corrupt - see attached gdb typescript.
Comment 1 Luke Kenneth Casson Leighton 2008-09-19 09:57:57 PDT
Created attachment 23570 [details]
gdb trace, disas and $rip
Comment 2 Jan Alonzo 2009-03-11 04:13:42 PDT
Created attachment 28472 [details]
we marshal the didConfirm parameter to a boolean not to a pointer
Comment 3 Holger Freyther 2009-03-11 06:41:07 PDT
aeh, didConfirm looks like an out pointer (on first sight), are you sure about this change vs. changing the marshaling?
Comment 4 Christian Dywan 2009-03-11 11:31:10 PDT
> -static gboolean webViewScriptConfirm(WebKitWebView* view, WebKitWebFrame* frame, const gchar* > message, gboolean* didConfirm, gpointer data)
> +static gboolean webViewScriptConfirm(WebKitWebView* view, WebKitWebFrame* frame, const gchar* > message, gboolean didConfirm, gpointer data)
> {
>     fprintf(stdout, "CONFIRM: %s\n", message);
> -    *didConfirm = TRUE;
> +    didConfirm = TRUE;
>    return TRUE;
> }

This is bogus if I may say that. Assigning a value to a local variable doesn't do anything at all. If anything, the marshaller must be wrong.

didConfirm represents the action of the user, ie. did confirm or did not confirm.
Comment 5 Jan Alonzo 2009-03-11 12:26:41 PDT
Created attachment 28487 [details]
change the marshaller instead
Comment 6 Jan Alonzo 2009-03-14 16:15:14 PDT
Landed in r41706. Thanks!
Comment 7 Johan Dahlin 2009-03-15 12:02:48 PDT
(In reply to comment #4)
> > -static gboolean webViewScriptConfirm(WebKitWebView* view, WebKitWebFrame* frame, const gchar* > message, gboolean* didConfirm, gpointer data)
> > +static gboolean webViewScriptConfirm(WebKitWebView* view, WebKitWebFrame* frame, const gchar* > message, gboolean didConfirm, gpointer data)
> > {
> >     fprintf(stdout, "CONFIRM: %s\n", message);
> > -    *didConfirm = TRUE;
> > +    didConfirm = TRUE;
> >    return TRUE;
> > }
> 
> This is bogus if I may say that. Assigning a value to a local variable doesn't
> do anything at all. If anything, the marshaller must be wrong.
> 
> didConfirm represents the action of the user, ie. did confirm or did not
> confirm.
> 

This doesn't look quite right. It won't be possible to support this pattern in language bindings. Why not just use the return value and remove the third argument to the callbacks instead of always returning TRUE?

Comment 8 Christian Dywan 2009-03-15 12:49:34 PDT
(In reply to comment #7)
> (In reply to comment #4)
> > > -static gboolean webViewScriptConfirm(WebKitWebView* view, WebKitWebFrame* frame, const gchar* > message, gboolean* didConfirm, gpointer data)
> > > +static gboolean webViewScriptConfirm(WebKitWebView* view, WebKitWebFrame* frame, const gchar* > message, gboolean didConfirm, gpointer data)
> > > {
> > >     fprintf(stdout, "CONFIRM: %s\n", message);
> > > -    *didConfirm = TRUE;
> > > +    didConfirm = TRUE;
> > >    return TRUE;
> > > }
> > 
> > This is bogus if I may say that. Assigning a value to a local variable doesn't
> > do anything at all. If anything, the marshaller must be wrong.
> > 
> > didConfirm represents the action of the user, ie. did confirm or did not
> > confirm.
> > 
> 
> This doesn't look quite right. It won't be possible to support this pattern in
> language bindings. Why not just use the return value and remove the third
> argument to the callbacks instead of always returning TRUE?

Because the return value determines whether the signal was handled or not.