Bug 125520 - EDITING: Calc: Error when dragging and dropping OLE objects with 'Insert as Copy'
Summary: EDITING: Calc: Error when dragging and dropping OLE objects with 'Insert as C...
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
6.1.0.0.alpha1+
Hardware: All All
: medium minor
Assignee: Armin Le Grand
URL:
Whiteboard: target:7.0.0 target:6.4.3
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2019-05-27 10:20 UTC by Andrés Maldonado
Modified: 2020-03-17 13:06 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
chart.ods (12.13 KB, application/vnd.oasis.opendocument.spreadsheet)
2019-05-27 10:22 UTC, Andrés Maldonado
Details
6.1_bibisect.log (2.37 KB, text/x-log)
2019-06-04 15:49 UTC, Andrés Maldonado
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrés Maldonado 2019-05-27 10:20:29 UTC
Description:
(I already have a fix, but I'm opening this to provide a detailed description)

Steps to Reproduce:
1. Open the attached file 'chart.ods'
2. Open a new Calc window
3. Open the Navigator and change the drag mode to 'Insert as Copy'
4. At the bottom of the Navigator, select 'chart (inactive)'
5. Unfold 'OLE objects' and drag&drop 'Object 1' to the sheet

Actual Results:
The chart is not inserted. Only it's contour is visible.

Expected Results:
The chart should be visible


Reproducible: Always


User Profile Reset: Yes


OpenGL enabled: Yes

Additional Info:
This is a regression. A bibisect shows that the first bad commit is 91b0d2122bde (https://gerrit.libreoffice.org/#/c/53933/)

After a good time debugging, I found that in svx/source/svdraw/svdxcgv.cxx:745 the argument given to CloneSdrObject is not the correct one.
Comment 1 Andrés Maldonado 2019-05-27 10:22:13 UTC
Created attachment 151700 [details]
chart.ods
Comment 2 Andrés Maldonado 2019-05-27 10:31:04 UTC
The fix: https://gerrit.libreoffice.org/#/c/73026/
Comment 3 Oliver Brinzing 2019-05-28 17:37:38 UTC
confirming
Comment 4 Xisco Faulí 2019-05-30 11:52:17 UTC
Not reproducible in

Version: 5.2.0.0.alpha1+
Build ID: 5b168b3fa568e48e795234dc5fa454bf24c9805e
CPU Threads: 4; OS Version: Linux 4.15; UI Render: default; 
Locale: ca-ES (ca_ES.UTF-8)
Comment 5 Andrés Maldonado 2019-06-04 15:45:59 UTC
This is reproducible in

Version: 6.3.0.0.beta1 (x64)
Build ID: a187af327633f5f00363be5131bd21a13e0f1a7b
CPU threads: 2; OS: Windows 10.0; UI render: default; VCL: win; 
Locale: en-US (en_US); UI-Language: en-US
Calc: threaded

and in 

Version: 6.1.0.1 (x64)
Build ID: 378e26bd4f22a135cef5fa17afd5d4171d8da21a
CPU threads: 2; OS: Windows 10.0; UI render: default; 
Locale: en-US (en_US); Calc: group threaded

but not in

Version: 6.0.7.3 (x64)
Build ID: dc89aa7a9eabfd848af146d5086077aeed2ae4a5
CPU threads: 2; OS: Windows 10.0; UI render: default; 
Locale: en-US (en_US); Calc: group

I did a bibisect in branch 6.1 and found that the first bad commit is 91b0d2122bde (https://gerrit.libreoffice.org/#/c/53933/)
Comment 6 Andrés Maldonado 2019-06-04 15:49:27 UTC
Created attachment 151912 [details]
6.1_bibisect.log

Here is the bibisect log
Comment 7 raal 2019-06-06 15:00:36 UTC
This seems to have begun at the below commit.
Adding Cc: to Armin Le Grand; Could you possibly take a look at this one? Thanks

author	Armin Le Grand <Armin.Le.Grand@cib.de>	2018-05-07 11:44:26 +0200
committer	Armin Le Grand <Armin.Le.Grand@cib.de>	2018-05-08 01:53:46 +0200
commit	91b0d2122bdee361bf5412a42d48ff051159cbf2 (patch)
tree	003ef60b93668847803a812486534ebc805e6546
parent	bdccb7e9991d83029eb2f2f11327b54534a00db8 (diff)
tdf#116977 secured ::Clone methods
Comment 8 Commit Notification 2019-09-08 00:22:11 UTC
Andrés Maldonado committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/+/e4cea049c80f4fd6d2a586e73fe9fa08ebd08371%5E%21

tdf#125520 Fix OLE objects drag&drop with 'Insert as Copy'

It will be available in 6.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 9 Xisco Faulí 2019-12-03 10:57:57 UTC
A polite ping to Andrés Maldonado:
Is this bug fixed? if so, could you please close it as RESOLVED FIXED ?
Otherwise, Could you please explain what's missing?
Thanks
Comment 10 Commit Notification 2020-02-12 21:29:47 UTC
Xisco Faulí committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/09e6824bc868990095233825c415556399dd0652

tdf#130614: Revert "tdf#125520 Fix OLE objects drag&drop with 'Insert as Copy'"

It will be available in 7.0.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 11 Commit Notification 2020-02-13 06:42:09 UTC
Xisco Faulí committed a patch related to this issue.
It has been pushed to "libreoffice-6-4":

https://git.libreoffice.org/core/commit/d3e77bd112c3f7084b0dd92c36ff60f1fd1ddc2f

tdf#130614: Revert "tdf#125520 Fix OLE objects drag&drop with 'Insert as Copy'"

It will be available in 6.4.2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Commit Notification 2020-02-18 09:03:53 UTC
Xisco Faulí committed a patch related to this issue.
It has been pushed to "libreoffice-6-4-1":

https://git.libreoffice.org/core/commit/25937732b9ef881651611f99d4178e90888642a2

tdf#130614: Revert "tdf#125520 Fix OLE objects drag&drop with 'Insert as Copy'"

It will be available in 6.4.1.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 13 Armin Le Grand 2020-02-24 20:34:31 UTC
grepping, looks as if just target SdrModel is wrong...
Comment 14 Armin Le Grand 2020-03-04 16:24:01 UTC
svx/source/svdraw/svdxcgv.cxx:745 is the wrong place - it creates the clone that is visualized during D&D itself. The real clone gets created in SdrExchangeView::Paste line 300:

            SdrObject* pNewObj(pSrcOb->CloneSdrObject(*mpModel));

All the target SdrModel ptrs are correct. Scenario is as follows:
(1) D&D starts, a temp SdrModel gets created with a single page, the SdrModel gets cloned to there (svdxcgv.cxx:745)
(2) At end of D&D that clone gets cloned to target SdrModel and SdrPage

Using such a simplified SdrModel for D&D/Copy&Paste is standard - it gets saved/loaded in clipboard using that mechanism.

Problem seems more that already at the 1st clone the EmbeddedObjectContainer stuff gets not copied because

        ::comphelper::IEmbeddedHelper* pDestPers(getSdrModelFromSdrObject().GetPersist());
        ::comphelper::IEmbeddedHelper* pSrcPers(rObj.getSdrModelFromSdrObject().GetPersist());
        if( pDestPers && pSrcPers )
        {

nullptr == pDestPers which is probably because the temp SdrModel does indeed not have Persist data (aka associated file AFAIK?).

Thus my guess to fix that is that the in-between SDrModel created at SdrExchangeView::CreateMarkedObjModel()::703 somehow would need a Persist in OLE cases...?
Comment 15 Armin Le Grand 2020-03-04 16:39:33 UTC
Comment in code:
// tdf#125520 - temp SdrModel will need a comphelper::IEmbeddedHelper
// to succesfully clone the OLE content,  use the one from source model
// in the temporary SdrModel - it gets not deleted in SdrModel destructor.
// As long as the temporary SdrModel is used temprary (and does NOT get
// extended to a full document) this *should* work. There stay some
// concerns about what may happen in BG and if saved/loaded from clipboard,
// so this *might* need to be enhanced in the future.

Works now with 2nd chart (tried Draw/Impress/Writer, but not possible there using Navigator), so let's use this
Comment 16 Commit Notification 2020-03-04 19:55:16 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/d08d5c1857482cb3789ed2896921abeb83f4d217

tdf#125520 enhance internal OLE cloning

It will be available in 7.0.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 17 Xisco Faulí 2020-03-10 09:13:03 UTC
Verified in

Version: 7.0.0.0.alpha0+
Build ID: a11c10a83f6fceae6cfb519725d06f8eaf1013fb
CPU threads: 4; OS: Linux 4.19; UI render: default; VCL: gtk3; 
Locale: en-US (en_US.UTF-8); UI-Language: en-US
Calc: threaded

@Armin, thanks for fixing this issue!!
Comment 18 Xisco Faulí 2020-03-10 09:18:22 UTC
Hi Armin,
Actually your commit < d08d5c1857482cb3789ed2896921abeb83f4d217 > reintroduced bug 130614. Changing it to REOPENED. Could you please take a look ?
Comment 19 Armin Le Grand 2020-03-10 12:46:20 UTC
Hi Xisco - will have a look ASAP
Comment 20 Armin Le Grand 2020-03-10 13:00:05 UTC
This is really bad - like a hen/egg problem :-( The temp model has no own comphelper::IEmbeddedHelper, so it cannot clone chart contents. If using the one from source document, it will die when that gets closed - sigh...
Comment 21 Armin Le Grand 2020-03-10 14:42:29 UTC
There are already some quite 'creative' parts in that problem area, when looking at ::AllocModel, e.g.

SdDrawDocument::AllocModel() uses AllocSdDrawDocument() which uses mpCreatingTransferable and comments like
        // Document is created for drag & drop/clipboard. To be able to
        // do this, the document has to know a DocShell (SvPersist).
show that this is not the 1st problem of this kind.

Calc uses something like

    static SfxObjectShell* pGlobalDrawPersist;          // for AllocModel

which gets set once and then consumed, see ScDrawLayer::SetGlobalDrawPersist and usages - a global, use-once parameter - argh.

But this leads at least to ScDrawLayer::AllocModel() and pGlobalDrawPersist would be used if it would have been set. Checking where this is set (ScDrawLayer::SetGlobalDrawPersist)...
Comment 22 Armin Le Grand 2020-03-10 14:54:11 UTC
It is e.g. used in ScDrawView::BeginDrag() in sc\source\ui\view\drawvie4.cxx where also the case using CTRL+C starts which is ScDrawView::DoCopy(). Thus the former change in SdrExchangeView::CreateMarkedObjModel()

                pNewModel->SetPersist(mpModel->GetPersist());

is even wriong - it replaces the already set persist with the source document.

This all shows that this fix was wrong. It seems as if there are missing mechanisms for the original case in the task - to start D&D using the Navigator. Checking...
Comment 23 Armin Le Grand 2020-03-10 15:21:15 UTC
Adapted lcl_DoDragObject in sc\source\ui\navipi\content.cxx as needed to support the existing SetGlobalDrawPersist mechanism. Removed changes in SdrExchangeView::CreateMarkedObjModel() which were wrong and replaced with a warning if no Persist is given and cloned object is an OLE. Change is on gerrit (see https://gerrit.libreoffice.org/c/core/+/90263).
Comment 24 Commit Notification 2020-03-10 19:07:41 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/f9e2cfc0d2e244c1550a0e2cc8de960f82eaf9cf

tdf#125520 create a persist correctly for OLE

It will be available in 7.0.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 25 Xisco Faulí 2020-03-11 15:03:20 UTC
Issue verified in

Version: 7.0.0.0.alpha0+
Build ID: dde0cf9eafed91c88d9999a294ea32fe0e8aa4bc
CPU threads: 4; OS: Linux 4.19; UI render: default; VCL: gtk3; 
Locale: en-US (en_US.UTF-8); UI-Language: en-US
Calc: threaded

@Armin, thanks for fixing this issue! Can we close it as RESOLVED FIXED ?
Comment 26 Commit Notification 2020-03-17 13:06:14 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "libreoffice-6-4":

https://git.libreoffice.org/core/commit/90f57175abf31ebad850c9653b41eccd230893b8

tdf#125520 create a persist correctly for OLE

It will be available in 6.4.3.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.