Re: valgrind still unhappy despite expect changes

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.



>>>>>>>>>>>

  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.


-- 
  Cheers
  Jorge.-