[PATCH] fix proposed for fdo#48368

classic Classic list List threaded Threaded
4 messages Options
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

[PATCH] fix proposed for fdo#48368

Hello,

I reproduced the problem indicated by fdo#48368 and attached to the bugtracker bt with symbols (see https://bugs.freedesktop.org/attachment.cgi?id=59985&action=edit)

Here are the lines of basctl/source/basicide/basidesh.cxx which seem to cause the problem :
    420         for ( sal_uLong nWin = 0; bCanClose && ( nWin < aIDEWindowTable.size() ); nWin++ )
    421         {
    422             IDEBaseWindow* pWin = aIDEWindowTable[ nWin ];
    423             if ( !pWin->CanClose() )

I runned this on gdb and found this :
aIDEWindowTable.size() = 1 but even at the first loop, pWin is null so line 423 fails.

I searched about aIDEWindowTable and found this in basctl/source/inc/basidesh.hxx :
     62 #if _SOLAR__PRIVATE
     63 typedef std::map<sal_uInt16, IDEBaseWindow*> IDEWindowTable;
     64 #else
     65 typedef std::map<sal_uInt16, void*> IDEWindowTable;
     66 #endif

So I propose this straight forward fix :
diff --git a/basctl/source/basicide/basidesh.cxx b/basctl/source/basicide/basidesh.cxx
index e4dcd98..02e10c2 100644
--- a/basctl/source/basicide/basidesh.cxx
+++ b/basctl/source/basicide/basidesh.cxx
@@ -417,9 +417,9 @@ sal_uInt16 BasicIDEShell::PrepareClose( sal_Bool bUI, sal_Bool bForBrowsing )
     else
     {
         sal_Bool bCanClose = sal_True;
-        for ( sal_uLong nWin = 0; bCanClose && ( nWin < aIDEWindowTable.size() ); nWin++ )
+        for (IDEWindowTable::const_iterator it = aIDEWindowTable.begin(); bCanClose && (it != aIDEWindowTable.end()); ++it)
         {
-            IDEBaseWindow* pWin = aIDEWindowTable[ nWin ];
+            IDEBaseWindow* pWin = it->second;
             if ( !pWin->CanClose() )
             {
                 if ( !m_aCurLibName.isEmpty() && ( pWin->IsDocument( m_aCurDocument ) || pWin->GetLibName() != m_aCurLibName ) )

I can commit and push on master of course but I'd like first your opinion about this.

Julien.
Noel Power-3 Noel Power-3
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] fix proposed for fdo#48368

On 14/04/12 22:20, julien2412 wrote:

> Hello,
>
> I reproduced the problem indicated by fdo#48368 and attached to the
> bugtracker bt with symbols (see
> https://bugs.freedesktop.org/attachment.cgi?id=59985&action=edit)
>
> Here are the lines of basctl/source/basicide/basidesh.cxx which seem to
> cause the problem :
>      420         for ( sal_uLong nWin = 0; bCanClose&&  ( nWin<
> aIDEWindowTable.size() ); nWin++ )
>      421         {
>      422             IDEBaseWindow* pWin = aIDEWindowTable[ nWin ];
>      423             if ( !pWin->CanClose() )
>
> I runned this on gdb and found this :
> aIDEWindowTable.size() = 1 but even at the first loop, pWin is null so line
> 423 fails.
>
> I searched about aIDEWindowTable and found this in
> basctl/source/inc/basidesh.hxx :
>       62 #if _SOLAR__PRIVATE
>       63 typedef std::map<sal_uInt16, IDEBaseWindow*>  IDEWindowTable;
>       64 #else
>       65 typedef std::map<sal_uInt16, void*>  IDEWindowTable;
>       66 #endif
>
> So I propose this straight forward fix :
> diff --git a/basctl/source/basicide/basidesh.cxx
> b/basctl/source/basicide/basidesh.cxx
> index e4dcd98..02e10c2 100644
> --- a/basctl/source/basicide/basidesh.cxx
> +++ b/basctl/source/basicide/basidesh.cxx
> @@ -417,9 +417,9 @@ sal_uInt16 BasicIDEShell::PrepareClose( sal_Bool bUI,
> sal_Bool bForBrowsing )
>       else
>       {
>           sal_Bool bCanClose = sal_True;
> -        for ( sal_uLong nWin = 0; bCanClose&&  ( nWin<
> aIDEWindowTable.size() ); nWin++ )
> +        for (IDEWindowTable::const_iterator it = aIDEWindowTable.begin();
> bCanClose&&  (it != aIDEWindowTable.end()); ++it)
>           {
> -            IDEBaseWindow* pWin = aIDEWindowTable[ nWin ];
> +            IDEBaseWindow* pWin = it->second;
>               if ( !pWin->CanClose() )
>               {
>                   if ( !m_aCurLibName.isEmpty()&&  ( pWin->IsDocument(
> m_aCurDocument ) || pWin->GetLibName() != m_aCurLibName ) )
>
> I can commit and push on master of course but I'd like first your opinion
> about this.
>
please commit looks like the correct fix to me ( on a side note can
anyone remember what ( #if _SOLAR__PRIVATE ) is all about,  I can't see
any reason for it in the context of this code in anycase :-/ and would
be tempted to delete it. And btw thanks, always a pleasure to come back
from vacation to see something from the to do list fixed by someone else
:-))) thanks again!!

Noel


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] fix proposed for fdo#48368

Noel Power-3 wrote
>...
> I can commit and push on master of course but I'd like first your opinion
> about this.
>
please commit looks like the correct fix to me ( on a side note can
anyone remember what ( #if _SOLAR__PRIVATE ) is all about,  I can't see
any reason for it in the context of this code in anycase :-/ and would
be tempted to delete it. And btw thanks, always a pleasure to come back
from vacation to see something from the to do list fixed by someone else
:-))) thanks again!!

Noel
No problem, I'll do this as soon as get back home (so after day work :-) ).

BTW, I noticed that const_iterators were used. As a beginner, I don't understand how const_iterators can work in this file whereas most of the time the object referenced is modified, eg :
    248     for( IDEWindowTable::const_iterator it = aIDEWindowTable.begin(); it != aIDEWindowTable.end(); ++it )
    249     {
    250         // no store; does already happen when the BasicManagers are destroyed
    251         delete it->second;
    252     }

    338     for( std::vector<IDEBaseWindow*>::const_iterator it = aDeleteVec.begin(); it != aDeleteVec.end(); ++it )
    339     {
    340         IDEBaseWindow* pWin = *it;
    341         pWin->StoreData();

But perhaps I misunderstood something (which wouldn't be surprising :-))

Julien
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

[PUSHED] Re: [PATCH] fix proposed for fdo#48368

I pushed the commit http://cgit.freedesktop.org/libreoffice/core/commit/?id=8bfee2c0eb6fcec49d05562bf1cb945356a4180e for the fix and updated bug status to RESOLVED/FIXED

Julien.