Q: why does Nav_stack_clean() free the data and Nav_stack_truncate() not?
While reviewing nav.c to figure out the reload crash bug I noticed something that seems inconsistent: both Nav_stack_truncate() and Nav_stack_clean() remove items from the navigation stack, yet Nav_stack_truncate() does not free the data it removes and Nav_stack_clean() does. Is this a bug? If not, what haven't I understood? Regards, Jeremy Henty
Hi Jeremy, On Sat, May 24, 2008 at 08:15:47PM +0100, Jeremy Henty wrote:
While reviewing nav.c to figure out the reload crash bug I noticed something that seems inconsistent: both Nav_stack_truncate() and Nav_stack_clean() remove items from the navigation stack, yet Nav_stack_truncate() does not free the data it removes and Nav_stack_clean() does. Is this a bug? If not, what haven't I understood?
Nav_stack_truncate() and Nav_stack_insert() are ad-hoc bindings. They're called just in one place of the code. They're not clean well-defined functions of an ADT. This is "your" mistake. :-) I stumbled upon Nav_stack_clean() some time ago, and wondered whether it still serves a clear purpose or if it could be replaced. The nav.c module is asking for a clean ADT, and if you can work on it, it's certainly welcomed. Beware that it's more complex than it looks. Especially the repush code. Nav.c takes care of redirections, reloads, repushes, expects, jumps, pushes, etc. BTW, if it's to be rewritten it'd be good to have the TABs interface in mind. This is, if we end with a clean ADT or object, we could have an array of them, one for each tab, and switch freely among navigation stacks. -- Cheers Jorge.-
On Sun, May 25, 2008 at 09:31:30AM -0400, Jorge Arellano Cid wrote:
Hi Jeremy,
Nav_stack_truncate() and Nav_stack_insert() are ad-hoc bindings. They're called just in one place of the code. They're not clean well-defined functions of an ADT.
This is "your" mistake. :-)
I'll cheerfully abandon my expectations of consistency. :-) But what about memory[1]? Doesn't it leak when Nav_stack_truncate() shrinks bw->nav_stack ? It seems to me that Nav_stack_truncate() should free all those pointers (and therefore Nav_stack_insert() shouldn't free anything). I'll supply a patch if you agree.
The nav.c module is asking for a clean ADT, and if you can work on it, it's certainly welcomed.
Then I hope people will review the patch I sent to the discussion with corvid about the Redirect/Reload crash bug, which is all about the handling of the navigation stack.
BTW, if it's to be rewritten it'd be good to have the TABs interface in mind.
Oh noes, you have spotted my hidden agenda! :-) I was indeed thinking that if I got my head around this it would prepare me for looking at tabs.
This is, if we end with a clean ADT or object, we could have an array of them, one for each tab, and switch freely among navigation stacks.
As a first step, maybe we could modify uicmd.cc to indirect though a new BrowserWindowSet object. The callbacks would be changed to look up the current BrowserWindow of the BrowserWindowSet . Initially the BrowserWindowSet could be a stub that managed a single BrowserWindow , later it could wrap an array of BrowserWindows . Regards, Jeremy Henty [1] Dillo's memory, that is, not mine. I'm afraid fixing Dillo's memory leaks is easier than fixing my own!
On Sun, May 25, 2008 at 04:39:46PM +0100, Jeremy Henty wrote:
On Sun, May 25, 2008 at 09:31:30AM -0400, Jorge Arellano Cid wrote:
Hi Jeremy,
Nav_stack_truncate() and Nav_stack_insert() are ad-hoc bindings. They're called just in one place of the code. They're not clean well-defined functions of an ADT.
This is "your" mistake. :-)
I'll cheerfully abandon my expectations of consistency. :-) But what about memory[1]? Doesn't it leak when Nav_stack_truncate() shrinks bw->nav_stack ? It seems to me that Nav_stack_truncate() should free all those pointers (and therefore Nav_stack_insert() shouldn't free anything). I'll supply a patch if you agree.
Yes it leaks. (a_Nav_free is incomplete BTW). Patch committed, plus the 's/insert/append/' patch.
The nav.c module is asking for a clean ADT, and if you can work on it, it's certainly welcomed.
Then I hope people will review the patch I sent to the discussion with corvid about the Redirect/Reload crash bug, which is all about the handling of the navigation stack.
BTW, if it's to be rewritten it'd be good to have the TABs interface in mind.
Oh noes, you have spotted my hidden agenda! :-) I was indeed thinking that if I got my head around this it would prepare me for looking at tabs.
This is, if we end with a clean ADT or object, we could have an array of them, one for each tab, and switch freely among navigation stacks.
As a first step, maybe we could modify uicmd.cc to indirect though a new BrowserWindowSet object. The callbacks would be changed to look up the current BrowserWindow of the BrowserWindowSet . Initially the BrowserWindowSet could be a stub that managed a single BrowserWindow , later it could wrap an array of BrowserWindows .
It can be done in several ways. The design needs to consider at least front/background TAB loading and async events. e.g. idle functions, cache updates, messages. -- Cheers Jorge.-
participants (2)
-
jcid＠dillo.org -
onepoint＠starurchin.org