Bug 109267 - Assertion failed when undoing delete inside redline insert
Summary: Assertion failed when undoing delete inside redline insert
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:6.0.0
Keywords:
Depends on:
Blocks: Track-Changes Assert
  Show dependency treegraph
 
Reported: 2017-07-22 09:47 UTC by Rosemary Sebastian
Modified: 2017-08-31 09:05 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
bt with debug symbols (8.17 KB, text/plain)
2017-07-22 17:37 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rosemary Sebastian 2017-07-22 09:47:50 UTC
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
Comment 1 Xisco Faulí 2017-07-22 11:58:29 UTC
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
Comment 2 Julien Nabet 2017-07-22 17:37:20 UTC
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.
Comment 3 Aron Budea 2017-08-04 05:22:25 UTC
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).
Comment 4 Michael Stahl (CIB) 2017-08-23 13:41:03 UTC
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.
Comment 5 Rosemary Sebastian 2017-08-25 07:54:53 UTC
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!
Comment 6 Michael Stahl (CIB) 2017-08-25 21:04:00 UTC
okay, proceeded as proposed in comment #4 and comment #5

so i guess we can call this fixed now
Comment 7 Commit Notification 2017-08-25 21:04:44 UTC
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.
Comment 8 Commit Notification 2017-08-25 21:05:00 UTC
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.
Comment 9 Xisco Faulí 2017-08-30 18:12:20 UTC
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