Bug 23194 – [GTK] fix crashers

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Benjamin Otte]

Here's fixes for two crashers.

The first is just a simple #if 0 so no two callbacks get called on the policy object, the other fix makes the finalize function the dispose function, so that objects get released at the right time.

[Benjamin Otte]

Created attachment 26531 [details]
suggested fix

[Benjamin Otte]

For reference, here's a paste from IRC discussing the finalize => dispose change.

<Company> http://pastebin.com/m72177544
<Company> what's happening is that the WebView gets last-unreffed which calls g_object_destroy() on it, then finalizes it
<Company> g_object_destroy calls destroy() on all children, which causes the scrollbars to be destroyed, and they free their adjustments
<Company> after that web_view_finalize calls ~Page which causes the stack trace above to appear
<Company> and that sets the destroyed scrollbar's allocation
<Company> and that code uses a range->allocation->value unconditionally
<Company> ergo: boom
<xan> (you mean gtk_object_destroy)
<Company> right
<Company> sorry
<Company> the web view should likely destroy the Page in dispose, not in finalize
<Company> but i don't know the code well enough to know if that's gonna work
<xan> also, just 'in theory', you are supposed to release refs to external objects in dispose, not finalize
<Company> yeah, that's why i'm suggesting it

[Holger Freyther]

Comment on attachment 26531 [details]
suggested fix

Please split this patch into two parts. There is no relationship between the first crash and the second one.



>      webkit_web_policy_decision_ignore(decision);
>  
> +#if 0
>      if (!priv->isCancelled)
>          (core(priv->frame)->loader()->*(priv->framePolicyFunction))(WebCore::PolicyDownload);
> +#endif
>  }

what happens if you just remove the call to webkit_web_policy_decision_ignore? 





>  
>  void webkit_web_policy_decision_cancel(WebKitWebPolicyDecision* decision)
> diff --git a/WebKit/gtk/webkit/webkitwebview.cpp b/WebKit/gtk/webkit/webkitwebview.cpp
> index 91f3b80..d3f7cd7 100644
> --- a/WebKit/gtk/webkit/webkitwebview.cpp
> +++ b/WebKit/gtk/webkit/webkitwebview.cpp
> @@ -807,31 +807,55 @@ static void webkit_web_view_real_paste_clipboard(WebKitWebView* webView)
>      frame->editor()->command("Paste").execute();
>  }
>  
> -static void webkit_web_view_finalize(GObject* object)
> +static void webkit_web_view_dispose(GObject* object)

okay.


> -    webkit_web_view_stop_loading(WEBKIT_WEB_VIEW(object));
> +    if (priv->corePage) {
> +        webkit_web_view_stop_loading(WEBKIT_WEB_VIEW(object));
>  
> -    core(priv->mainFrame)->loader()->detachChildren();
> -    delete priv->corePage;
> +        core(priv->mainFrame)->loader()->detachChildren();
> +        delete priv->corePage;
> +        priv->corePage = 0;
> +    }


alternatively you can write this as
if (priv->mainFrame)
    core(priv->mainFrame)->loader()->detachChildren()
delete priv->corePage;
priv->corePage;


>  
> -    if (priv->horizontalAdjustment)
> +    if (priv->horizontalAdjustment) {
>          g_object_unref(priv->horizontalAdjustment);
> -    if (priv->verticalAdjustment)
> +        priv->horizontalAdjustment = NULL;

Do not use NULL here and at the other occasions.

[Benjamin Otte]

Created attachment 26578 [details]
fix WebView to use dispose

[Benjamin Otte]

Created attachment 26579 [details]
make DOwnload policy decision not fail

[Holger Freyther]

Comment on attachment 26578 [details]
fix WebView to use dispose

Some extra empty lines between if () {} statements would only cost a byte and helps readability. Thanks :)

[Holger Freyther]

Landed in r39775 and r39776.

[Holger Freyther]

Okay, I got fooled on the dispose change. From the GObject documentation of dispose "The object is also expected to be able to answer client method invocations (with possibly an error code but no memory violation) until finalize is executed.".

The WebCore::Page is deleted by the dispose implementation and there are plenty of methods in webkitwebview.cpp that do _not_ check if the priv->page is valid. Do you find some time to correct this?

[Benjamin Otte]

Making sure the API still works after a dispose call has never been done on any code based on GObject that I know of. That part of the documentation is at least misleading, but most likely just wrong.

[Alexander Butenko]

adding null check for priv->corePage partly fixes a problem. After adding a warning in if(!priv->corePage) im getting results like this:

$ ./GtkLauncher 'about:blank'
Segmentation fault
$  ./GtkLauncher 'http://google.com'
** (lt-GtkLauncher:13954): WARNING **: corePage is null
$

Backtrace: http://pastebin.com/m38551baf
Index: WebKit/gtk/webkit/webkitwebview.cpp
===================================================================
--- WebKit/gtk/webkit/webkitwebview.cpp	(revision 39830)
+++ WebKit/gtk/webkit/webkitwebview.cpp	(working copy)
@@ -602,14 +602,16 @@
 static void webkit_web_view_set_scroll_adjustments(WebKitWebView* webView, GtkAdjustment* hadj, GtkAdjustment* vadj)
 {
     FrameView* view = core(webkit_web_view_get_main_frame(webView))->view();
+    WebKitWebViewPrivate* priv = webView->priv;
 
+    if(!priv->corePage)
+	return;
+
     if (hadj)
         g_object_ref(hadj);
     if (vadj)
         g_object_ref(vadj);
 
-    WebKitWebViewPrivate* priv = webView->priv;
-
     if (priv->horizontalAdjustment)
         g_object_unref(priv->horizontalAdjustment);
     if (priv->verticalAdjustment)

[Xan Lopez]

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

[Martin Robinson]

What's the state of this bug? Are these still valid crashers?

[Benjamin Otte]

I don't remember seeing it in recent times. And it's certainly very old. So feel free to close this.

[Martin Robinson]

Thanks!