[Gtk] crash when closing page from javascript

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Dan Winship]

go to http://mysterion.org/~danw/close.html. click the button on the web page, which just runs "window.close()". Nothing happens. Click it a second time, and the browser crashes.

trace is from epiphany, but it crashes in GtkLauncher too

Program received signal SIGSEGV, Segmentation fault.
0x00f01b80 in webkit_web_view_focus_out_event (widget=0x86d10e8, 
    event=0x8734000) at WebKit/gtk/webkit/webkitwebview.cpp:601
601	    core(webView)->focusController()->setActive(false);
Current language:  auto; currently c++
(gdb) where
#0  0x00f01b80 in webkit_web_view_focus_out_event (widget=0x86d10e8, 
    event=0x8734000) at WebKit/gtk/webkit/webkitwebview.cpp:601
#1  0x03e3ee24 in _gtk_marshal_BOOLEAN__BOXED (closure=0x8165930, 
    return_value=0xbfffd864, n_param_values=2, param_values=0x8702d68, 
    invocation_hint=0xbfffd850, marshal_data=0xf01b40) at gtkmarshalers.c:84
#2  0x00a929b9 in g_type_class_meta_marshal (closure=0x8165930, 
    return_value=0xbfffd864, n_param_values=2, param_values=0x8702d68, 
    invocation_hint=0xbfffd850, marshal_data=0xe4) at gclosure.c:878
#3  0x00a94332 in IA__g_closure_invoke (closure=0x8165930, 
    return_value=0xbfffd864, n_param_values=2, param_values=0x8702d68, 
    invocation_hint=0xbfffd850) at gclosure.c:767
#4  0x00aa969b in signal_emit_unlocked_R (node=<value optimized out>, 
    detail=<value optimized out>, instance=0x86d10e8, 
    emission_return=0xbfffd9ac, instance_and_params=0x8702d68)
    at gsignal.c:3285
#5  0x00aaaba8 in IA__g_signal_emit_valist (instance=0x86d10e8, signal_id=45, 
    detail=0, var_args=0xbfffda10 "L\332\377\277\1") at gsignal.c:2990
#6  0x00aab1a6 in IA__g_signal_emit (instance=0x86d10e8, signal_id=45, 
    detail=0) at gsignal.c:3037
#7  0x03f57bf6 in gtk_widget_event_internal (widget=<value optimized out>, 
    event=0x8734000) at gtkwidget.c:4761
#8  0x03f67731 in do_focus_change (widget=0x86d10e8, in=138938072)
    at gtkwindow.c:5254
#9  0x03f681ec in gtk_window_real_set_focus (window=0x8144428, focus=0x0)
    at gtkwindow.c:5456
#10 0x00aa1118 in IA__g_cclosure_marshal_VOID__OBJECT (closure=0x816dd80, 
    return_value=0x0, n_param_values=2, param_values=0x8701918, 
    invocation_hint=0xbfffdc80, marshal_data=0x3f68040) at gmarshal.c:636
#11 0x00a929b9 in g_type_class_meta_marshal (closure=0x816dd80, 
    return_value=0x0, n_param_values=2, param_values=0x8701918, 
    invocation_hint=0xbfffdc80, marshal_data=0x1a0) at gclosure.c:878
#12 0x00a94332 in IA__g_closure_invoke (closure=0x816dd80, return_value=0x0, 
    n_param_values=2, param_values=0x8701918, invocation_hint=0xbfffdc80)
    at gclosure.c:767
#13 0x00aa969b in signal_emit_unlocked_R (node=<value optimized out>, 
    detail=<value optimized out>, instance=0x8144428, emission_return=0x0, 
    instance_and_params=0x8701918) at gsignal.c:3285
#14 0x00aaad1d in IA__g_signal_emit_valist (instance=0x8144428, signal_id=81, 
    detail=0, var_args=0xbfffde40 "\244\201\250") at gsignal.c:2980
#15 0x00aab1a6 in IA__g_signal_emit (instance=0x8144428, signal_id=81, 
    detail=0) at gsignal.c:3037
#16 0x03f6f01b in _gtk_window_internal_set_focus (window=0x8144428, focus=0x0)
    at gtkwindow.c:1603
#17 0x03f6f10f in IA__gtk_window_set_focus (window=0x8144428, focus=0x0)
    at gtkwindow.c:1591
#18 0x03f6f1e2 in _gtk_window_unset_focus_and_default (window=0x8144428, 
    widget=0x86d10e8) at gtkwindow.c:5538
#19 0x03f66f8f in IA__gtk_widget_unparent (widget=0x86d10e8)
    at gtkwidget.c:2873
#20 0x03d79f03 in gtk_bin_remove (container=0x8465c30, child=0x86d10e8)
    at gtkbin.c:109
#21 0x03e9f9a2 in gtk_scrolled_window_remove (container=0x8465c30, 
    child=0x86d10e8) at gtkscrolledwindow.c:1701
#22 0x00aa1118 in IA__g_cclosure_marshal_VOID__OBJECT (closure=0x816ccc8, 
    return_value=0x0, n_param_values=2, param_values=0x8701190, 
    invocation_hint=0xbfffe130, marshal_data=0x3e9f8f0) at gmarshal.c:636
#23 0x00a929b9 in g_type_class_meta_marshal (closure=0x816ccc8, 
    return_value=0x0, n_param_values=2, param_values=0x8701190, 
    invocation_hint=0xbfffe130, marshal_data=0x170) at gclosure.c:878
#24 0x00a94332 in IA__g_closure_invoke (closure=0x816ccc8, return_value=0x0, 
    n_param_values=2, param_values=0x8701190, invocation_hint=0xbfffe130)
    at gclosure.c:767
#25 0x00aa92b5 in signal_emit_unlocked_R (node=<value optimized out>, 
    detail=<value optimized out>, instance=0x8465c30, emission_return=0x0, 
    instance_and_params=0x8701190) at gsignal.c:3177
#26 0x00aaad1d in IA__g_signal_emit_valist (instance=0x8465c30, signal_id=78, 
    detail=0, var_args=0xbfffe2f0 "") at gsignal.c:2980
#27 0x00aab1a6 in IA__g_signal_emit (instance=0x8465c30, signal_id=78, 
    detail=0) at gsignal.c:3037
#28 0x03db161e in IA__gtk_container_remove (container=0x8465c30, 
    widget=0x86d10e8) at gtkcontainer.c:1233
#29 0x03f616ed in gtk_widget_dispose (object=0x86d10e8) at gtkwidget.c:7898
#30 0x00f029af in webkit_web_view_dispose (object=0x86d10e8)
    at WebKit/gtk/webkit/webkitwebview.cpp:964
#31 0x080c1f0f in ephy_web_view_dispose (object=0x86d10e8)
    at ephy-web-view.c:490
#32 0x00a96418 in IA__g_object_unref (_object=0x86d10e8) at gobject.c:2393
#33 0x03e37819 in IA__gtk_propagate_event (widget=0x86d10e8, event=0x8299198)
    at gtkmain.c:2399
#34 0x03e38ac9 in IA__gtk_main_do_event (event=0x8299198) at gtkmain.c:1601
#35 0x00727eba in gdk_event_dispatch (source=0x814d620, callback=0, 
    user_data=0x0) at gdkevents-x11.c:2367
#36 0x009dfd78 in g_main_dispatch (context=<value optimized out>)
    at gmain.c:1814
#37 IA__g_main_context_dispatch (context=<value optimized out>) at gmain.c:2367
#38 0x009e3310 in g_main_context_iterate (context=0x814d668, 
    block=<value optimized out>, dispatch=1, self=0x81251a0) at gmain.c:2445
#39 0x009e377f in IA__g_main_loop_run (loop=0x812da00) at gmain.c:2653
#40 0x03e39029 in IA__gtk_main () at gtkmain.c:1205
#41 0x0806b3c4 in main (argc=1, argv=0xbffff774) at ephy-main.c:781

[Xan Lopez]

I can't reproduce this, which webkit revision are you using exactly?

[Dan Winship]

latest svn (r46105). it was crashing before that too though.

when you say you can't reproduce, does that mean the window closes the *first* time you click the button? because if not, that may also be a bug. (It doesn't actually close the window in Firefox though, so I'm not sure if there's some sort of "don't let javascript close windows it didn't open" thing going on. However, this is a simplification of another site where a window is opened by clicking on a link and then later closed by javascript in that window, so the closing is definitely correct there.)

does valgrind show anything?

[Jan Alonzo]

(In reply to comment #2)
> latest svn (r46105). it was crashing before that too though.

Hi Dan

> when you say you can't reproduce, does that mean the window closes the *first*
> time you click the button? because if not, that may also be a bug. (It doesn't
> actually close the window in Firefox though, so I'm not sure if there's some
> sort of "don't let javascript close windows it didn't open" thing going on.

Yeah that's what it's actually doing.

> However, this is a simplification of another site where a window is opened by
> clicking on a link and then later closed by javascript in that window, so the
> closing is definitely correct there.)

Is the site public? Do you mind sharing it here if it is?

Thanks.

[Priit Laes (IRC: plaes)]

(In reply to comment #3)
> (In reply to comment #2)
> > latest svn (r46105). it was crashing before that too though.
> 
> > However, this is a simplification of another site where a window is opened by
> > clicking on a link and then later closed by javascript in that window, so the
> > closing is definitely correct there.)
> 
> Is the site public? Do you mind sharing it here if it is?

I also ran into something like this, so I created a minified testcase:
http://plaes.org/files/2009-Q3/webkit-bug-27439/

PS. You can also see that window positioning is not set correctly (at least with ephy)

[Jan Alonzo]

Created attachment 33933 [details]
Patch v1

[Priit Laes (IRC: plaes)]

(In reply to comment #5)
> Created an attachment (id=33933) [details]
> Patch v1

Doesn't crash anymore, but window does not close either :(

Also, real life testcase that this patch seems to fix:

1) Go to http://www.jassi.ee/
2) Click on the map
3) Either crash happens after a click or when you try to close the popup window

[Jan Alonzo]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=33933) [details] [details]
> > Patch v1
> 
> Doesn't crash anymore, but window does not close either :(

Embedders need to listen to close-web-view to either destroy or hide their windows which I don't think ephy does atm.

[Xan Lopez]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created an attachment (id=33933) [details] [details] [details]
> > > Patch v1
> > 
> > Doesn't crash anymore, but window does not close either :(
> 
> Embedders need to listen to close-web-view to either destroy or hide their
> windows which I don't think ephy does atm.

Wouldn't it be with this patch *mandatory* that you handle the signal? Otherwise you'd leak the view object, which used to be unreffed by closeWindowSoon until now. If that's the case you should make the documentation more specific about that.

[Jan Alonzo]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Created an attachment (id=33933) [details] [details] [details] [details]
> > > > Patch v1
> > > 
> > > Doesn't crash anymore, but window does not close either :(
> > 
> > Embedders need to listen to close-web-view to either destroy or hide their
> > windows which I don't think ephy does atm.
> 
> Wouldn't it be with this patch *mandatory* that you handle the signal?

Now it's kinda mandatory. How does a WebView created by create-web-view get freed prior to when this signal was introduced?

> Otherwise you'd leak the view object, which used to be unreffed by
> closeWindowSoon until now. If that's the case you should make the documentation
> more specific about that.

I did.

      *
+     * It is also recommended that #WebKitWebView::close-web-view be handled
+     * to hide the #WebKitWebView.
+     *

Maybe add something like "free the WebView you created in create-web-view"?

[Xan Lopez]

(In reply to comment #9)
> Now it's kinda mandatory. How does a WebView created by create-web-view get
> freed prior to when this signal was introduced?
> 

Well, in the general case it's handled by the client, since it's the client who created the view right? But in the case of the windows destroyed by window.close() the code that emits the signal would destroy the view itself, so in theory you didn't need to do anything (although this is probably too simplistic anyway, since for popups in new windows you'd need to get rid of the toplevel anyway...).

> > Otherwise you'd leak the view object, which used to be unreffed by
> > closeWindowSoon until now. If that's the case you should make the documentation
> > more specific about that.
> 
> I did.
> 
>       *
> +     * It is also recommended that #WebKitWebView::close-web-view be handled
> +     * to hide the #WebKitWebView.
> +     *
> 
> Maybe add something like "free the WebView you created in create-web-view"?

Yeah, I read that, but I meant explaining explicitly that you have to free/hide the view in the callback yourself. 'Recommended' sounds a bit like 'it would be good if you do it, but you don't have to', but I don't think that's the case now?

Also, I think it would be good to have a test (or tests) about this (we can do it after we land the patch though).

[Jan Alonzo]

(In reply to comment #10)
> (In reply to comment #9)
> > Now it's kinda mandatory. How does a WebView created by create-web-view get
> > freed prior to when this signal was introduced?
> > 
> 
> Well, in the general case it's handled by the client, since it's the client who
> created the view right? 

Right.

> But in the case of the windows destroyed by
> window.close() the code that emits the signal would destroy the view itself, so
> in theory you didn't need to do anything (although this is probably too
> simplistic anyway, since for popups in new windows you'd need to get rid of the
> toplevel anyway...).

 
> > Maybe add something like "free the WebView you created in create-web-view"?
> 
> Yeah, I read that, but I meant explaining explicitly that you have to free/hide
> the view in the callback yourself. 'Recommended' sounds a bit like 'it would be
> good if you do it, but you don't have to', but I don't think that's the case
> now?

Right. I'll mention that it's mandatory.
 
> Also, I think it would be good to have a test (or tests) about this (we can do
> it after we land the patch though).

I'll try to come up an updated patch with test in the next couple of days. Thanks for the review.

[Jan Alonzo]

Comment on attachment 33933 [details]
Patch v1

Clearing review. I'll update the patch based on Xan's feedback.

[Jan Alonzo]

Created attachment 34486 [details]
updated patch, no test

[Jan Alonzo]

(In reply to comment #13)
> Created an attachment (id=34486) [details]
> updated patch, no test

I'll add the test when I get more time.

[Jan Alonzo]

Created attachment 34887 [details]
updated signal doc

[Oliver Hunt]

Comment on attachment 34887 [details]
updated signal doc

I realise i'm not a gtk person, but this patch seems obvious enough to me, r=me

[Xan Lopez]

(In reply to comment #16)
> (From update of attachment 34887 [details])
> I realise i'm not a gtk person, but this patch seems obvious enough to me, r=me

Thanks, somehow most of my bugmail of the last days went to the spam folder an I missed this (among other things).

[Eric Seidel (no email)]

Comment on attachment 34887 [details]
updated signal doc

Clearing flags on attachment: 34887

Committed r47493: <http://trac.webkit.org/changeset/47493>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.