View Issue Details

IDProjectCategoryView StatusLast Update
0011039ScribusGeneralpublic2012-11-13 20:37
Reporterchhitz Assigned Tojghali  
PrioritynormalSeveritycrashReproducibilityalways
Status closedResolutionfixed 
Product Version1.5.0svn 
Fixed in Version1.5.0svn 
Summary0011039: crash when deleting text
Description(svn r17758)

Scribus crashes when trying to delete multiple lines of text with different fonts in a text-box.
Steps To Reproduce1. open the attached crash.sla
2. delete both lines of text ('line1' and 'line2') at the same time
3. Scribus crashes with an error 11 (SIGSEGV)
Additional InformationThe in PageItem_TextFrame::deleteSelectedTextFromFram() the pointer 'ts' is NULL when ts->at(0) is called.

Stack-Trace:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000030
0x0000000100b563bc in TransactionState::sizet (this=0x0) at undomanager.cpp:753
753 return size_;
(gdb) bt
#0 0x0000000100b563bc in TransactionState::sizet (this=0x0) at undomanager.cpp:753
0000001 0x0000000100b5643d in TransactionState::at (this=0x0, index=0) at undomanager.cpp:713
0000002 0x00000001004579c9 in PageItem_TextFrame::deleteSelectedTextFromFrame (this=0x106c3c000) at pageitem_textframe.cpp:4022
0000003 0x000000010045e7a1 in PageItem_TextFrame::handleModeEditKey (this=0x106c3c000, k=0x7fff5fbff1d8, keyRepeat=@0x1078bc7f3) at pageitem_textframe.cpp:3841
0000004 0x00000001009619c5 in ScribusMainWindow::keyPressEvent (this=0x1078bc500, k=0x7fff5fbff1d8) at scribus.cpp:1607
0000005 0x0000000100ae8d49 in ScribusView::eventFilter (this=0x10fa37e20, obj=0x10fa37e20, event=0x7fff5fbff1d8) at scribusview.cpp:4318
0000006 0x0000000105731501 in QCoreApplicationPrivate::sendThroughObjectEventFilters ()
0000007 0x0000000104813588 in QApplicationPrivate::notify_helper ()
0000008 0x0000000104813c0c in QApplication::notify ()
0000009 0x0000000105733a12 in QCoreApplication::notifyInternal ()
0000010 0x0000000104812d2a in qt_sendSpontaneousEvent ()
0000011 0x0000000104885af2 in QKeyMapper::sendKeyEvent ()
0000012 0x0000000104886f3e in QKeyMapperPrivate::translateKeyEvent ()
0000013 0x00000001047ca04f in qt_dispatchKeyEvent ()
0000014 0x00000001047bcebe in -[QCocoaView keyDown:] ()
0000015 0x00007fff8e6e37ac in -[NSWindow sendEvent:] ()
0000016 0x00000001047c295f in -[QCocoaWindow sendEvent:] ()
0000017 0x00007fff8e67ca55 in -[NSApplication sendEvent:] ()
0000018 0x00000001047c6260 in -[QNSApplication sendEvent:] ()
0000019 0x00007fff8e6130c6 in -[NSApplication run] ()
0000020 0x00000001047cf10f in QEventDispatcherMac::processEvents ()
0000021 0x0000000105730e38 in QEventLoop::exec ()
0000022 0x0000000105734163 in QCoreApplication::exec ()
0000023 0x000000010151d4bc in mainApp (argc=1, argv=0x7fff5fbffc80) at main_nix.cpp:86
0000024 0x000000010151d5a0 in main (argc=2, argv=0x7fff5fbffc80) at main_nix.cpp:54
TagsNo tags attached.
Patch

Activities

chhitz

2012-08-19 18:01

reporter  

crash.sla (12,239 bytes)

chhitz

2012-08-19 18:12

reporter  

0001-check-ts-for-NULL.patch (1,564 bytes)   
From 3859d9b433763e0b50c0f16b27d268adef7e129e Mon Sep 17 00:00:00 2001
From: Christian Hitz <christian@klarinett.li>
Date: Sun, 19 Aug 2012 18:25:30 +0200
Subject: [PATCH] check ts for NULL

---
 Scribus/scribus/pageitem_textframe.cpp | 4 ++--
 1 Datei geändert, 2 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)

diff --git a/Scribus/scribus/pageitem_textframe.cpp b/Scribus/scribus/pageitem_textframe.cpp
index 5bb2c46..0ddaed5 100644
--- a/Scribus/scribus/pageitem_textframe.cpp
+++ b/Scribus/scribus/pageitem_textframe.cpp
@@ -4019,7 +4019,7 @@ void PageItem_TextFrame::deleteSelectedTextFromFrame()
 					{
 						added = false;
 						lastIsDelete = false;
-						if(is && dynamic_cast<ScItemState<CharStyle>*>(ts->at(0))->get("ETEA") == "delete_frametext" && lastPos<is->getInt("START"))
+						if(is && ts && dynamic_cast<ScItemState<CharStyle>*>(ts->at(0))->get("ETEA") == "delete_frametext" && lastPos<is->getInt("START"))
 						{
 							if(is->getItem().equiv(lastParent))
 							{
@@ -4029,7 +4029,7 @@ void PageItem_TextFrame::deleteSelectedTextFromFrame()
 							}
 							lastIsDelete = true;
 						}
-						else if(is && dynamic_cast<ScItemState<CharStyle>*>(ts->at(0))->get("ETEA") == "delete_frametext"  && lastPos>=is->getInt("START"))
+						else if(is && ts && dynamic_cast<ScItemState<CharStyle>*>(ts->at(0))->get("ETEA") == "delete_frametext"  && lastPos>=is->getInt("START"))
 						{
 							if(is && is->getItem().equiv(lastParent)){
 								is->set("TEXT_STR",is->get("TEXT_STR") + itemText.text(lastPos,i - lastPos));
-- 
1.7.11.3

0001-check-ts-for-NULL.patch (1,564 bytes)   

chhitz

2012-08-19 18:13

reporter   ~0028840

The patch 0001-check-ts-for-NULL.patch prevents the crash.

ale

2012-08-20 16:56

manager   ~0028843

while having a look at the patch i had the feeling that the condition does not feel right... way too complex... and, somehow, making it even more complex did not seem to be a good idea :-)

i had a look at the code, and while i did not fully understand it, i had the feeling that a better solution is:

if (ts)
{
    UndoTransaction trans = undoManager->beginTransaction(Um::Selection,Um::IDelete,Um::Delete,"",Um::IDelete);
    for (int i=start; i <= stop; ++i)
    {
        ....
    }
}

i was having the feeling that when ts is not set, all the content of the for loop would always be skipped...

but that's not true, since ts can be nullified in
if(!ts || !lastIsDelete){
   undoManager->action(this, is);
   ts = NULL;
}

but it still feels wrong.

i had a further look at it:

if(is && dynamic_cast<ScItemState<CharStyle>*>(ts->at(0))->get("ETEA") == "delete_frametext" && lastPos<is->getInt("START"))
else if(is && dynamic_cast<ScItemState<CharStyle>*>(ts->at(0))->get("ETEA") == "delete_frametext" && lastPos>=is->getInt("START"))

is the same as:

if(is && dynamic_cast<ScItemState<CharStyle>*>(ts->at(0))->get("ETEA") == "delete_frametext")
{
    if lastPos<is->getInt("START"))
       ...
    else
       ...
}

or am i missing something?


now, back to the if condition. while i'm still not sure if the inner check for ts is correct but is can never become null inside of the loop!

so it thought that it should probably be:

if (is)
{
    UndoTransaction trans = undoManager->beginTransaction(Um::Selection,Um::IDelete,Um::Delete,"",Um::IDelete);
    for (int i=start; i <= stop; ++i)
    {
        ....
    }
}

not true either: "is" can be null on the first run of the loop and become non null afterwards.

argh! i get more and more convinced that this method needs some more documentation, ts and is should get better names and...

... and looking at it i'm still not convinced that ts can become null if it was non null at the beginning, except in the last iteration of the loop...

pierrick or cezary: can you explain what this method is doing?


ah, and one last thing:

else if(is && dynamic_cast<ScItemState<CharStyle>*>(ts->at(0))->get("ETEA") == "delete_frametext" && lastPos>=is->getInt("START"))
{
    if(is && is->getItem().equiv(lastParent)){

the second "if is" is not necessary :-)

and one more last thing: i don't know what
    ts->pushBack(this,is);
does but i have the feeling that we should be able to cache
    ts->at(0))->get("ETEA")
and call it before the loop starts.

looking at the documentation, pushBack calls push_back, which in a qt vector would be an alias for append... (why are we using stl_vector instead of a QVector here?) and, if i understood it well, at(0) is returning the first element... which should not the one being appended... is really cacheable?

all in all: a way too complex method for a monday evening!

Issue History

Date Modified Username Field Change
2012-08-19 18:01 chhitz New Issue
2012-08-19 18:01 chhitz File Added: crash.sla
2012-08-19 18:12 chhitz File Added: 0001-check-ts-for-NULL.patch
2012-08-19 18:13 chhitz Note Added: 0028840
2012-08-20 16:56 ale Note Added: 0028843
2012-08-20 18:11 jghali Status new => resolved
2012-08-20 18:11 jghali Fixed in Version => 1.5.0svn
2012-08-20 18:11 jghali Resolution open => fixed
2012-08-20 18:11 jghali Assigned To => jghali
2012-11-13 20:37 cbradney Status resolved => closed