[Libreoffice-bugs] [Bug 109267] Assertion failed when undoing delete inside redline insert

[bugzilla-daemon at bugs.documentfoundation.org]

https://bugs.documentfoundation.org/show_bug.cgi?id=109267

Michael Stahl <mstahl at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mstahl at redhat.com

--- Comment #4 from Michael Stahl <mstahl at redhat.com> ---

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 at 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/libreoffice-bugs/attachments/20170823/39549662/attachment.html>