Bug 20940 – [GTK] use of confirm dialog (yes/no) causes segfault

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Luke Kenneth Casson Leighton]

the gboolean pointer into which the yes/no result is to be placed
appears to be corrupt - see attached gdb typescript.

[Luke Kenneth Casson Leighton]

Created attachment 23570 [details]
gdb trace, disas and $rip

[Jan Alonzo]

Created attachment 28472 [details]
we marshal the didConfirm parameter to a boolean not to a pointer

[Holger Freyther]

aeh, didConfirm looks like an out pointer (on first sight), are you sure about this change vs. changing the marshaling?

[Christian Dywan]

> -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.

[Jan Alonzo]

Created attachment 28487 [details]
change the marshaller instead

[Jan Alonzo]

Landed in r41706. Thanks!

[Johan Dahlin]

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

[Christian Dywan]

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