Bug 23194 - [GTK] fix crashers
Summary: [GTK] fix crashers
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:
: 23055 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-01-08 13:38 PST by Benjamin Otte
Modified: 2010-10-12 18:15 PDT (History)
2 users (show)

See Also:


Attachments
suggested fix (4.78 KB, patch)
2009-01-08 13:43 PST, Benjamin Otte
zecke: review-
Details | Formatted Diff | Diff
fix WebView to use dispose (3.94 KB, patch)
2009-01-09 14:49 PST, Benjamin Otte
no flags Details | Formatted Diff | Diff
make DOwnload policy decision not fail (1.42 KB, patch)
2009-01-09 14:50 PST, Benjamin Otte
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Otte 2009-01-08 13:38:32 PST
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.
Comment 1 Benjamin Otte 2009-01-08 13:43:05 PST
Created attachment 26531 [details]
suggested fix
Comment 2 Benjamin Otte 2009-01-08 13:46:31 PST
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
Comment 3 Holger Freyther 2009-01-08 15:58:57 PST
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.
Comment 4 Benjamin Otte 2009-01-09 14:49:08 PST
Created attachment 26578 [details]
fix WebView to use dispose
Comment 5 Benjamin Otte 2009-01-09 14:50:54 PST
Created attachment 26579 [details]
make DOwnload policy decision not fail
Comment 6 Holger Freyther 2009-01-09 14:57:55 PST
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 :)
Comment 7 Holger Freyther 2009-01-09 19:48:53 PST
Landed in r39775 and r39776.
Comment 8 Holger Freyther 2009-01-10 15:57:33 PST
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?
Comment 9 Benjamin Otte 2009-01-12 02:31:00 PST
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.
Comment 10 Alexander Butenko 2009-01-12 14:21:13 PST
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)
Comment 11 Xan Lopez 2009-03-01 22:39:36 PST
*** Bug 23055 has been marked as a duplicate of this bug. ***
Comment 12 Martin Robinson 2010-10-12 16:36:22 PDT
What's the state of this bug? Are these still valid crashers?
Comment 13 Benjamin Otte 2010-10-12 18:07:46 PDT
I don't remember seeing it in recent times. And it's certainly very old. So feel free to close this.
Comment 14 Martin Robinson 2010-10-12 18:15:42 PDT
Thanks!