GNOME Bugzilla – Bug 142184
Reproducable crash when closing popup at http://www.frip.dk/boksidan/
Last modified: 2005-07-09 16:04:42 UTC
[Epiphany 1.2.5] Epiphany crashes as soon as I close the popup window that shows up at http://www.frip.dk/boksidan/. I have epiphany-extensions installed, but popup blocking was obviously not turned on for that site. Steps to reproduce: 1) Go to http://www.frip.dk/boksidan/ 2) Close the popup window that appears. Result: Epiphany will crash. I don't have a debug build, but chpe can reproduce the bug so he will attach a trace below.
+ Trace 46741
Note that the tab=0x0/embed=0x0 in #1-#4 are probably gdb bogosity. However:
+ Trace 46742
$16 = (GtkWidget *) 0x0 (gdb) p /x ((GtkObject*) tab)->flags $21 = 0x210621 Analysis: Neither tab nor embed are realized; i.e. the signal is called after they have been unrealized and since embed->priv->browser == NULL, after they've been destroyed. Embed isn't inside Tab anymore. Marco, any idea how to solve this?
Shouldnt the embed->priv->browser == NULL check deal with this already ?
We removed that check with the patch from bug 141262.
Well the quick fix could be to put the NULL check back. We need to figure out these damned lifetime issues now though. Can you put a full trace ? I want to see how the title signal is emitted.
Oh please put a comment about it, or next time I'm going to remove it again and regress.
So I think there are two sides of the issue. One is that I think gtkmozembed should not emit events when EmbedPrivate has been deleted. Gtkmozembed api itself would crash I think. I have an idea about it but I should talk with blizzard. The other is that our code could be more robust. I tend to consider ephy-embed like a wrapper around EphyBrowser (like mozilla-dowload around MozDownload etc). So I think it would be good EphyBrowser and ephy-embed life cycle would match exactly. I would call ->Destroy from destroy and ensure the stuff we initialized in Init is realesed (== nsnull), this is similar to what gtkmozembed does too. Though, and here we diverge from gtkmozembed, I'd delete the EphyBrowser object only on finalize. For this particular case that means our mDOMWindow check in GetDocumentUrl would avoid the crash. What do you think ? If you agree on it we could prolly just go ahead with this on head instead of the quick fix. Better ideas ?
Ok, I made those changes: delete the embed's EphyBrowser only on finalize (but still Destroy() it on destroy), and made the public EphyBrowser methods safe to be called after Destroy (if (!mWebBrowser/mDOMWindow) return ...;). However I think we should do something more: make EphyTab not emit any signals after it's destroyed (so that extensions don't have to check for that). We could copy crispin's fix for galeon and just block the signal handlers the tab connected to the embed in the destroy signal handler.
Where is the patch ?;) I'd use ENSURE, not ifs, except for GetDocumentUrl (and I'd put a comment there for why we use if). Actually, if mozilla ENSURE works also for not debug build I'd use ENSURE everywhere, it's good to leave the warning in debug builds there I think. I dont think we should work around the problem in ephy-tab unless there is evidence it's necessary. We want to solve this in gtkmozembed, since it's a problem for all gtkmozembed users.
Created attachment 27578 [details] [review] patch Well I had already checked it in, since this was just the described changes. Yes, NS_ENSURE_TRUE/NS_ENSURE_SUCCESS works in non-debug too, it just returns without printing a message on console. I don't think we should make a distinction between functions which we may use internally in ephy after destroy like GetDocumentUrl and which we don't use, since extensions may use other functions after destroy. Either all functions are ok to use, or none, imho; so either NS_ENSURE_TRUE for all or if (!...) return; for all. I agree that we really want this fixed in gtkmozembed too.
>I don't think we should make a distinction between functions which we may use >internally in ephy after destroy like GetDocumentUrl and which we don't use, >since extensions may use other functions after destroy. Either all functions >are ok to use, or none, imho; so either NS_ENSURE_TRUE for all or if (!...) >return; for all. Sure. I was just thinking we needed the work around for GetDocumentUrl on the model of g_returns. There you would get a crash in non debug builds. With mozilla is different though. I think they should definately be all ENSURE, it's something that should not happen.
I've now changed them all to NS_ENSURE_TRUE.
Verified Fixed.
Did you release in Destroy stuff we are referencing in Init ?
Yes, ::Destroy() sets mWebBrowser and mDOMWindow to nsnull, detaches the listeners attached in ::Init(), and sets mInitialized to PR_FALSE. Btw, I noticed we never seem to delete the listeners themselves... do we leak them, or are they just weakref'd?
*** Bug 143127 has been marked as a duplicate of this bug. ***
Getting reports of this on Debian too http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=248342 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=251802 Any idea when the next release will be?
*** Bug 144312 has been marked as a duplicate of this bug. ***
*** Bug 145024 has been marked as a duplicate of this bug. ***
*** Bug 145949 has been marked as a duplicate of this bug. ***
*** Bug 146044 has been marked as a duplicate of this bug. ***
*** Bug 146100 has been marked as a duplicate of this bug. ***
*** Bug 146303 has been marked as a duplicate of this bug. ***
*** Bug 146338 has been marked as a duplicate of this bug. ***
*** Bug 146371 has been marked as a duplicate of this bug. ***
*** Bug 146423 has been marked as a duplicate of this bug. ***
*** Bug 146572 has been marked as a duplicate of this bug. ***
*** Bug 146573 has been marked as a duplicate of this bug. ***
*** Bug 146587 has been marked as a duplicate of this bug. ***
*** Bug 147363 has been marked as a duplicate of this bug. ***
*** Bug 165443 has been marked as a duplicate of this bug. ***
*** Bug 307647 has been marked as a duplicate of this bug. ***