[Webkit-unassigned] [Bug 21837] critical error in Gtk FrameLoaderClient::frameLoaderDestroyed()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21837





------- Comment #11 from jmalonzo at gmail.com  2008-12-03 04:17 PDT -------
(In reply to comment #9)
> (From update of attachment 25422 [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. 


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.