Description: Undo of a delete inside a redline insert makes Writer crash. Steps to Reproduce: 1. Open a Writer document. 2. Turn on Track Changes. 3. Insert some text. E.g., insert "abc". 4. Delete text in between any 2 characters. E.g., delete "b". 5. Now Ctrl+Z results in a crash. Actual Results: Writer crashes. Expected Results: Writer doesn't crash. Reproducible: Always User Profile Reset: No Additional Info: User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36
I can't reproduce it in Version: 6.0.0.0.alpha0+ Build ID: a9588baca8137f51e2ca72e40b1f448b0e1885d1 CPU threads: 4; OS: Linux 4.8; UI render: default; VCL: gtk3; Locale: ca-ES (ca_ES.UTF-8); Calc: group nor in Versión: 5.3.4.2 Id. de compilación: f82d347ccc0be322489bf7da61d7e4ad13fe2ff3 Subproc. CPU: 1; SO: Windows 6.1; Repres. IU: predet.; Motor de trazado: HarfBuzz; Configuración regional: es-ES (es_ES); Calc: group but I can in Version: 6.0.0.0.alpha0+ Build ID: a9588baca8137f51e2ca72e40b1f448b0e1885d1 CPU threads: 1; OS: Windows 6.1; UI render: default; TinderBox: Win-x86@39, Branch:master, Time: 2017-07-21_03:03:23 Locale: es-ES (es_ES); Calc: group so it's Window only
Created attachment 134784 [details] bt with debug symbols On pc Debian x86-64 with master sources updated yesterday, I got an assert (not a crash) since I use enable-dbgutil.
Yes, it's an assert. Removing regression-related keywords, since it's not a confirmed regression, and it's hard to check (though it's possible with daily dbgutil bibisect repo in Linux). Xisco, is it correct the Linux build you tested with wasn't a debug build? My Win build failed at the same point as bug 109997, but it's a few days old, the bug might be gone (the other bug report is still open, though).
looking at commit bd37233020266a5892d6ec7022688e3dfb9cef75: this isn't obvious to me... what does the return value of AppendRedline actually mean? so i looked at all of the callers that actually check the return value, of which there are few: * doccomp.cxx: bool bRedlineAccepted = pDoc->getIDocumentRedlineAccess().AppendRedline( pDestRedl, true ); // if AppendRedline has deleted our redline, we may not keep a // reference to it if( ! bRedlineAccepted ) pDestRedl = nullptr; it looks like here we're interested in whether the actual object that was passed to AppendRedline is still alive, in order to prevent a use-after-free * unocrsrhelper.cxx: makeRedline bool bRet = pRedlineAccess->AppendRedline( pRedline, false ); pRedlineAccess->SetRedlineFlags_intern( nPrevMode ); if( !bRet ) throw lang::IllegalArgumentException(); here on the other hand we are probably interested in whether it was "successfully inserted", even if that means it was merged, so we can report to the caller if there was an "invalid redline" argument * undobj.cxx: this has the assert bool const bSuccess = rDoc.getIDocumentRedlineAccess().AppendRedline( pRedl, true ); assert(bSuccess); // SwRedlineSaveData::RedlineToDoc: insert redline failed (void) bSuccess; // unused in non-debug clearly we aren't concerned about a use-after-free here since it's the last usage of "pRedl" so it might as well consume it. the "bMerged" variable was initially introduced (as "bError") in commit 81286906d0b76a3b6c4443378877828290c3e5f0 most likely with "docx import fixes for: redlines". i find it very likely that this was targeting the unocrsrhelper.cxx "makeRedline" use case since that is called by the DOCX filter. i think this is all very confusing and the AppendRedline should not really return a boolean, but perhaps 2 booleans, or even better maybe some 3-valued enum to cover all cases. why does that assert exist anyway? it was introduced by ... me apparently! commit 33ddea2d2bdb996a7f8eb7189b311bafa0de367e Author: Michael Stahl <mst@openoffice.org> AuthorDate: Tue May 18 14:22:07 2010 +0200 sw33bf04: #i97421#: fix loop in AppendRedline: SwUndoDelete::Redo(): recreate redline save data (based on patch by majun51). SwDoc::AppendRedline(): fix loop on DELETE/INSERT without extent. well i guess the younger me wasn't entirely aware of what the return value of AppendRedline actually indicated... most likely the particular case here should be considered a success, what probably happens is that the insertion of the character inside the insert redline already expands the insert redline so adding an additional insert redline there is optimized and merged, that's all fine. so if you want to tweak your commit to fix the boolean return type of AppendRedline i'll happily merge it.
Hello Michael Stahl Feel free to restore my gerrit patch and fix the boolean return type if you want to merge it to master. No objection from my side. Here's the link: https://gerrit.libreoffice.org/#/c/40303/ Thanks!
okay, proceeded as proposed in comment #4 and comment #5 so i guess we can call this fixed now
Rosemary Sebastian committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=355715d333bced59256afb005ac9f243d37aa23e tdf#109267: Fix crash during undo of delete inside redline insert It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=9cabd72ef14e19897f4d6f078758ac8b1aa6c02f tdf#109267 sw: fix confusing return value of AppendRedline() It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Verified in Version: 6.0.0.0.alpha0+ Build ID: 78960ad06faca055a6d97afbc764c902d5d07f6f CPU threads: 1; OS: Windows 6.1; UI render: default; TinderBox: Win-x86@39, Branch:master, Time: 2017-08-30_06:31:19 Locale: es-ES (es_ES); Calc: group