Bug 90579 - EDITING: Crash on cancel "Tools Cell Contents Formula to Value" on more than one cell
Summary: EDITING: Crash on cancel "Tools Cell Contents Formula to Value" on more than ...
Status: CLOSED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.4.0.0.alpha2
Hardware: Other All
: high critical
Assignee: Caolán McNamara
QA Contact:
URL:
Whiteboard: target:5.3.0 target:5.2.0.1 target:5.1.5
Keywords: haveBacktrace, implementationError
Depends on:
Blocks:
 
Reported: 2015-04-12 15:27 UTC by pierre-yves samyn
Modified: 2016-09-23 07:26 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
windbg trace (23.72 KB, text/plain)
2015-04-12 15:27 UTC, pierre-yves samyn
Details
CrashCancelFormulaToValue.ods (10.88 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-04-12 15:28 UTC, pierre-yves samyn
Details
bt with debug symbols (master sources) (8.75 KB, text/plain)
2015-04-12 19:43 UTC, Julien Nabet
Details
bt with symbols (7.65 KB, text/plain)
2016-06-04 17:08 UTC, Julien Nabet
Details
here's the valgrind log (20.41 KB, text/plain)
2016-06-17 10:09 UTC, Caolán McNamara
Details

Note You need to log in before you can comment on or make changes to this bug.
Description pierre-yves samyn 2015-04-12 15:27:02 UTC
Created attachment 114746 [details]
windbg trace

Hi

Platform: Windows 7/64 & Version: 4.4.2.2
Build ID: c4c7d32d0d49397cad38d62472b0bc8acff48dd6
Locale: fr_FR

Crash on cancel "Tools contains Cell Formula to Value" on more than one cell

Steps to reproduce:
1. Open the CrashCancelFormulaToValue.ods attached 
2. Select C1:C2 (2 cells containing formula)
3. Tools> Cell Contents> Formula to Value

Expected & actual result: no more formula in C1:C2

4. Edit> Undo or Ctrl+Z

Expected result: formulas restore
Actual result: crash (see windbg.txt)


Regards
Comment 1 pierre-yves samyn 2015-04-12 15:28:47 UTC
Created attachment 114747 [details]
CrashCancelFormulaToValue.ods
Comment 2 raal 2015-04-12 18:08:02 UTC
I can confirm with Version: 4.5.0.0.alpha0+
Build ID: b024e36ddb3b53163d7a01f6f7b5aadb7a858cd9
TinderBox: Linux-rpm_deb-x86_64@46-TDF, Branch:master, Time: 2015-03-31_09:12:20
Comment 3 Julien Nabet 2015-04-12 19:43:51 UTC
Created attachment 114751 [details]
bt with debug symbols (master sources)

On pc Debian x86-64 with master sources updated today, I could reproduce this.
I attached a bt of the crash. Quite strangely, the bt is quite different than Pierre-Yves's bt but perhaps it's because his concerns 4.4 branch and mine master branch.
Comment 4 raal 2015-04-12 19:59:50 UTC
Crash with Version: 4.4.0.0.alpha2+
Build ID: 3f94c9e9ddfd807b449f3bb9b232cf2041fa12d2

New feature introduced in 4.4  https://wiki.documentfoundation.org/ReleaseNotes/4.4#Direct_conversion_of_formulas_into_static_values
Comment 5 Robinson Tryon (qubit) 2015-12-10 10:05:08 UTC Comment hidden (obsolete)
Comment 6 Julien Nabet 2016-06-04 15:57:56 UTC
On LO Debian package 5.1.3.2, I don't see the option to give a new try.
Comment 7 pierre-yves samyn 2016-06-04 16:22:49 UTC
Hi

(In reply to Julien Nabet from comment #6)
> On LO Debian package 5.1.3.2, I don't see the option to give a new try.

Thank you Julien,

New steps to reproduce:

1. Open the CrashCancelFormulaToValue.ods attached 
2. Select C1:C2 (2 cells containing formula)
3. Data> Calculate> Formula to Value

Expected & actual result: no more formula in C1:C2

4. Edit> Undo or Ctrl+Z

Expected result: formulas restore
Actual results: 

C1: String displayed ok (2310 as expected) but formula (formula bar) not ok (contains just =)
C2: Err:2843

5. File> Close> Don't save: crash

Note: errors are not always the same (sometimes Err:2805 or 2852..., sometimes C1 also displays error...)

Regards
Pierre-Yves
Comment 8 Julien Nabet 2016-06-04 17:08:33 UTC
Created attachment 125485 [details]
bt with symbols

Thank you Pierre-Yves for your feedback.

On pc Debian x86-64 with master sources updated today, I could reproduce this.
Comment 9 Julien Nabet 2016-06-04 17:22:04 UTC
Eike: thought you might be interested in this one.
Comment 10 Julien Nabet 2016-06-04 17:23:05 UTC
Some gdb additional info:
(gdb) frame 2
#2  0x00002aaad4d29cb9 in (anonymous namespace)::GroupFormulaCells::operator() (this=0x7fffffff2d30, node=...)
    at /home/julien/lo/libreoffice/sc/source/core/data/column3.cxx:2954
2954	        ScFormulaCellGroupRef xPrevGrp = pPrev->GetCellGroup();
(gdb) p node
$1 = (
    mdds::multi_type_vector<mdds::mtv::custom_block_func3<mdds::mtv::default_element_block<52, svl::SharedString>, mdds::mtv::noncopyable_managed_element_block<53, EditTextObject>, mdds::mtv::noncopyable_managed_element_block<54, ScFormulaCell> >, mdds::detail::mtv_event_func>::value_type &) @0x7fffffff2fc0: {type = 54, position = 0, size = 2, 
  data = 0x8e16e50, __private_data = {block_index = 0}}
(gdb) frame 5
#5  0x00002aaad4d3ec77 in ScColumn::SwapNonEmpty (this=0x2bbc150, rValues=..., rStartCxt=..., rEndCxt=...) at /home/julien/lo/libreoffice/sc/source/core/data/column4.cxx:456
456	    RegroupFormulaCells();
(gdb) p rValues
$2 = (sc::TableValues &) @0x8d80388: {mpImpl = std::unique_ptr<sc::TableValues::Impl> containing 0x8d7a690}
(gdb) p rStartCxt
$3 = (sc::StartListeningContext &) @0x7fffffff34b0: {mrDoc = @0x2a811a0, mpSet = std::shared_ptr (count 3, weak 0) 0x8e17ab0, mpColSet = std::shared_ptr (empty) 0x0}
(gdb) p rEndCxt
$4 = (sc::EndListeningContext &) @0x7fffffff34e0: {mrDoc = @0x2a811a0, maSet = {maDoc = std::__debug::vector of length 0, capacity 0, mbInit = false}, 
  mpPosSet = std::shared_ptr (count 3, weak 0) 0x8e17ab0, mpOldCode = 0x0, maPosDelta = {nRow = 0, nCol = 0, nTab = 0, static detailsOOOa1 = {
      eConv = formula::FormulaGrammar::CONV_OOO, nRow = 0, nCol = 0}}}
(gdb)
Comment 11 Caolán McNamara 2016-06-17 10:09:33 UTC
Created attachment 125703 [details]
here's the valgrind log

The mdds stuff really confuses the hell out of this bt
Comment 12 Caolán McNamara 2016-06-17 10:53:32 UTC
hmm, if I do the conversion 1 cell at a time and then undo each cell one at a time its ok
Comment 13 Caolán McNamara 2016-06-17 13:58:07 UTC
I'm reasonably confident this is a problem in mdds. On the undo we go from a single block of no-formulas to two blocks of one formula each.

so

in include/mdds/multi_type_vector_def.inl we enter ::swap_single_to_multi_blocks

and do...

    // Get the new elements from the other container.
    blocks_type new_blocks;
    other.exchange_elements(
        *blk_src->mp_data, src_offset, dst_block_index1, dst_offset1, dst_block_index2, dst_offset2, len, new_blocks);

in ::exchange_elements

after 

    element_block_func::assign_values_from_block(*blk->mp_data, src_data, src_offset, len);

in gdb

((const mdds::mtv::noncopyable_managed_element_block<54, ScFormulaCell> *)(m_blocks[0]->mp_data))->m_array[0]

has the value of the ScFormulaCell*

on return back to ::swap_single_to_multi_blocks we go to the src_offset == 0 and src_tail_len == 0 case which is 

        if (src_tail_len == 0)
        {
            // the whole block needs to be replaced.
            delete_block(blk_src);
            m_blocks.erase(m_blocks.begin()+block_index);
        }

((const mdds::mtv::noncopyable_managed_element_block<54, ScFormulaCell> *)(blk_src->mp_data))->m_array[0]

confirms that our ScFormulaCell* is here also and delete_block deletes the full block and deletes the m_array which deletes the ScFormulaCell* and so its deleted there while also transferred to the new_block above.

The new block goes on to be inserted, but its all broken now because it refers to dead data.

Presumably we need to either clear the part of the block that will be deleted of its transferred data at exchange time. Or overwrite its data with zero. Or swap at exchange time instead of copy. Or something of that nature anyway.
Comment 14 Commit Notification 2016-06-17 15:44:27 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9ec54e92407cd632c4e38317f914edd557835a86

Resolves: tdf#90579 swap_single_to_multi_blocks seems broken

It will be available in 5.3.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 15 Commit Notification 2016-06-17 15:46:02 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-5-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6114072a59cfff36218aea70e1b52fa4c3ba64b4&h=libreoffice-5-2

Resolves: tdf#90579 swap_single_to_multi_blocks seems broken

It will be available in 5.2.0.1.

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 16 Caolán McNamara 2016-06-17 15:50:06 UTC
Sent my effort to Kohei as https://gitlab.com/mdds/mdds/merge_requests/2 hopefully he'll have some ideas if that's the right solution or not. https://gerrit.libreoffice.org/26429 for 5-1 in the meantime
Comment 17 Commit Notification 2016-06-22 10:36:08 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=19312e13c4ab802c2bd878f4448556f245884a13&h=libreoffice-5-1

Resolves: tdf#90579 swap_single_to_multi_blocks is broken

It will be available in 5.1.5.

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 18 pierre-yves samyn 2016-09-23 07:26:03 UTC
Hi

Verified on windows 7/64 & Version: 5.2.0.1 (x64)
Build ID: fcbcb4963bda8633ba72bd2108ca1e802aad557d
CPU Threads: 2; OS Version: Windows 6.1; UI Render: default; 
Locale: fr-FR (fr_FR)

Sorry for the delay and thank you 

Regards
Pierre-Yves