Re: valgrind still unhappy despite expect changes

Jorge wrote:
> On Thu, Sep 01, 2011 at 07:47:48PM +0000, corvid wrote:
> > Jorge wrote:
> > > On Thu, Sep 01, 2011 at 10:56:19AM -0300, Jorge Arellano Cid wrote:
> > > > On Thu, Sep 01, 2011 at 03:38:58PM +0200, Johannes Hofmann wrote:
> > > > > On Thu, Sep 01, 2011 at 09:57:39AM -0300, Jorge Arellano Cid wrote:
> > > > > > On Wed, Aug 31, 2011 at 09:42:21PM +0000, corvid wrote:
> > > > > > > Jorge wrote:
> > > 
> > > > > > > [...]
> > > > > > > In this one
> > > > > > > http://starurchin.org/dillo/valgrind/7ea9fc809376ddf7dde2908e2ecf999aea274130.html
> > > > > > > at least, it looks pretty clear that the bw is gone.
> > > 
> > > 
> > > > > > [...]
> > > > > >   Does the invalid read apply to 'bw' or 'bw->nav_expect_url'?
> > > > > >   (I don't know valgrind's semantics on it)
> > > > > 
> > > > > I'd guess bw is no longer pointing to valid memory (i.e. has been
> > > > > free'd) and therefore reading the 4 byte nav_expect_url pointer
> > > > > causes the valgrind message.
> > > > 
> > > >   OK, this line gave light in that direction:
> > > > 
> > > > ==28991==  Address 0x6961a34 is 44 bytes inside a block of size 68 free'd
> > > > 
> > > >   ... and I observe that bw has 17 items, 17*4 = 68,
> > > >     and the expect URL is the 12th item, (12-1)*4 = 44.
> > > > 
> > > >   So it's an already freed 'bw' and not its expected URL.
> > > > 
> > > >   This starts to make sense...  ;-)
> > > 
> > >   BTW, why there are two different stack traces?
> > >   What does the second one mean with respect to the first?
> > 
> > The top one shows the invalid read, and the bottom shows how the
> > memory was freed that the top tried to read.
> 
>   Thanks.
> 
>   Well, in that case we have a weird situation:
> 
> Both stacks have:
> 
>   ==28991==    by 0x8053F49: CustTabs::handle(int) (uicmd.cc:204)
> 
> Which  translates  into  a bw (window or tab) that's closed twice
> by using the keyboard.
> 
> 
>   Strange as it seems, let's analyze it a bit:
> For __both_ cases:
> 
> 1.- fltk is sending the SHORTCUT event for KEYS_CLOSE_TAB
> 2.- dillo receives it in CustTabs::handle() and gets the bw with:
> 
>       UI *ui = (UI*)wizard()->value();
>       BrowserWindow *bw = a_UIcmd_get_bw_by_widget(ui);
> 
>   (which means the ui is the current one and that the bw is in
>    our valid bw list.)
> 
> 3.- dillo calls a_UIcmd_close_bw() which:
> 
>   a) stops the clients
>   b) tabs->remove_tab(ui)   // This switches to another bw
>                               and removes the ui from wizard
> 
>     * Here the question is how can dillo find the ui and same
>       bw, in the wizard as current, and in the valid bw list
>       (after 3c), for a second time at 2.- ?
> 
>   c) a_Bw_free(bw)          // the bw is removed from our valid
>                               list and freed.
> 
> 
> 
>   The only answer I can find for the above question is:
> The memory space for bw is reused with a new ui. This is:
> 
>   * create a bw (e.g. address 0x888).
>   * close the bw, free the bw (0x888 is available again)
>   * create a bw (malloc reuses address 0x888).
>   * close the bw  // Here we have the situation!
>   * free the bw (0x888 is available again)
> 
> 
>   which is compatible with:
> 
>   "Valgrind Bug254420 - memory pool tracking broken"
>     http://bugs.kde.org/show_bug.cgi?id=254420
> 
> 
>   but, of course, I may be wrong...
> 
> 
> 
> > > [...]
> > >   OTOH, analyzing the code:
> > > 
> > > AFAIS:
> > > 
> > >  * The only place where a bw is freed is in a_UIcmd_close_bw [3]:
> > >    (the only call to a_Bw_free() in the codebase).
> > >  * bw is valid at [1]
> > >  * The problematic a_Nav_cancel_expect_if_eq() is called from
> > >    inside the call chain at [2].
> > >  * No other a_UIcmd_close_bw() appears in the log, so how came
> > >    bw to be invalid then?
> > > 
> > > 
> > >      void a_UIcmd_close_bw(void *vbw)
> > >      {
> > >         BrowserWindow *bw = (BrowserWindow *)vbw;
> > >         UI *ui = BW2UI(bw);
> > >         CustTabs *tabs = ui->tabs();
> > >   1     Layout *layout = (Layout*)bw->render_layout;
> > >      
> > >         MSG("a_UIcmd_close_bw bw=%p\n", bw);
> > >   2     a_Bw_stop_clients(bw, BW_Root + BW_Img + BW_Force);
> > >         delete(layout);
> > >         if (tabs) {
> > >            tabs->remove_tab(ui);
> > >            if (tabs->num_tabs() == 0)
> > >               delete tabs->window();
> > >         }
> > >   3     a_Bw_free(bw);
> > >         MSG("a_UIcmd_close_bw freed(bw=%p)\n", bw);
> > >      }
> > > 
> > > 
> > >   Any help is appreciatted.
> > 
> > My guess is that it could be something like
> > first, one bw asks for a page, and that becomes conn->bw,
> > and then another bw starts also asking for the page, and
> > the first bw closes, invalidating the conn->bw, and then
> > the second bw closes and the cancel expect code tries to
> > use the old bw.
> 
>   "bw" is a bit a misleading name, bacause each tab is a bw.
> 
>   OTOH, each tab has its own control panel and thus becomes
> a browser window object.
> 
>   In the case you propose there'd be two different bw.

Right. Why do you feel that they can't be two bws that were
both closed from the keyboard?

>   Bottom line:
> 
>    This bug is currently holding the dillo-3.0 release.
> 
>    Given  the  analysis,  that  nobody knows how to reproduce it,
> that  even  if  real, it doesn't do much harm, the situation is:
> if there's no news in a couple of days, the release will be made.

No objections here.