Bug 126742 - writer: OLE object resized incorrectly
Summary: writer: OLE object resized incorrectly
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All Windows (All)
: medium normal
Assignee: Armin Le Grand
URL:
Whiteboard: target:7.0.0 target:6.4.3 target:7.2....
Keywords:
Depends on:
Blocks: OLE-Objects
  Show dependency treegraph
 
Reported: 2019-08-07 06:34 UTC by Vasily Melenchuk (CIB)
Modified: 2023-10-27 10:00 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vasily Melenchuk (CIB) 2019-08-07 06:34:30 UTC
Description:
With given below test scenario height of OLE object is changing in an unexpected way.

Steps to Reproduce:
1. Create new calc doc with sample data (I've used a simple table with data 3x3), save it but do not close document.
2. Create new writer doc
3. Menu: Insert -> Object -> OLE object
4. Select "create from file" and select calc file saved on step 1.
5. Click option "Link to file"
6. Double click on inserted OLE object
7. Increase height of object
8. Click outside of OLE object to close editing
9. Double click on inserted object once again

Actual Results:
Opened object height is much more than expected and does not corresponds object height.

Expected Results:
During editing no unexpected height changes are visible.


Reproducible: Always


User Profile Reset: No



Additional Info:
Comment 1 Xisco Faulí 2019-11-21 12:33:31 UTC
Dear Vasily Melenchuk,
This bug has been in ASSIGNED status for more than 3 months without any
activity. Are you still working on this issue ?
Comment 2 Vasily Melenchuk (CIB) 2019-11-21 14:54:32 UTC
Patch fixing this problem is in Gerrit https://gerrit.libreoffice.org/#/c/77002/
However it is crashing several unit tests
Comment 3 Commit Notification 2020-03-11 07:59:12 UTC
Samuel Mehrbrodt committed a patch related to this issue.
It has been pushed to "master":

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

tdf#126742 Don't change visible area while retrieving it

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 4 Commit Notification 2020-03-16 17:28:26 UTC
Samuel Mehrbrodt committed a patch related to this issue.
It has been pushed to "libreoffice-6-4":

https://git.libreoffice.org/core/commit/67a52b17b51495a6112f42de7cb465d69f2bb139

tdf#126742 Don't change visible area while retrieving it

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.
Comment 5 Armin Le Grand 2021-03-01 09:41:35 UTC
Still happens in "libreoffice-6-4" despite backport of comment 4.
Need deeper look - indeed, *relies* on step (1) keeping calc file open. Guess is that somewhere in-between a scale/adaption of units between SC/SW is missing, sc works in100thmm while sw works in twips. Need to find by debugging into it at OLE activation between apps - sigh...
Comment 6 Armin Le Grand 2021-03-01 16:06:35 UTC
hecked usage of m_bResizeNoScale in  SfxInPlaceClient_Impl - this should be used to nit show the content scaled when deactivated anyways, and helps on activation to not scale, but show the correct content.
Also checked on SfxBaseModel::setVisualAreaSize/SfxBaseModel::getVisualAreaSize and it seems that setVisualAreaSize/ is not called (by purpose?) for the resize. That again may cortrespond with the initially removed c185263f45a556e6c695c766476e67fbd2ea3593 call to SfxObjectShell::SetVisArea.
Need to compare with master version...
Comment 7 Armin Le Grand 2021-03-01 16:30:33 UTC
Compare with master version shows that this happens on Windows-Master, too (!)
This is a surprise, I checked this task already.
Re-checking on Linux shows that it does indeed *not* happen there on master, this is very strange(!) but I double-checked.
Also the scaling-the-content error does not happen on Linux but on Windows, so it seems we have a system-dependent behaviour and error here - this does not make it easier ;-(
Re-checking now, will probably have to live-compare on both systems (but will be able to work on master?)...
Also re-checking: Does it go wrong on Linux 6.4 version?
->Happens on Windows master
->Happens on Windows 6.4
->Not on Linux master
->building linux 6.4 for checking there right now...
Note: for all tests I created a new calc file as described in the original description
Comment 8 Armin Le Grand 2021-03-02 08:45:06 UTC
->building linux 6.4 for checking there right now...
-> not on linux 6.4
So this error is system-specific, happens on Windows and on master.
I was not aware that we have OLE-handling system-specific stuff, but it seems so. This means to set back some stuff of this task. Reopening, setting to Windows...
Comment 9 Armin Le Grand 2021-03-02 10:47:30 UTC
Very complex stuff, doing debug win/unx parallel. Looks as if SfxInPlaceClient_Impl::SizeHasChanged() is much more often called at win than on linux, e.g. at initial OLE activation. Need to dig deeper - sigh.
Comment 10 Armin Le Grand 2021-03-02 15:38:28 UTC
Cont debugging this
Opening calc file with 3x3 numbers, then insert/OLE as linked file. Size shown seems to depend on file used, now using same file on win/unx (will append)
W: Asks Dialog 'Document in use', offers OpenReadOnly/OpenCopy/Cancel
   Use OpenReadOnly *twice*
   3x3 shown and vertically bigger, *not* selected
L: No dialog, just opening
   same part shown, selected
Activate both (use context menu to do no harm/move with doubleclick)
W: Asks Dialog 'Document in use', one more time use OpenReadOnly
L: Just activates

Linux: Size change works OK, content gets not scaled. Next activation OK. When re-inserting OLE, changed size is remembered (!). When closing SW and reinserting size is remembered (!). When restaring LO size is remembered -> changed size is *saved* in still open calc file ?!?
Checked the DateStamp at the file, it indeed gets *changed* when initially changing size (!)

Win: Size change works OK, same as L. But on deactivate content gets *scaled* as a graphic should be showing the content before size change in the size-changed frame. Reactivation then gets worse, shows relatively scaled content in additionally size-changed frame, e.g. when making smaller initially frame gets again smaller, content scaled even smaller due to trying to still show the same content as before.
Inserting OLE again does not keep size change (see Linux), also not after SW re-open or restart -> size *not saved* in calc file.

This change of the calc file on size-change of it's OLE may already be a reason for the bug: Getting the size next time from OLE on WIN may get the wrong size.
Thus it might be worth checking why it cannot just be activated o Win and to ask if this is wanted - which behavior is intended, Win or Linux?
Comment 11 Armin Le Grand 2021-03-03 09:30:05 UTC
Trying on Win now. When not keeping the calc file open (as in (1) of description) I can see that when activationg and size changing
- no 'Document in Use' pops up
- calc doc *is* changed (date checked) on size change of activated OLE
and all works well. One more hint that this might be the problem.
Trying to find the 'Document in Use' reason on Windows - it's not on Linux and I think that is correct - it is open, but in the same LO-instance...
Another test would be to see the reason, force to 'Document in Use' on Linux and see if it then creates the same error...
Comment 12 Armin Le Grand 2021-03-03 10:03:32 UTC
Seems to be related to bHandleSysLocked in sfx2/source/doc/docfile.cxx:1336. Gets called at insert,
W: 1stcall true, 2nd true, 3rd (activate) true
L: 1stcall false, 2nd false, 3rd (activate) false
forcing to vice-versa for check...
-> get the same errors on Linux
-> get working on Win, but *only* until deactivate. Then bHandleSysLocked *is* false on win, but deactivated frame shows graphic-scaled content -> error
Still this is prove that this is the problem. Now need to find out why on Windows bHandleSysLocked is/gets true and write to shared-opened-locked calc is/seems not possible...
Comment 13 Armin Le Grand 2021-03-03 17:12:02 UTC
Looking at bool bHandleSysLocked which is#
( bLoading && bUseSystemLock && !pImpl->xStream.is() && !pImpl->m_pOutStream )
L: true, true, false, true -> false
W: all true -> true
Diffrerence is in pImpl->xStream.is(), empty on win, set on linux. Lets see where it comes from...
W: sfx2\source\doc\docfile.cxx SfxMedium::GetMedium_Impl():2667
 //TODO/MBA: what happens if property is not there?!
 aMedium[utl::MediaDescriptor::PROP_STREAM()] >>= pImpl->xStream;
 where 'utl::MediaDescriptor::PROP_STREAM()'-> 'Stream'
 is *not* in aMedium (which is a HashMap with 7 entries)
L:
 Problems to look into hashMap, but *must* contain a 'Stream' entry since one gets returned. Comment line of MBA seems prophetic, must have been added at Sun/StarDivision times, lets see...

Line 2667 is from 536a6d6ca67d0 (Noel Grandin         2019-02-06 10:09:28 +0200 2666), probably only modified.
Comment 14 Armin Le Grand 2021-03-03 17:26:13 UTC
On Windows aMedium starts with 4 entries, call to
  aMedium.addInputStreamOwnLock()
adds empty 'Stream' though. Then 'Document in Use' dialog triggers, after that another call where aMedium starts with 5 entries, this time call to
 aMedium[utl::MediaDescriptor::PROP_URL()] <<= aFileName;
 aMedium.erase( utl::MediaDescriptor::PROP_READONLY() );
 aMedium.addInputStream();
since bFromTempFile is true which then creates an existing  'Stream' entry.
But gets not used, further calls happen, 2nd 'Document in Use' and so on.

Thus on win aMedium.addInputStreamOwnLock() goes wrong, should work on linux - checking, probably only called once...
Comment 15 Armin Le Grand 2021-03-04 09:28:29 UTC
aMedium.addInputStreamOwnLock() being called once on linux is wrong, SfxMedium::GetMedium_Impl() gets called pretty often (of course) due to creating and populating the SfxMedium_Impl stuff.
More promising seems SfxMedium::LockOrigFileOnDemand, checking if that is a good point for inverstigations -> ACK, it is. Gets triggered at pressing OK at 'Insert OLE Object' Dialog. Compare-debug from there...
Comment 16 Armin Le Grand 2021-03-04 09:55:16 UTC
Real diff starts at MediaDescriptor::impl_openStreamWithURL at line 665 which is
 xStream = aContent.openWriteableStream();
that works on unx but throws an exception at Win. Win continues opening in read-only mode (sucessful).

In SfxMedium::LockOrigFileOnDemand line 1294 linux is done, but win triggers on line 1300 due to m_xLockingStream.is() being false, trying to get it now. This again triggers calls to MediaDescriptor::impl_openStreamWithURL trying openWriteableStream() which continues to fail.

This then leads in sfx2/source/doc/docfile.cxx to having that difference in bHandleSysLocked mentioned in comment 13.

-> Basic reason is that on Win the same stream describing the calc file *cannot* be opened writable (again) in the OLE container. Not sure but I remember that MBA mentioned something like that years ago ?!?

Need to check this now, but either this is possible on win and needs to be added or not possible and keeping a link to that file needs to be forbidden in the dialog - will see...
Comment 17 Armin Le Grand 2021-03-04 16:33:00 UTC
Did a quick test how far this has to do with 'Link to File' option in Dialog. Without it just a copy is created and all works well (the copy is writable). Thus disabling this (if we can detect that) when the file to use is already opened is an option on Windows.

Win lands in osl_openFile using CreateFileW in sal\osl\w32\file.cxx which is part of the Windows standard API. Flags used are GENERIC_WRITE for dwDesiredAccess and OPEN_EXISTING for dwCreation, and it fails.

There also exists FILE_SHARE_WRITE as part of dwShareMode, but this may require that thjis was used when initially opening that file (create/load calc), but still can try to force to it

force -> multiple calls, only forced when real file was in name (also calls for lockfile & a tempfile). Activation works, size change is OK
force when deactivate -> same pattern used, but got errors and scaled content again. May be possible to handle this, but would need more knowledge.

Thinking about other alternatives...
Comment 18 Armin Le Grand 2021-03-04 16:40:10 UTC
Checking what exactly the linux version of osl_openFile is doing and if shared_write is somehow represented there in a special way...
Comment 19 Armin Le Grand 2021-03-04 18:31:52 UTC
Linux opens with flags == 2 and mode == 438 in sal/osl/unx/file.cxx openFilePath.
flags==2 -> O_RDWR
mode==438 -> 0666 -> DEFFILEMODE(S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH)
S_IRUSR/S_IWUSR->read/write/ByOwner
S_IRGRP/S_IWGRP->read/write/ByGroup
S_IROTH|S_IWOTH->read/write/ByOthers

Cross-reading on net sources shows that on Linux indeed two file handles can have write access to one file - you may need to sync yourself (fcntl/flock/lockf or mutex) to avoid harm. We use fcntl in sal/osl/unx/file.cxx in openFilePath, so as long as the file is open, only the last handle opening it writes (correct?). This means in our case the calc file is opened and can be written from both as a whole. Checking if editing in one changes the other and vice-versa...
Comment 20 Armin Le Grand 2021-03-05 09:28:42 UTC
It may be an option - on windows - to check if the file is already opened (if that is possible, does LO have a global list of opened files?). In that case it may be possible to just 'share' the write access - e.g.- using SfxMedium's std::unique_ptr< SfxMedium_Impl > pImpl as std::shared_ptr instead...?

Another possibility would be to use FILE_SHARE_WRITE more prominent in osl_openFile of sal\osl\w32\file.cxx. This would be a small but deep change, at least this would bring win behaviour closer to linux one - maybe need to check then if something simlar to fcntl exists/needs to be used in openFilePath then.

To check this make current build for win, 'make check' repeat after rough change...
Comment 21 Armin Le Grand 2021-03-05 12:07:53 UTC
Did now https://gerrit.libreoffice.org/c/core/+/111995 to check the 2nd option for Windows from Comment 20, waiting for Windows-build.
Comment 22 Armin Le Grand 2021-03-05 15:25:40 UTC
Gerrit triggered an error in CppunitTest_chart2_pivot_chart_test, investigation shows indeed FILE_SHARE_WRITE is needed for !osl_File_OpenFlag_Write.#
Also checking whole CppunitTest_sc_subsequent_export_test...
Changing, re-checking...
Comment 23 Armin Le Grand 2021-03-05 16:07:17 UTC
Also checking 1st option for Windows from Comment 20. I found no place where SfxMedium_Impl or it's owner, SfxMedium, is somehow globally registerd. Would need to do so, probably for a specialized case (which files can be inserted as OLE?). Could then check in SfxMedium::GetMedium_Impl:2667 if pImpl->xStream is set, or - also possible - LockOrigFileOnDemand:1294 if GetLockingStream_Impl did the right thing. Q is where and which SfxMedium/SfxMedium_Impl to hold...
Comment 24 Armin Le Grand 2021-03-08 16:08:21 UTC
Results from 2nd gerrit buld are in, checking CppunitTest_emfio_emf and CppunitTest_sw_odfexport, try to get some clues what may be influencing these..
Comment 25 Armin Le Grand 2021-03-08 16:21:13 UTC
Maybe it's possible to just handle the current problem case in osl_openFile and hand over another flag. there are currently in include\osl\file.h:
#define osl_File_OpenFlag_Read      0x00000001
#define osl_File_OpenFlag_Write     0x00000002
#define osl_File_OpenFlag_Create    0x00000004
#define osl_File_OpenFlag_NoLock    0x00000008
and additionally in include\osl\detail\file.h:
#define osl_File_OpenFlag_Trunc 0x00000010L
#define osl_File_OpenFlag_NoExcl 0x00000020L
#define osl_File_OpenFlag_Private 0x00000040L
so adding one might be feasible. But first check if the more common 2nd solution for windows on gerrit may work.

No error on my local Win build for CppunitTest_emfio_emf.
No error on my local Win build for CppunitTest_sw_odfexport.
-> rebase and let gerrit check again
Only Windows build would be needed, is there a way to tell gerrit to check only one plattform...?
Comment 26 Armin Le Grand 2021-03-09 12:13:59 UTC
Option (2) of comment 20 does gerrit test sucessfully (at last - all false negative results on Windows build - sigh): https://gerrit.libreoffice.org/c/core/+/111995
So, seriously thjinking about pros/cons of this solution
- Only changes and thus affects Windows version
- Win version closer with behaviour to linux
- If this triggers problems on Win these are not new since Linux does it all the time
- Self-cared sync is required due to Linux requirements anyways, so no new requirements with this change
Thinking about more and evaluating...
Comment 27 Armin Le Grand 2021-03-09 19:05:22 UTC
Real fix now on https://gerrit.libreoffice.org/c/core/+/112237,
Test scenario (with verified Wnidows-build) was https://gerrit.libreoffice.org/c/core/+/111995
Comment 28 Armin Le Grand 2021-03-12 08:41:06 UTC
Discussed with Mike (thanks for hints/suggestions) and checked locking behaviour between LO and WordPad:

WP, LO without patch: opening on WP first -> can open with LO and safe, checked that indeed saving changed file
WP, LO without patch: opening on LO first -> cannot open wth WP
WP, LO with patch: opening on WP first -> can open with LO and safe, checked that indeed saving changed file
WP, LO with patch: opening on LO first -> cannot open wth WP
-> looks good AFAICT

That we *can* open and change/save a file open in WP (already with current master) is strange, too (an error? Is there a BugReport?), but no change caused here.
When opening on WP, change and save on LO keep open, change on WP and try to save at least WP says not possible and does not safe. Also same with/without the patch.
Comment 29 Commit Notification 2021-03-12 09:02:43 UTC
Armin Le Grand (Allotropia) committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/2b4cd99d3360ccffb9829a02412824864d045753

tdf#126742 make Windows file handling more unx-like

It will be available in 7.2.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 30 Commit Notification 2021-03-15 10:43:16 UTC
Armin Le Grand (Allotropia) committed a patch related to this issue.
It has been pushed to "libreoffice-7-1":

https://git.libreoffice.org/core/commit/5f7e94735ecd38faa8beb825f76e0d483fee7101

tdf#126742 make Windows file handling more unx-like

It will be available in 7.1.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.