View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0011039 | Scribus | General | public | 2012-08-19 18:01 | 2012-11-13 20:37 |
Reporter | chhitz | Assigned To | jghali | ||
Priority | normal | Severity | crash | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | 1.5.0svn | ||||
Fixed in Version | 1.5.0svn | ||||
Summary | 0011039: 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 Reproduce | 1. 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 Information | The 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 | ||||
Tags | No tags attached. | ||||
Patch | |||||
|
|
|
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 |
|
The patch 0001-check-ts-for-NULL.patch prevents the crash. |
|
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! |
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 |