Re: latest master
I think this happened because of the patch that transformed mainwindow to have an ::instance() method, but didn`t fully fixed the rest of the code. On Tue, Feb 18, 2014 at 9:47 PM, Thiago Macieira wrote: > Em ter 18 fev 2014, às 16:30:01, Dirk Hohndel escreveu: > > > Can someone verify whether the pointer was null? It looks unlikely > though. > > > > Definitely not null. And adding qDebug() to the lastUsedImageDir() > > function I was able to see that it got called. But something in that > > process destroyed the MainWindow object. Specifically, returning from > > that function appeared to cause bad things to happen inside the > > MainWindow object. And Valgrind didn't find ANYTHING. > > > > I can easily reproduce the situation if you want to poke at it on a life > > machine :-) > > I could reproduce on mine. > > Actually, my bullet grazed the skull of the bug here. dive_list() did not > return NULL, but it did become null after that line. > > Here's why: > MainWindow().dive_list()->lastUsedImageDir(), > > That should have been: > MainWindow::instance().dive_list() > > That MainWindow() there creates a new main window temporary, which > overwrites > m_Instance. At the end of that line (at the ;), the temporary gets > destroyed > and sets m_Instance to null. > > So the next MainWindow::instance() call returns null. > > So Robert's patch fixes the issue. It's correct. In addition to that, > please > apply my patch that adds an assertion to prevent this problem from > happening > again. > > -- > Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org >Software Architect - Intel Open Source Technology Center > PGP/GPG: 0x6EF45358; fingerprint: > E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 > > ___ > subsurface mailing list > subsurface@hohndel.org > http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface > ___ subsurface mailing list subsurface@hohndel.org http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
Em ter 18 fev 2014, às 16:30:01, Dirk Hohndel escreveu: > > Can someone verify whether the pointer was null? It looks unlikely though. > > Definitely not null. And adding qDebug() to the lastUsedImageDir() > function I was able to see that it got called. But something in that > process destroyed the MainWindow object. Specifically, returning from > that function appeared to cause bad things to happen inside the > MainWindow object. And Valgrind didn't find ANYTHING. > > I can easily reproduce the situation if you want to poke at it on a life > machine :-) I could reproduce on mine. Actually, my bullet grazed the skull of the bug here. dive_list() did not return NULL, but it did become null after that line. Here's why: MainWindow().dive_list()->lastUsedImageDir(), That should have been: MainWindow::instance().dive_list() That MainWindow() there creates a new main window temporary, which overwrites m_Instance. At the end of that line (at the ;), the temporary gets destroyed and sets m_Instance to null. So the next MainWindow::instance() call returns null. So Robert's patch fixes the issue. It's correct. In addition to that, please apply my patch that adds an assertion to prevent this problem from happening again. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Software Architect - Intel Open Source Technology Center PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 ___ subsurface mailing list subsurface@hohndel.org http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
On Tue, 2014-02-18 at 16:25 -0800, Thiago Macieira wrote: > Em ter 18 fev 2014, às 16:16:10, Dirk Hohndel escreveu: > > On Feb 18, 2014, at 3:47 PM, Tomaz Canabrava wrote: > > > dirk, can you test the patch? I couldn`t really understand why it crashed > > > if not static, since we should always have that object. saing that - the > > > correct way to handle that should be from the Settings, no? > > The patch does indeed fix things - and I am extremely curious to understand > > WHY. > > I'm curious too. > > If dive_list() returns a null pointer, it's undefined behaviour to call a > member function (lastUsedImageDir). It's technically undefined even to call a > static member -- any dereferencing of a null pointer is undefined behaviour. > Kernel devs must remember a GCC issue with that. > > Can someone verify whether the pointer was null? It looks unlikely though. Definitely not null. And adding qDebug() to the lastUsedImageDir() function I was able to see that it got called. But something in that process destroyed the MainWindow object. Specifically, returning from that function appeared to cause bad things to happen inside the MainWindow object. And Valgrind didn't find ANYTHING. I can easily reproduce the situation if you want to poke at it on a life machine :-) > I'll debug to see what's happening. Let me know if you need help reproducing the issue /D ___ subsurface mailing list subsurface@hohndel.org http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
Em ter 18 fev 2014, às 16:16:10, Dirk Hohndel escreveu:
> On Feb 18, 2014, at 3:47 PM, Tomaz Canabrava wrote:
> > dirk, can you test the patch? I couldn`t really understand why it crashed
> > if not static, since we should always have that object. saing that - the
> > correct way to handle that should be from the Settings, no?
> The patch does indeed fix things - and I am extremely curious to understand
> WHY.
I'm curious too.
If dive_list() returns a null pointer, it's undefined behaviour to call a
member function (lastUsedImageDir). It's technically undefined even to call a
static member -- any dereferencing of a null pointer is undefined behaviour.
Kernel devs must remember a GCC issue with that.
Can someone verify whether the pointer was null? It looks unlikely though.
I'll debug to see what's happening.
> > QSettings s;
> > s.value("lastUsedImageDirectory").toString() or something
>
> I’ll be happy to look at different implementations, but having spent way too
> much time trying to track this one down, I really want to know why it
> caused the crash in the first place.
>
> It clearly overwrote private data in the MainWindow object (in the case that
> I can reliably reproduce the ui member of the MainWindow object suddenly
> contained invalid data).
--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
Software Architect - Intel Open Source Technology Center
PGP/GPG: 0x6EF45358; fingerprint:
E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358
___
subsurface mailing list
subsurface@hohndel.org
http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
On Feb 18, 2014, at 3:47 PM, Tomaz Canabrava wrote:
> dirk, can you test the patch? I couldn`t really understand why it crashed if
> not static, since we should always have that object.
> saing that - the correct way to handle that should be from the Settings, no?
The patch does indeed fix things - and I am extremely curious to understand WHY.
> QSettings s;
> s.value("lastUsedImageDirectory").toString() or something
I’ll be happy to look at different implementations, but having spent way too
much
time trying to track this one down, I really want to know why it caused the
crash
in the first place.
It clearly overwrote private data in the MainWindow object (in the case that I
can
reliably reproduce the ui member of the MainWindow object suddenly contained
invalid data).
/D
___
subsurface mailing list
subsurface@hohndel.org
http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
dirk, can you test the patch? I couldn`t really understand why it crashed
if not static, since we should always have that object.
saing that - the correct way to handle that should be from the Settings, no?
QSettings s;
s.value("lastUsedImageDirectory").toString() or something
On Tue, Feb 18, 2014 at 8:26 PM, Robert C. Helling wrote:
>
> On 17 Feb 2014, at 23:57, Dirk Hohndel wrote:
>
> >
> > This does not include the promised whitespace overhaul.
> > Instead I added a few outstanding patches and went down the rabbit hole
> > of trying to track down a crash with Robert's image shift patches.
> >
> > Here's how I can reliably reproduce this.
> >
> > Open a dive log in Subsurface - one of the test dives will do.
> > Right click on a dive and pick "load images"
> > Pick a few images
> > Click "select image of divecomputer showing time
> > select one
> > click OK
> >
> > This reliably overwrites memory with garbage for me. Depending how many
> > images I pick and which ones, it often leads to a crash.
> >
> > Tomaz offered to look at this as I wasted way too much time on it
> > (basically I lost all the time I had intended to cleanup the code base
> > instead). So I am checking this in, but be careful - you have been
> > warned that this overwrites memory, so don't play with it using your
> > ream data file.
> >
>
>
>
> I don’t know enough C++ to understand why this patch works but the upshot
> is: The crash was caused when lastUsedImageDir was called from another
> class and was called via on DiveList object. Making it a static function to
> be called with DiveList::lastUsedImageDir makes the crash go away.
>
> Good night
> Robert
>
> --
>
> .oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oO
> Robert C. Helling Elite Master Course Theoretical and Mathematical
> Physics
> Scientific Coordinator
> Ludwig Maximilians Universitaet Muenchen, Dept.
> Physik
> print "Just another Phone: +49 89 2180-4523 Theresienstr. 39, rm. B339
> stupid .sig\n"; http://www.atdotde.de
>
>
> ___
> subsurface mailing list
> subsurface@hohndel.org
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
>
>
___
subsurface mailing list
subsurface@hohndel.org
http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
On 17 Feb 2014, at 23:57, Dirk Hohndel wrote: > > This does not include the promised whitespace overhaul. > Instead I added a few outstanding patches and went down the rabbit hole > of trying to track down a crash with Robert's image shift patches. > > Here's how I can reliably reproduce this. > > Open a dive log in Subsurface - one of the test dives will do. > Right click on a dive and pick "load images" > Pick a few images > Click "select image of divecomputer showing time > select one > click OK > > This reliably overwrites memory with garbage for me. Depending how many > images I pick and which ones, it often leads to a crash. > > Tomaz offered to look at this as I wasted way too much time on it > (basically I lost all the time I had intended to cleanup the code base > instead). So I am checking this in, but be careful - you have been > warned that this overwrites memory, so don't play with it using your > ream data file. > 0001-make-lastUsedImageDir-static-to-prevent-a-crash.patch Description: Binary data I don’t know enough C++ to understand why this patch works but the upshot is: The crash was caused when lastUsedImageDir was called from another class and was called via on DiveList object. Making it a static function to be called with DiveList::lastUsedImageDir makes the crash go away. Good night Robert -- .oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oOo.oO Robert C. Helling Elite Master Course Theoretical and Mathematical Physics Scientific Coordinator Ludwig Maximilians Universitaet Muenchen, Dept. Physik print "Just another Phone: +49 89 2180-4523 Theresienstr. 39, rm. B339 stupid .sig\n"; http://www.atdotde.de ___ subsurface mailing list subsurface@hohndel.org http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
I'll try to debug this too. On Tue, Feb 18, 2014 at 3:41 AM, Dirk Hohndel wrote: > On Mon, 2014-02-17 at 23:34 -0300, Tomaz Canabrava wrote: > > > > > > > > On Mon, Feb 17, 2014 at 7:57 PM, Dirk Hohndel > > wrote: > > > > This does not include the promised whitespace overhaul. > > Instead I added a few outstanding patches and went down the > > rabbit hole > > of trying to track down a crash with Robert's image shift > > patches. > > > > Here's how I can reliably reproduce this. > > > > Open a dive log in Subsurface - one of the test dives will do. > > Right click on a dive and pick "load images" > > Pick a few images > > Click "select image of divecomputer showing time > > select one > > click OK > > > > This reliably overwrites memory with garbage for me. Depending > > how many > > images I pick and which ones, it often leads to a crash. > > > > Tomaz offered to look at this as I wasted way too much time on > > it > > (basically I lost all the time I had intended to cleanup the > > code base > > instead). So I am checking this in, but be careful - you have > > been > > warned that this overwrites memory, so don't play with it > > using your > > ream data file. > > > > > > This doesnt mean that I'll fix it, people, gimme a bit of help > > searching for htis. ;) > > If I gave the impression that it's only Tomaz' job to fix this then I > want to apologize. I hope that everyone with a bit of debugging > experience helps an pokes at it. > > I am doing the same - I found at least one thing that seems related and > wrong, but that alone didn't cause the issue :-( > > /D > > > ___ > subsurface mailing list > subsurface@hohndel.org > http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface > -- *Boris Barbulovski* http://mkfusion.bokicsoft.com/ ___ subsurface mailing list subsurface@hohndel.org http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
On Mon, 2014-02-17 at 23:34 -0300, Tomaz Canabrava wrote: > > > > On Mon, Feb 17, 2014 at 7:57 PM, Dirk Hohndel > wrote: > > This does not include the promised whitespace overhaul. > Instead I added a few outstanding patches and went down the > rabbit hole > of trying to track down a crash with Robert's image shift > patches. > > Here's how I can reliably reproduce this. > > Open a dive log in Subsurface - one of the test dives will do. > Right click on a dive and pick "load images" > Pick a few images > Click "select image of divecomputer showing time > select one > click OK > > This reliably overwrites memory with garbage for me. Depending > how many > images I pick and which ones, it often leads to a crash. > > Tomaz offered to look at this as I wasted way too much time on > it > (basically I lost all the time I had intended to cleanup the > code base > instead). So I am checking this in, but be careful - you have > been > warned that this overwrites memory, so don't play with it > using your > ream data file. > > > This doesnt mean that I'll fix it, people, gimme a bit of help > searching for htis. ;) If I gave the impression that it's only Tomaz' job to fix this then I want to apologize. I hope that everyone with a bit of debugging experience helps an pokes at it. I am doing the same - I found at least one thing that seems related and wrong, but that alone didn't cause the issue :-( /D ___ subsurface mailing list subsurface@hohndel.org http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
Re: latest master
On Mon, Feb 17, 2014 at 7:57 PM, Dirk Hohndel wrote: > > This does not include the promised whitespace overhaul. > Instead I added a few outstanding patches and went down the rabbit hole > of trying to track down a crash with Robert's image shift patches. > > Here's how I can reliably reproduce this. > > Open a dive log in Subsurface - one of the test dives will do. > Right click on a dive and pick "load images" > Pick a few images > Click "select image of divecomputer showing time > select one > click OK > > This reliably overwrites memory with garbage for me. Depending how many > images I pick and which ones, it often leads to a crash. > > Tomaz offered to look at this as I wasted way too much time on it > (basically I lost all the time I had intended to cleanup the code base > instead). So I am checking this in, but be careful - you have been > warned that this overwrites memory, so don't play with it using your > ream data file. > This doesnt mean that I'll fix it, people, gimme a bit of help searching for htis. ;) > > /D > > ___ > subsurface mailing list > subsurface@hohndel.org > http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface > ___ subsurface mailing list subsurface@hohndel.org http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
