[PATCH] Fix for dialog Manage Breakpoints crash

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

[PATCH] Fix for dialog Manage Breakpoints crash

 I started out trying to fix fdo 42778 and noticed that my initial description of the problem was a bit to narrow. It wasn't just the pass count that wasn't stored correctly but the breakpoints all together. In worst case it makes the program crash. This fix, stores the values. I'm not that experienced with C++ so could someone have a second look at this there might very well be a "cleaner" way of writing it.

After building it with the fix, Manage Breakpoints works almost as expected on my machine.
If you set the pass count to 1 it stops at the first 2 stops at the second. 1 should pass one and stop the second time. I'll try to fix this in another patch though.
-- 
Niklas Johansson

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

0001-Fixes-BreakPointManager.patch (1022 bytes) Download Attachment
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix for dialog Manage Breakpoints crash

Hi Niklas,

On Monday, 2011-11-28 12:27:16 +0100, Niklas Johansson wrote:

> In worst case it makes the program crash. This fix, stores
> the values. I'm not that experienced with C++ so could someone have
> a second look at this there might very well be a "cleaner" way of
> writing it.

Good find!

I think I would do it slightly different, the patch fixes the symptom
correctly by allocating new objects, but the underlying cause to me
seems that the original intention was to transfer the pointers (hence
the method's name) but that somehow got mangled with the transition from
old List to ::std::vector


> --- a/basctl/source/basicide/bastypes.cxx
> +++ b/basctl/source/basicide/bastypes.cxx
> @@ -288,7 +288,7 @@ void BreakPointList::transfer(BreakPointList & rList)
>  {
>      reset();
>      for (size_t i = 0; i < rList.size(); ++i)
> -        maBreakPoints.push_back( rList.at( i ) );
> +        maBreakPoints.push_back( new BreakPoint(*rList.at( i )) );
>      rList.reset();
>  }
Instead of creating copies I'd do

-      rList.reset();
+      rList.clear();

Please check if that fixes the problem.

  Eike

--
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment
Niklas Johansson Niklas Johansson
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix for dialog Manage Breakpoints crash


>> In worst case it makes the program crash. This fix, stores
>> the values. I'm not that experienced with C++ so could someone have
>> a second look at this there might very well be a "cleaner" way of
>> writing it.
> Good find!
>
> I think I would do it slightly different, the patch fixes the symptom
> correctly by allocating new objects, but the underlying cause to me
> seems that the original intention was to transfer the pointers (hence
> the method's name) but that somehow got mangled with the transition from
> old List to ::std::vector
>
>
>> --- a/basctl/source/basicide/bastypes.cxx
>> +++ b/basctl/source/basicide/bastypes.cxx
>> @@ -288,7 +288,7 @@ void BreakPointList::transfer(BreakPointList&  rList)
>>   {
>>       reset();
>>       for (size_t i = 0; i<  rList.size(); ++i)
>> -        maBreakPoints.push_back( rList.at( i ) );
>> +        maBreakPoints.push_back( new BreakPoint(*rList.at( i )) );
>>       rList.reset();
>>   }
> Instead of creating copies I'd do
>
> -      rList.reset();
> +      rList.clear();
>
> Please check if that fixes the problem.
>
>    Eike
That fixes the problem, and definitely looks like a cleaner solution. Do
you create the patch or should I create a new one?

I created and attached a patch for the pass count.
Description:
"Example: When entering pass count 2 it is expected that the breakpoint
is passed two times before breaking not to break the second time."

Thanks
Niklas Johansson


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

0001-Fixes-pass-count-in-Manage-Breakpoints.patch (1023 bytes) Download Attachment
Eike Rathke-2 Eike Rathke-2
Reply | Threaded
Open this post in threaded view
|

Re: [PUSHED][PATCH] Fix for dialog Manage Breakpoints crash

Hi Niklas,

On Tuesday, 2011-11-29 09:50:26 +0100, Niklas Johansson wrote:

> >-      rList.reset();
> >+      rList.clear();
> >
> >Please check if that fixes the problem.
>
> That fixes the problem, and definitely looks like a cleaner
> solution. Do you create the patch or should I create a new one?

I included that with your patch.

> I created and attached a patch for the pass count.

Great, thanks! Pushed to master:
http://cgit.freedesktop.org/libreoffice/core/commit/?id=0083479a80de831b0ae90877aa4f5d6c4670eb49

I just mentioned fdo#42778 and the crash in the commit's summary.

As this seems to be your first contribution to the code base please
confirm with a blanket statement that you contribute this and further
patches under LGPLv3+ and MPL 1.1 licenses.

Thanks
  Eike

--
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment