Bug 21837 - critical error in Gtk FrameLoaderClient::frameLoaderDestroyed()
: critical error in Gtk FrameLoaderClient::frameLoaderDestroyed()
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 22491
:
  Show dependency treegraph
 
Reported: 2008-10-23 13:22 PST by
Modified: 2009-01-03 12:08 PST (History)


Attachments
add a ref in the constructor (1.22 KB, patch)
2008-10-23 14:06 PST, Jonathon Jongsma (jonner)
alp: review-
Review Patch | Details | Formatted Diff | Diff
Fix leaks (9.14 KB, patch)
2008-10-24 23:48 PST, Alp Toker
no flags Review Patch | Details | Formatted Diff | Diff
updated and cleaned up Alp's patch (6.08 KB, patch)
2008-11-24 03:06 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
Add unit test for this bug (2.33 KB, patch)
2008-11-25 12:09 PST, Holger Freyther
darin: review+
Review Patch | Details | Formatted Diff | Diff
Updated patch based on feedback from zecke (11.71 KB, patch)
2008-12-04 11:46 PST, Jan Alonzo
no flags Review Patch | Details | Formatted Diff | Diff
WIP Stop leaking (10.04 KB, patch)
2008-12-20 18:56 PST, Holger Freyther
no flags Review Patch | Details | Formatted Diff | Diff
Fix the reference counting (16.04 KB, patch)
2008-12-21 10:52 PST, Holger Freyther
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-10-23 13:22:13 PST
We're unreffing the m_frame object but never taking a ref on it.  Backtrace for the critical warning is as follows:

#0  0xb7f797f2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2
#1  0xb5731085 in raise () from /lib/tls/i686/cmov/libc.so.6
#2  0xb5732a01 in abort () from /lib/tls/i686/cmov/libc.so.6
#3  0xb58970ea in IA__g_logv (log_domain=0xb59414cb "GLib-GObject", 
    log_level=G_LOG_LEVEL_CRITICAL, 
    format=0xb58cc8e8 "%s: assertion `%s' failed", 
    args1=0xbfa60adc "*-\224µP \224µ@¼ø·l÷\t\b0s\211µx¦\224µ\230£!\b(\v¦¿\032¸\221µË\024\224µ&-\224µP \224µ")
    at /build/buildd/glib2.0-2.16.6/glib/gmessages.c:497
#4  0xb5897129 in IA__g_log (log_domain=0xb59414cb "GLib-GObject", 
    log_level=G_LOG_LEVEL_CRITICAL, 
    format=0xb58cc8e8 "%s: assertion `%s' failed")
    at /build/buildd/glib2.0-2.16.6/glib/gmessages.c:517
#5  0xb589738b in IA__g_return_if_fail_warning (
    log_domain=0xb59414cb "GLib-GObject", 
    pretty_function=0xb5942d2a "g_object_unref", 
    expression=0xb5942050 "object->ref_count > 0")
    at /build/buildd/glib2.0-2.16.6/glib/gmessages.c:532
#6  0xb591b81a in IA__g_object_unref (_object=0x809f768)
    at /build/buildd/glib2.0-2.16.6/gobject/gobject.c:1742
#7  0xb63caf25 in WebKit::FrameLoaderClient::frameLoaderDestroyed (
    this=0x821a398) at WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:235
#8  0xb68dd4c8 in ~FrameLoader (this=0x821a724)
    at WebCore/loader/FrameLoader.cpp:282
#9  0xb699bbc0 in ~FramePrivate (this=0x821a700) at WebCore/page/Frame.cpp:1819
#10 0xb699c7a2 in ~Frame (this=0x821a420) at WebCore/page/Frame.cpp:176
#11 0xb63d343c in webkit_web_frame_finalize (object=0x809f768)
    at ./JavaScriptCore/wtf/RefCounted.h:96
#12 0xb591b8cb in IA__g_object_unref (_object=0x809f768)
    at /build/buildd/glib2.0-2.16.6/gobject/gobject.c:1793
#13 0xb63e0ec1 in webkit_web_view_finalize (object=0x808b150)
    at WebKit/gtk/webkit/webkitwebview.cpp:781
#14 0xb591b8cb in IA__g_object_unref (_object=0x808b150)
    at /build/buildd/glib2.0-2.16.6/gobject/gobject.c:1793
#15 0xb5c5231e in IA__gtk_object_destroy (object=0x808b150)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkobject.c:403
#16 0xb5b4e21f in gtk_bin_forall (container=0x806d710, include_internals=0, 
    callback=0x6, callback_data=0x0)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkbin.c:133
#17 0xb5c8e7b1 in gtk_scrolled_window_forall (container=0x806d710, 
    include_internals=0, callback=0xb5d58980 <IA__gtk_widget_destroy>, 
    callback_data=0x0)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkscrolledwindow.c:1021
#18 0xb5b938a7 in IA__gtk_container_foreach (container=0x806d710, 
    callback=0xb5d58980 <IA__gtk_widget_destroy>, callback_data=0x0)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkcontainer.c:1480
#19 0xb5b941d0 in gtk_container_destroy (object=0x806d710)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkcontainer.c:1020
#20 0xb5c91208 in gtk_scrolled_window_destroy (object=0x806d710)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkscrolledwindow.c:799
#21 0xb5926aef in IA__g_cclosure_marshal_VOID__VOID (closure=0x80713a8, 
    return_value=0x0, n_param_values=1, param_values=0xbfa61164, 
    invocation_hint=0xbfa6106c, marshal_data=0xb5c91160)
    at /build/buildd/glib2.0-2.16.6/gobject/gmarshal.c:77
#22 0xb5918069 in g_type_class_meta_marshal (closure=0x80713a8, 
    return_value=0x0, n_param_values=1, param_values=0xbfa61164, 
    invocation_hint=0xbfa6106c, marshal_data=0x4c)
    at /build/buildd/glib2.0-2.16.6/gobject/gclosure.c:567
#23 0xb591981f in IA__g_closure_invoke (closure=0x80713a8, return_value=0x0, 
    n_param_values=1, param_values=0xbfa61164, invocation_hint=0xbfa6106c)
    at /build/buildd/glib2.0-2.16.6/gobject/gclosure.c:490
#24 0xb592e4c5 in signal_emit_unlocked_R (node=0x80713f0, detail=0, 
    instance=0x806d710, emission_return=0x0, instance_and_params=0xbfa61164)
    at /build/buildd/glib2.0-2.16.6/gobject/gsignal.c:2556
#25 0xb592fc0f in IA__g_signal_emit_valist (instance=0x5908, signal_id=7, 
    detail=0, 
    var_args=0xbfa6139c "\234\215åµ\234\215åµ\020×\006\bÈ\023¦¿!\207Õµ\020×\006\b\020×\006\b") at /build/buildd/glib2.0-2.16.6/gobject/gsignal.c:2199
#26 0xb592ff59 in IA__g_signal_emit (instance=0x806d710, signal_id=7, detail=0)
    at /build/buildd/glib2.0-2.16.6/gobject/gsignal.c:2243
#27 0xb5c52611 in gtk_object_dispose (gobject=0x806d710)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkobject.c:418
#28 0xb5d58721 in gtk_widget_dispose (object=0x806d710)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkwidget.c:7854
#29 0xb591bd60 in IA__g_object_run_dispose (object=0x806d710)
    at /build/buildd/glib2.0-2.16.6/gobject/gobject.c:573
#30 0xb5c5231e in IA__gtk_object_destroy (object=0x806d710)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkobject.c:403
#31 0xb5b4e21f in gtk_bin_forall (container=0x808d378, include_internals=0, 
    callback=0x6, callback_data=0x0)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkbin.c:133
#32 0xb5b938a7 in IA__gtk_container_foreach (container=0x808d378, 
    callback=0xb5d58980 <IA__gtk_widget_destroy>, callback_data=0x0)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkcontainer.c:1480
#33 0xb5b941d0 in gtk_container_destroy (object=0x808d378)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkcontainer.c:1020
#34 0xb5d68c11 in gtk_window_destroy (object=0x808d378)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkwindow.c:4190
#35 0xb5926aef in IA__g_cclosure_marshal_VOID__VOID (closure=0x80713a8, 
    return_value=0x0, n_param_values=1, param_values=0xbfa617c4, 
    invocation_hint=0xbfa616cc, marshal_data=0xb5d68b90)
    at /build/buildd/glib2.0-2.16.6/gobject/gmarshal.c:77
#36 0xb5918069 in g_type_class_meta_marshal (closure=0x80713a8, 
    return_value=0x0, n_param_values=1, param_values=0xbfa617c4, 
    invocation_hint=0xbfa616cc, marshal_data=0x4c)
    at /build/buildd/glib2.0-2.16.6/gobject/gclosure.c:567
#37 0xb5919749 in IA__g_closure_invoke (closure=0x80713a8, return_value=0x0, 
    n_param_values=1, param_values=0xbfa617c4, invocation_hint=0xbfa616cc)
    at /build/buildd/glib2.0-2.16.6/gobject/gclosure.c:490
#38 0xb592e4c5 in signal_emit_unlocked_R (node=0x80713f0, detail=0, 
    instance=0x808d378, emission_return=0x0, instance_and_params=0xbfa617c4)
    at /build/buildd/glib2.0-2.16.6/gobject/gsignal.c:2556
#39 0xb592fc0f in IA__g_signal_emit_valist (instance=0x5908, signal_id=7, 
    detail=0, var_args=0xbfa619fc "º|Õµ\234\215åµxÓ\b\b(\032¦¿!\207ÕµxÓ\b\b")
    at /build/buildd/glib2.0-2.16.6/gobject/gsignal.c:2199
#40 0xb592ff59 in IA__g_signal_emit (instance=0x808d378, signal_id=7, detail=0)
    at /build/buildd/glib2.0-2.16.6/gobject/gsignal.c:2243
#41 0xb5c52611 in gtk_object_dispose (gobject=0x808d378)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkobject.c:418
#42 0xb5d58721 in gtk_widget_dispose (object=0x808d378)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkwidget.c:7854
#43 0xb5d658b6 in gtk_window_dispose (object=0x808d378)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkwindow.c:1969
#44 0xb591bd60 in IA__g_object_run_dispose (object=0x808d378)
    at /build/buildd/glib2.0-2.16.6/gobject/gobject.c:573
#45 0xb5c5231e in IA__gtk_object_destroy (object=0x808d378)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkobject.c:403
#46 0xb5c2bfc7 in IA__gtk_main_do_event (event=0x809a2f0)
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkmain.c:1492
#47 0xb5aa3a9a in gdk_event_dispatch (source=0x806fd58, callback=0, 
    user_data=0x0) at /build/buildd/gtk+2.0-2.12.9/gdk/x11/gdkevents-x11.c:2351
#48 0xb588dcc6 in IA__g_main_context_dispatch (context=0x806fda0)
    at /build/buildd/glib2.0-2.16.6/glib/gmain.c:2012
#49 0xb5891083 in g_main_context_iterate (context=0x806fda0, block=1, 
    dispatch=1, self=0x807d838)
    at /build/buildd/glib2.0-2.16.6/glib/gmain.c:2645
#50 0xb5891467 in IA__g_main_loop_run (loop=0x8212e10)
    at /build/buildd/glib2.0-2.16.6/glib/gmain.c:2853
#51 0xb5c2c264 in IA__gtk_main ()
    at /build/buildd/gtk+2.0-2.12.9/gtk/gtkmain.c:1163
#52 0x08049cfd in main (argc=Cannot access memory at address 0x6
) at WebKitTools/GtkLauncher/main.c:227
------- Comment #1 From 2008-10-23 14:06:32 PST -------
Created an attachment (id=24617) [details]
add a ref in the constructor
------- Comment #2 From 2008-10-23 14:49:44 PST -------
Actually, this is really a case that's screaming out for a GRefPtr so we don't need to do all of this manual ref-counting...
------- Comment #3 From 2008-10-23 21:37:06 PST -------
Confirmed. This started happening after r37676 from bug #20403

We need to make sure this patch doesn't introduce leaks before landing it (need to check for circular references of WebKitWebFrames).
------- Comment #4 From 2008-10-24 07:11:07 PST -------
(From update of attachment 24617 [details])
Yeah, the fix is a bit more involved. I have a patch working, will post soon.
------- Comment #5 From 2008-10-24 23:48:28 PST -------
Created an attachment (id=24671) [details]
Fix leaks

Turns that _all_ WebKitWebFrames and all WebCore::Frame's are being leaked, including other resources they keep alive.

This is a WIP patch that fixes all the Frame leaks. Full of debug comments and eeds testing, haven't tried DRT yet.
------- Comment #6 From 2008-11-24 03:06:02 PST -------
Created an attachment (id=25422) [details]
updated and cleaned up Alp's patch 

Updated alp's patch which fixes the frame leaks. We are still leaking CachedResources and WebCoreNodes (according to the debug-build stdout) but that will probably go to a separate patch. No crashes in DRT. No regression was added to what's already enabled in Gtk DRT.

This probably needs some work. But I would like to get feedback so I know what still needs to be done. 

Cheers.
------- Comment #7 From 2008-11-25 12:09:54 PST -------
Created an attachment (id=25494) [details]
Add unit test for this bug

Add a unit test for this bug, also attempt to test that we are destructing the webframe. Instead of taking a real reference on the webkitwebframe we might need to take a weak ref but the api is not too nice...
------- Comment #8 From 2008-11-25 12:10:52 PST -------
Add the dependency on the unit test bug.
------- Comment #9 From 2008-11-25 12:29:23 PST -------
(From update of attachment 25422 [details])
> diff --git a/WebKit/gtk/ChangeLog b/WebKit/gtk/ChangeLog
> index 8d05ebc..50c8605 100644
> --- a/WebKit/gtk/ChangeLog
> +++ b/WebKit/gtk/ChangeLog
> @@ -1,3 +1,21 @@
> +2008-11-24  Jan Michael Alonzo  <jmalonzo@webkit.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Original patch by Alp Toker. Updated by Jan Alonzo.
> +
> +        critical error in Gtk FrameLoaderClient::frameLoaderDestroyed()
> +        http://bugs.webkit.org/show_bug.cgi?id=21837


^^^ be a bit more verbose?

IIRC:
   WebCore::Frame and WebCore::FrameLoaderClient have the same lifetime. Initially we wanted to have WebKitWebFrame have the same lifetime as well otherwise you have to add plenty of NULL checks to WebKitWebFrame... Think of someone doing a g_object_ref on the WebKitWebFrame but the page the user navigated to was left, the WebCore::Frame destroyed....if WebCore::Frame is destroyed before the WebKitWebFrame we will have to add null checks to WebKitWebFrame. So what we have instead is something like a circular dependency and fail to break the circle. (or alternatively the lifetime of WebKitWebFrame should determine the ifetime of WebCore::Frame).



>  bool FrameLoaderClient::hasWebView() const
>  {
> -    notImplemented();
> -    return true;
> +    return getViewFromFrame(m_frame) != NULL;

No NULL in C++ code, this should never be false?




> diff --git a/WebKit/gtk/webkit/webkitwebframe.cpp b/WebKit/gtk/webkit/webkitwebframe.cpp
> index 6a4c4d5..3d8548f 100644
> --- a/WebKit/gtk/webkit/webkitwebframe.cpp
> +++ b/WebKit/gtk/webkit/webkitwebframe.cpp
> @@ -97,9 +97,11 @@ static void webkit_web_frame_finalize(GObject* object)
>      WebKitWebFrame* frame = WEBKIT_WEB_FRAME(object);
>      WebKitWebFramePrivate* priv = frame->priv;
>  
> -    priv->coreFrame->loader()->cancelAndClear();

FrameLoader is already gone?

>      priv->coreFrame = 0;
>  
> +    // priv->client will be deleted when the WebCore::Frame is actually
> +    // deleted, which may happen slightly later than WebFrame finalization.
> +

this is where the fun starts... A g_object_ref(frame) will keep the WebCore::Frame
alive as well?


>      g_free(priv->name);
>      g_free(priv->title);
>      g_free(priv->uri);
> @@ -239,7 +241,7 @@ WebKitWebFrame* webkit_web_frame_init_with_web_view(WebKitWebView* webView, HTML
>  
>      priv->webView = webView;
>      priv->client = new WebKit::FrameLoaderClient(frame);
> -    priv->coreFrame = Frame::create(viewPriv->corePage, element, priv->client).releaseRef();
> +    priv->coreFrame = Frame::create(viewPriv->corePage, element, priv->client);


okay, we start with refcount one and the above should be case?


>  
> -    webkit_web_view_stop_loading(WEBKIT_WEB_VIEW(object));
> +    if (Frame* mainFrame = priv->corePage->mainFrame())
> +        mainFrame->loader()->detachFromParent();

hmm... 




> +//    priv->children = 0;

NO!


> -    g_object_unref(priv->backForwardList);
> -    g_signal_handlers_disconnect_by_func(priv->webSettings, (gpointer)webkit_web_view_settings_notify, webView);
> +    g_signal_handlers_disconnect_by_func(priv->webSettings,
> +                                         (gpointer)webkit_web_view_settings_notify, webView);
>      g_object_unref(priv->webSettings);
> +    g_object_unref(priv->backForwardList);

unrelated changes? separate patch then?


>      g_object_unref(priv->webInspector);
> -    g_object_unref(priv->mainFrame);

I have to check this when I'm awake. But we should write something regarding the lifetime and ownership of the various components.


>      g_object_unref(priv->imContext);
>      gtk_target_list_unref(priv->copy_target_list);
>      gtk_target_list_unref(priv->paste_target_list);
> @@ -1513,7 +1516,11 @@ static void webkit_web_view_init(WebKitWebView* webView)
>  #endif
>  
>      GTK_WIDGET_SET_FLAGS(webView, GTK_CAN_FOCUS);
> -    priv->mainFrame = WEBKIT_WEB_FRAME(webkit_web_frame_new(webView));
> +
> +    // This unintuitively creates a new WebFrame and sets it as the main frame
> +    // of the WebView when the owner element argument is 0.
> +    webkit_web_frame_init_with_web_view(webView, 0);
> +
>      priv->lastPopupXPosition = priv->lastPopupYPosition = -1;
>      priv->editable = false;
>  
> @@ -1878,8 +1885,7 @@ WebKitWebFrame* webkit_web_view_get_main_frame(WebKitWebView* webView)
>  {
>      g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);
>  
> -    WebKitWebViewPrivate* priv = webView->priv;
> -    return priv->mainFrame;
> +    return kit(core(webView)->mainFrame());


this can only make sense if someone is calling get_main_frame from within the _finalize of the WebKitWebView? Do we want to support that?
------- Comment #10 From 2008-12-03 03:45:28 PST -------
(From update of attachment 25494 [details])
> +static void test_webkit_web_frame_lifetime(void)
> +{
> +    WebKitWebView* webView;
> +    WebKitWebFrame* webFrame;
> +    g_test_bug("21837");
> +
> +    webView = WEBKIT_WEB_VIEW(webkit_web_view_new());
> +    g_object_ref_sink(webView);
> +    g_assert_cmpint(G_OBJECT(webView)->ref_count, ==, 1);
> +    webFrame = webkit_web_view_get_main_frame(webView);
> +    g_assert_cmpint(G_OBJECT(webFrame)->ref_count, ==, 1);
> +
> +    // Add dummy reference on the WebKitWebFrame to keep it alive
> +    g_object_ref(webFrame);
> +    g_assert_cmpint(G_OBJECT(webFrame)->ref_count, ==, 2);
> +
> +    // This crashed with the original version
> +    g_object_unref(webView);
> +
> +    // Make sure that the frame got deleted as well. We did this
> +    // by adding an extra ref on the WebKitWebFrame and we should
> +    // be the one holding the last reference.
> +    g_assert_cmpint(G_OBJECT(webFrame)->ref_count, ==, 1);
> +    g_object_unref(webFrame);
> +}

Do we really want to support having a webView (and Page) destroyed without destroying the mainFrame first?
------- Comment #11 From 2008-12-03 04:17:09 PST -------
(In reply to comment #9)
> (From update of attachment 25422 [details] [review])
> IIRC:
>    WebCore::Frame and WebCore::FrameLoaderClient have the same lifetime.
> Initially we wanted to have WebKitWebFrame have the same lifetime as well
> otherwise you have to add plenty of NULL checks to WebKitWebFrame... Think of
> someone doing a g_object_ref on the WebKitWebFrame but the page the user
> navigated to was left, the WebCore::Frame destroyed....if WebCore::Frame is
> destroyed before the WebKitWebFrame we will have to add null checks to
> WebKitWebFrame. So what we have instead is something like a circular dependency
> and fail to break the circle. (or alternatively the lifetime of WebKitWebFrame
> should determine the ifetime of WebCore::Frame).

I agree that WebKitWebFrame should determine WebCore::Frame's lifetime, and FrameLoaderClient should determine WebKitWebFrame's lifetime? Should we follow Mac in this regard?

> > -    return true;
> > +    return getViewFromFrame(m_frame) != NULL;
> 
> No NULL in C++ code, this should never be false?

Ok. Yup. I'll add an assert to be safe.

> > +++ b/WebKit/gtk/webkit/webkitwebframe.cpp
> > @@ -97,9 +97,11 @@ static void webkit_web_frame_finalize(GObject* object)
> >      WebKitWebFrame* frame = WEBKIT_WEB_FRAME(object);
> >      WebKitWebFramePrivate* priv = frame->priv;
> >  
> > -    priv->coreFrame->loader()->cancelAndClear();
> 
> FrameLoader is already gone?

Not yet. I think the rationale was mac and qt are not doing it.

> 
> >      priv->coreFrame = 0;
> >  
> > +    // priv->client will be deleted when the WebCore::Frame is actually
> > +    // deleted, which may happen slightly later than WebFrame finalization.
> > +
> 
> this is where the fun starts... A g_object_ref(frame) will keep the
> WebCore::Frame
> alive as well?

See my previous comment (re: do we really want to support that behaviour?)

> >  
> > -    webkit_web_view_stop_loading(WEBKIT_WEB_VIEW(object));
> > +    if (Frame* mainFrame = priv->corePage->mainFrame())
> > +        mainFrame->loader()->detachFromParent();
> 
> hmm... 

I think detachFromParent is the right call here. Again, following Mac and Qt's impl.

> >      g_object_unref(priv->webInspector);
> > -    g_object_unref(priv->mainFrame);
> 
> I have to check this when I'm awake. But we should write something regarding
> the lifetime and ownership of the various components.

I'm fixing up the patch to have something like the ff:

1. WebKitWebFrame determines WebCore::Frame's lifetime.
2. FrameLoaderClient determines WebKitWebFrame's lifetime.
3. WebKitWebView triggers creation and destruction of the main frame but doesn't own it (FrameLoaderClient does).

Any comments/suggestion?

> >  
> > @@ -1878,8 +1885,7 @@ WebKitWebFrame* webkit_web_view_get_main_frame(WebKitWebView* webView)
> >  {
> >      g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);
> >  
> > -    WebKitWebViewPrivate* priv = webView->priv;
> > -    return priv->mainFrame;
> > +    return kit(core(webView)->mainFrame());
> 
> 
> this can only make sense if someone is calling get_main_frame from within the
> _finalize of the WebKitWebView? Do we want to support that?

What do you mean?

Thanks for the feedback. 
------- Comment #12 From 2008-12-04 11:46:12 PST -------
Created an attachment (id=25745) [details]
Updated patch based on feedback from zecke

Updated patch to address Holger's feedback.
------- Comment #13 From 2008-12-05 05:33:10 PST -------
(In reply to comment #11)
> (In reply to comment #9)

> 
> I agree that WebKitWebFrame should determine WebCore::Frame's lifetime, and
> FrameLoaderClient should determine WebKitWebFrame's lifetime? Should we follow
> Mac in this regard?

WebCore::Frame and FrameLoaderClient have the same lifetime. So with the above we end up with a circle... The question is what to do. And I start to believe the best way to deal with it, is to not have a circle.

We would end up with:
  WebKitWebView owns the Page. The Page ref's the WebKitWebFrame..
  WebKitWebFrame has a ref on the WebCore::Frame
  WebKitWebFrame has a pointer to the WebKitWebView and might be zero.
  WebKitWebPage has a pointer to the main frame, it will never be zero.

Truth:
  WebKitWebView has a WebCore::Frame... as the WebCore::Page is/should keep a ref on the main frame. This means the WebKitWebFrame mainFrame is always present.


This means:
  We will have to add null checks in WebKitWebFrame to see if the page/view is gone. Change the references accordingly...



Do you agree on this? If so we can go and start to look at the implementation..
------- Comment #14 From 2008-12-06 18:22:03 PST -------
(In reply to comment #13)
> WebCore::Frame and FrameLoaderClient have the same lifetime. So with the above
> we end up with a circle... The question is what to do. And I start to believe
> the best way to deal with it, is to not have a circle.

<snip> 

> This means:
>   We will have to add null checks in WebKitWebFrame to see if the page/view is
> gone. Change the references accordingly...

Ok, gotcha. Agreed.

> Do you agree on this? If so we can go and start to look at the implementation..

I'll check the patch again and upload another one. If you have thoughts on implementation, please let me know.

Also, I think I'll incorporate your test case in my patch. Is that OK with you?
------- Comment #15 From 2008-12-06 18:22:58 PST -------
(From update of attachment 25745 [details])
Clearing review flag as this needs update
------- Comment #16 From 2008-12-10 14:56:44 PST -------
(From update of attachment 25494 [details])
r=me
------- Comment #17 From 2008-12-20 18:56:09 PST -------
Created an attachment (id=26178) [details]
WIP Stop leaking

Work In Progress. This is not crashing with the test case, according to valgrind we do not leak frame's.

The FrameLoaderClient will remove the ref of the WebKitWebFrame and trigger destruction...
------- Comment #18 From 2008-12-21 10:52:21 PST -------
Created an attachment (id=26186) [details]
Fix the reference counting

Make the critical error go away. Please check that no frames are leaked.
------- Comment #19 From 2009-01-02 10:57:27 PST -------
(From update of attachment 26186 [details])
r=me
------- Comment #20 From 2009-01-03 12:08:16 PST -------
Landed in r39575. Let us see what is breaking.