Bug 105002 - MailMerge: Writer crashes using wizard 2nd time thru
Summary: MailMerge: Writer crashes using wizard 2nd time thru
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
5.3.0.0.alpha1+
Hardware: All All
: highest critical
Assignee: Maxim Monastirsky
QA Contact:
URL:
Whiteboard: target:5.5.0 target:5.4.0.1 target:5.3.4
Keywords: bibisected, bisected, haveBacktrace, regression
Depends on:
Blocks: Mail-Merge
  Show dependency treegraph
 
Reported: 2016-12-30 12:51 UTC by Alex Kempshall
Modified: 2017-05-30 22:42 UTC (History)
7 users (show)

See Also:
Crash report or crash signature: ["Dialog::EndDialog(long)"]


Attachments
Starting Document for replicating bug (12.24 KB, application/vnd.oasis.opendocument.text)
2016-12-30 12:53 UTC, Alex Kempshall
Details
Address List for replicating bug (7.61 KB, text/comma-separated-values)
2016-12-30 12:54 UTC, Alex Kempshall
Details
bt with symbols (6.77 KB, text/plain)
2016-12-30 14:54 UTC, Julien Nabet
Details
bt with debug symbols (6.27 KB, text/plain)
2017-05-24 19:46 UTC, Julien Nabet
Details
bts from Dialog::ImplInitDialogData and Dialog::dispose (28.35 KB, application/zip)
2017-05-24 20:17 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Kempshall 2016-12-30 12:51:59 UTC
In Writer when using the Mail Merge wizard to save a file. The first time through in a session is successful. The second time, in the same session, LibreOffice crashes. 


Steps to reproduce the problem.


1.
Download the files CompanySeedStarted1.odt and CompanySeedTestData1.csv from theis bug report

2.
Start LibreOffice writer with the document CompanySeedStarted1.odt downloaded in step 1.

3.
Tools -> Mail Merge Wizard

4.
The "Select starting document" dialog box should appear. The left pane should contain 5 steps. In the right pane select the "Use Current Document" Radio Button. Then press the "Next >>" Button.

5.
The "Select document type" dialog box should appear. Select the "Letter" Radio Button. Then press the "Next" Button.

6.
The "Insert address block" dialog box should appear. Press the "Select Different Address List" Button.

7.
The "Select Address List" dialog box should appear. Press the "Add" Button.

8.
The "Open - LibreOffice" dialog should appear. Navigate to the csv file downloaded in step 1. Once the file has been selected press the "Open" Button. The "Text Connections Settings" dialog should open. The csv file is standard so accept the defaults and Press the "OK" Button.

9.
The "Select Address List" dialog box should re-appear with the database CompanySeedTestData1 added to the list of available lists. If not already selected - select this list. Press the "OK" Button.

10.
Should be returned to the "Mail Merge Wizard" dialog. Press the "Finish" Button.

11.
The "Mail Merge" Toolbar should no be available.

12.
On the "Mail Merge" Toolbar Press the "Save Merged Documents" Button. 

13.
The "Save Merged Documents" dialog should appear. If not already selected select the "Save as a single large document" Radio Button. Then Press the "Save Documents" Button. 

14.
A progress message box will appear. This message box will close when all entries in the Address List have been merged.

15.
The "Save - LibreOffice" dialog should open. Give select an appropriate folder in which to save the merged file and give it a name. Press the "Save" Button. 

16.
Tools -> Mail Merge Wizard

17.
The "Select starting document" dialog box should appear. The left pane should contain 5 steps. In the right pane select the "Use Current Document" Radio Button. Then press the "Next >>" Button.

18.
Libreoffice goes into recovery mode.


Alex
Comment 1 Alex Kempshall 2016-12-30 12:53:40 UTC
Created attachment 130028 [details]
Starting Document for replicating bug
Comment 2 Alex Kempshall 2016-12-30 12:54:37 UTC
Created attachment 130029 [details]
Address List for replicating bug
Comment 3 Julien Nabet 2016-12-30 14:54:57 UTC
Created attachment 130033 [details]
bt with symbols

On pc Debian x86-64 with master sources updated today, I could reproduce this.
I don't know why crashreport details don't display much details but I'll attach a bt.
Comment 4 Julien Nabet 2016-12-30 14:56:30 UTC
Jan-Marek: thought you might be interested in this one since it concerns MM.
Comment 5 Alex Kempshall 2016-12-30 15:34:26 UTC
The following might help Jan-Marek

A)
If in step 17 instead of pressing the "Next >>" Button the "Finish" button is pressed, the dialog will close successfully. This third call of the Wizard, in this current session, won't crash. This procedure has to be repeated each time there's a successful merge and the Wizard is restarted in the current session.

B)
Remove this code in sw/source/ui/dbui/mailmergewizard.cxx !!!!!!!

    if (m_xConfigItem->GetTargetView())
    {
        //close the dialog, remove the target view, show the source view
        m_nRestartPage = _nState;
        //set ResultSet back to start
        m_xConfigItem->MoveResultSet(1);
        EndDialog(RET_REMOVE_TARGET);
        return;
    }

Alex
Comment 6 Julien Nabet 2016-12-30 15:38:13 UTC
Alex: I noticed an old commit from you here:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=ddafafe5f91e565be418e20eb7e447f8b1849f94

Would it be possible you submit a patch to review on gerrit (see https://wiki.documentfoundation.org/Development/GetInvolved)?
Comment 7 Alex Kempshall 2016-12-30 16:33:41 UTC
Julien

The commit 

https://cgit.freedesktop.org/libreoffice/core/commit/?id=ddafafe5f91e565be418e20eb7e447f8b1849f94

is from way back in 2011 and I would suggest unrelated to this problem. It looks to be that it was committed, in 2011, on my behalf by Norbert Thiebaud. I've not come across the problem it was to fix since then. So I suggest that patch has been applied or the that bug solved in some other way.

Is there something I'm missing as to why it should be reviewed by gerrit?

Alex
Comment 8 Julien Nabet 2016-12-30 17:55:04 UTC
(In reply to Alex Kempshall from comment #7)
> Julien
> 
> The commit 
> 
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=ddafafe5f91e565be418e20eb7e447f8b1849f94
> 
> is from way back in 2011 and I would suggest unrelated to this problem. It
> looks to be that it was committed, in 2011, on my behalf by Norbert
> Thiebaud. I've not come across the problem it was to fix since then. So I
> suggest that patch has been applied or the that bug solved in some other way.
> 
> Is there something I'm missing as to why it should be reviewed by gerrit?

Sorry, I indeed wasn't clear enough. In my previous message I meant you had contributed on LO (even if it's one time) but thought you'd be interested to contribute again.
Indeed, you've already made the analysis and found a code pointer (perhaps the root cause).
I provided the link about gerrit because the process must have changed a bit after 5 years.
Comment 9 Xisco Faulí 2017-05-10 09:18:40 UTC
Reproduced in

Versión: 5.3.2.2
Id. de compilación: 6cd4f1ef626f15116896b1d8e1398b56da0d0ee1
Subproc. CPU: 1; SO: Windows 6.1; Repr. de IU: predet.; Motor de trazado: HarfBuzz; 
Configuración regional: es-ES (es_ES); Calc: group
Comment 10 Xisco Faulí 2017-05-10 09:54:03 UTC
Regression introduced by:

author	Noel Grandin <noel@peralex.com>	2016-09-21 12:48:15 (GMT)
committer	Noel Grandin <noel.grandin@collabora.co.uk>	2016-10-27 06:08:30 (GMT)
commit	eca5ea9f79181d45cd7fbabe2313617d3025818a (patch)
tree	10b5837fe04212349825742b38f5a37be9ce7009
parent	bbd44f8f89839b5abb4ec6c7ea195431de5b2f48 (diff)
make the AbstractDialog stuff extend from VclReferenceBase
Because some stuff wants to multiple-inherit from VclAbstractDialog and
OutputDevice-subclasses, and we'd prefer to keep all the lifetime
management through a single smart pointer class (VclPtr)

The change in msgbox.cxx and window.cxx is to workaround a bug in
VS2013 to do with virtual inheritance and delegating constructors.

Bisected with bibisect-linux-64-5.3

Adding Cc: to Noel Grandin
Comment 11 Noel Grandin 2017-05-10 14:09:06 UTC
Sorry, I don't know what is going on here.

The immediate crash is a familiar one - the code in sw is deleting a dialog from inside the end-of-dialog-callback handler, which means the calling code finds itself executing on a deleted object.

I worked around that by adding a 
   VclPtr<Dialog> aThis; 
inside the EndDialog method, but then it crashes elsewhere inside wizard code.

The lifecycle of all these wizards and dialogs involved in mailmerge is somewhat... interesting.
Comment 12 Michael Meeks 2017-05-23 11:51:11 UTC
Stephan - did you have any advanced C++ wizardry input here ? =)
Comment 13 Julien Nabet 2017-05-24 19:46:51 UTC
Created attachment 133543 [details]
bt with debug symbols

On pc Debian x86-64 with master sources updated today, I still reproduce the crash (as expected), so let's update bt.
Comment 14 Julien Nabet 2017-05-24 20:17:30 UTC
Created attachment 133544 [details]
bts from Dialog::ImplInitDialogData and Dialog::dispose

To try to find root cause, I put breakpoints on Dialog::ImplInitDialogData and Dialog::dispose in vcl/source/window/dialog.cxx

I retrieved 12 bts (including the crash one).
The first eight bts (until step15 included) seem ok.

Then step 16 and step 17 create an object each. The pb is the object of step 17 is reset to nullptr then called.
Comment 15 Julien Nabet 2017-05-24 20:40:18 UTC
On 3 last bts (including the crash), we can notice this line:
sw/source/ui/dbui/mailmergewizard.cxx:136
which corresponds to 
EndDialog(RET_REMOVE_TARGET);
from the block indicated by Alex in https://bugs.documentfoundation.org/show_bug.cgi?id=105002#c5
(See http://opengrok.libreoffice.org/xref/core/sw/source/ui/dbui/mailmergewizard.cxx#126)

After having commented the quoted if block, I don't reproduce the crash anymore.
Comment 16 Julien Nabet 2017-05-24 21:09:03 UTC
For the record about if block quoted in my previous comment, _nState=0 at this location.

Jan: noticing your work about MM between the end of 2015 and beginning of 2016, thought you might have some useful thoughts here.
Comment 17 Maxim Monastirsky 2017-05-24 23:42:00 UTC
Unless I'm missing something, this seems to do the trick:

https://gerrit.libreoffice.org/38007
Comment 19 Julien Nabet 2017-05-25 15:56:01 UTC
I don't know if we can considered it as FIXED or if we should wait for backport at least in 5.4 branch.
Comment 20 Jan Holesovsky 2017-05-25 16:38:30 UTC
Good idea, cherry-picked that:

https://gerrit.libreoffice.org/#/c/38036
https://gerrit.libreoffice.org/#/c/38037
Comment 21 Commit Notification 2017-05-25 23:56:50 UTC
Maxim Monastirsky committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=5cabee5488d63281ad6b2f96d2cafa513c065640&h=libreoffice-5-4

tdf#105002 Don't crash on mail wizard recreation

It will be available in 5.4.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 22 Commit Notification 2017-05-25 23:59:17 UTC
Maxim Monastirsky committed a patch related to this issue.
It has been pushed to "libreoffice-5-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6b921e1c8a4779dfd6f5531842ab89d85aa99a85&h=libreoffice-5-3

tdf#105002 Don't crash on mail wizard recreation

It will be available in 5.3.4.

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 23 Xisco Faulí 2017-05-30 22:42:33 UTC
Verified in

Version: 5.5.0.0.alpha0+
Build ID: 36b1e6270bf2fbb333e2a69c4bb5931eba418289
CPU threads: 1; OS: Windows 6.1; UI render: default; 
TinderBox: Win-x86@62-TDF, Branch:MASTER, Time: 2017-05-29_14:06:19
Locale: es-ES (es_ES); Calc: group