Re: [Patch] nautilus crash #135727

[Alexander Larsson <alexl redhat com>]

On Tue, 2005-03-22 at 13:41 +0530, Vijaykumar Patwari wrote:
> Hi All,
> 
> Synopsis
> --------
> Crash when switching to catalog view repeatedly.
> 
> This bug is specific to 2.6 branch and not applicable to current sources.
> 
> Analysis and Solution
> ---------------------
> Basically it looks to be a timing issue for the out processess which are
> embeded onto nautilus. For ex: Image Collection View and Catalog View.
> 
> This bug is only visible switching from `Image collection view` to
> `Catalog view`.
> 
> Looks like Bonobo `ui_events` are being addressed after the existing 
> content_view widget is being destroyed. Instead, these events are to be 
> processed at very initial stage or in other words, before destroying the 
> existing view i.e window->content_view. I'm not very sure why this is 
> happens, but the attached patch looks to be a way to avoid these type of 
> situations.

Bonobo events generally result in queueing an idle and executing the
code from there to avoid reentrancy issues. However, if the view later
goes away we need to either handle that in the idle handler or remove
the idle handler when this happens. Fortunately all this is much easier
in HEAD which doesn't use bonobo.

> This patch basically avoids any usage of `window->content_view` if its
> not having the appropriate value or already destroyed (as in current case).
> 
> Bugzilla bug: http://bugzilla.gnome.org/show_bug.cgi?id=135727.
> 
> Kindly have a look at the attached patch.

The first part of the patch looks safe. I can't tell if its solving the
root cause or just covering over the problem in some cases. However,
thats probably ok for an old codebase.

The second part doesn't look right though. Calling
nautilus_window_set_content_view() when window->content_view == NULL is
surely not an error. content_view always starts as NULL, and gets set
using this function, doesn't it? Please explain this part better. 

Even if it is right, you shouldn't use g_return_if_fail() for normal
code flow. g_return_if_fail() is a form of assert() that can be disabled
at compile-time and is there to verify that callers conform to the
function call interface rules.

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a lounge-singing crooked sorceror whom everyone believes is mad. She's a 
bloodthirsty bisexual magician's assistant with a song in her heart and a 
spring in her step. They fight crime!