Download it now!
Bug 126255 - Relative links broken in Calc
Summary: Relative links broken in Calc
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
6.4.0.0.alpha1+
Hardware: All All
: medium normal
Assignee: Eike Rathke
URL:
Whiteboard: target:6.4.0
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-06 18:22 UTC by Jan-Marek Glogowski
Modified: 2019-07-10 13:22 UTC (History)
5 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 Jan-Marek Glogowski 2019-07-06 18:22:34 UTC
Description:
I didn't test other components. The origin of this bug is the failing tinderbox Win-x86_64_42.

In commit 3f7e8ddea89f ("Enable many more unit tests on all archs") I enabled many more unit tests an all archs to verify if any arch specific #ifdefs are still needed and fixed some fallout, Jenkins didn't catch in commit a2c665e4dd73 ("WIN fix some unit tests more generally").

But there is one problem left: Win-x86_64_42 fails with

24952 NEXT    D:/lode/dev/core/sc/qa/unit/subsequent_export-test.cxx(3086) : error : Assertion
24953         Test name: ScExportTest::testRelativePathsODS
24954 NEXT    assertion failed
24955         - Expression: aURL.startsWith("..")
24956         
24957         Failures !!!
24958         Run: 115   Failure total: 1   Failures: 1   Errors: 0

Thorsten thankfully added a SAL_DEBUG on that box, which shows:

> $ grep debug workdir/CppunitTest/sc_subsequent_export_test.test.log 
> debug:4516:2084:
> file:///D:/lode/dev/core/sc/qa/unit/data/ods/xlsx/databar.xlsx

So obviously the test fails. But it's actually broken on all architectures.

Just open the bug document of the unit test: sc/qa/unit/data/ods/fdo79305.ods. It just contains a
link as

<text:a xlink:href="../xlsx/databar.xlsx"
xlink:type="simple">/devel/libo/libo1/sc/qa/unit/data/xlsx/databar.xlsx</text:a>

For me LO fails to open the link, because it resolves the wrong target.

origin: /libreoffice/sc/qa/unit/data/ods/fdo79305.ods
target: /libreoffice/sc/qa/unit/data/ods/xlsx/databar.xlsx
- an ods too much.

The unit test just tests CPPUNIT_ASSERT(aURL.startsWith("..")), which doesn't make any sense. I would expect a correct absolute URL from LO for the relative link inside the document, so it can be forwarded to whatever system "open link" tool is used. Don't know how to test, if the contained link is absolute.

Maybe someone knows where to fix this bug. I really don't want to hide it again using #ifndef _WIN32, but since this all includes those security link settings and stuff, I would appreciate if someone else takes a stab at it.


Steps to Reproduce:
1. Open sc/qa/unit/data/ods/fdo79305.ods
2. Ctrl+Click the link


Actual Results:
Error message with wrong path.

Expected Results:
The linked document is opened.


Reproducible: Always


User Profile Reset: No



Additional Info:
Current master. Fails unit test on Windows. Manual test fails for the kde5 backend, but from the URL I guess it's broken on all archs.
Comment 1 Jan-Marek Glogowski 2019-07-06 18:29:34 UTC
IMHO the correct test would be to test the existence of the target file and somehow check the relative URL of the document. But since the test extracts the href via xpath it should always return "../xlsx/databar.xlsx", no?
Comment 2 Jan-Marek Glogowski 2019-07-07 00:56:34 UTC
Added Eike and Markus, as they fixed bug 79305 and added the unit test.

Workaround patch: https://gerrit.libreoffice.org/#/c/75172/
Comment 3 Commit Notification 2019-07-07 08:03:29 UTC
Jan-Marek Glogowski committed a patch related to this issue.
It has been pushed to "master":

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

tdf#126255 disable broken relative link unit test

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 4 Jan-Marek Glogowski 2019-07-07 08:21:59 UTC
Just removed the Whiteboard tag, as the bug was not fixed by the patch, but the failing unit test assert disabled.
Comment 5 Eike Rathke 2019-07-08 11:40:52 UTC
(In reply to Jan-Marek Glogowski from comment #0)
> <text:a xlink:href="../xlsx/databar.xlsx"
> xlink:type="simple">/devel/libo/libo1/sc/qa/unit/data/xlsx/databar.xlsx</
> text:a>
> 
> For me LO fails to open the link, because it resolves the wrong target.
> 
> origin: /libreoffice/sc/qa/unit/data/ods/fdo79305.ods
> target: /libreoffice/sc/qa/unit/data/ods/xlsx/databar.xlsx
> - an ods too much.
Given the  xlink:href="../xlsx/databar.xlsx"  the target is correct, it's not one ods too much but one ../ too little in the href. The first ../ escapes the ODF package (.ods) so to target a file ../xlsx/databar.xlsx relative to the current file it should be xlink:href="../../xlsx/databar.xlsx" instead.

That's exactly what my follow-up commit https://cgit.freedesktop.org/libreoffice/core/commit/?id=bc3b62e25eb0c3921fa600e80eeb314e45ecaaef to bug 79305 addressed, but the test document wasn't adapted.

I have no adhoc idea why the test apparently failed only on Windows if the target doesn't exist. I'll correct the test document and revert the temporary "fix" and then remove the #if !defined _WIN32 to see if Windows boxes still stumble upon.
Comment 6 Eike Rathke 2019-07-08 11:58:06 UTC
Or, actually the #if !defined _WIN32 was already removed with dfdd2b0126a677960946c5ff2b905e95b978b2d5 ... well, let's see.
Comment 7 Jan-Marek Glogowski 2019-07-08 11:58:27 UTC
(In reply to Eike Rathke from comment #5)
> (In reply to Jan-Marek Glogowski from comment #0)
> > <text:a xlink:href="../xlsx/databar.xlsx"
> > xlink:type="simple">/devel/libo/libo1/sc/qa/unit/data/xlsx/databar.xlsx</
> > text:a>
> > 
> > For me LO fails to open the link, because it resolves the wrong target.
> > 
> > origin: /libreoffice/sc/qa/unit/data/ods/fdo79305.ods
> > target: /libreoffice/sc/qa/unit/data/ods/xlsx/databar.xlsx
> > - an ods too much.
> Given the  xlink:href="../xlsx/databar.xlsx"  the target is correct, it's
> not one ods too much but one ../ too little in the href. The first ../
> escapes the ODF package (.ods) so to target a file ../xlsx/databar.xlsx
> relative to the current file it should be
> xlink:href="../../xlsx/databar.xlsx" instead.

Interesting. So ".." isn't directory-only in this context (I don't know how to express that better). Didn't know that. And maybe my match to file system semantics is completely wrong here.

> I have no adhoc idea why the test apparently failed only on Windows if the
> target doesn't exist. I'll correct the test document and revert the
> temporary "fix" and then remove the #if !defined _WIN32 to see if Windows
> boxes still stumble upon.

For whatever reason the other system paths from the link start with "..".

Thanks for looking into it.
Comment 8 Jan-Marek Glogowski 2019-07-08 12:00:29 UTC
(In reply to Eike Rathke from comment #6)
> Or, actually the #if !defined _WIN32 was already removed with
> dfdd2b0126a677960946c5ff2b905e95b978b2d5 ... well, let's see.

Yup - I didn't see any point in leaving the wrong assert, so I just commented that and the test can at least test loading the document.
Comment 9 Commit Notification 2019-07-08 15:21:04 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

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

Resolves: tdf#126255 correct xlink:href to escape package, tdf#79305 related

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 10 Commit Notification 2019-07-08 15:21:25 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

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

Revert "tdf#126255 disable broken relative link unit test"

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 11 Eike Rathke 2019-07-08 15:36:36 UTC
(In reply to Jan-Marek Glogowski from comment #7)
> Interesting. So ".." isn't directory-only in this context (I don't know how
> to express that better). Didn't know that. And maybe my match to file system
> semantics is completely wrong here.
See 3.7 Usage of IRIs Within Packages
https://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-part3.html#__RefHeading__752821_826425813
Comment 12 Thorsten Behrens (CIB) 2019-07-08 20:39:16 UTC
(In reply to Commit Notification from comment #10)
> Eike Rathke committed a patch related to this issue.
> It has been pushed to "master":
> 
> https://git.libreoffice.org/core/+/
> 1458d43690684c5ce6ed4409bef34a23cfbab7f2%5E%21
> 
> Revert "tdf#126255 disable broken relative link unit test"
> 
> 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.

Hmm, that does not seem to have helped. Win42 now fails again, with the old error:

|D:/lode/dev/core/sc/qa/unit/subsequent_export-test.cxx(3086) : error : Test name: ScExportTest::testRelativePathsODS
assertion failed
- Expression: aURL.startsWith("..")

As Jmux listed with the original description: on that setup, the URL becomes absolute - no idea why.
Comment 13 Jan-Marek Glogowski 2019-07-08 21:42:24 UTC
(In reply to Thorsten Behrens (CIB) from comment #12)
> Hmm, that does not seem to have helped. Win42 now fails again, with the old
> error:
> 
> |D:/lode/dev/core/sc/qa/unit/subsequent_export-test.cxx(3086) : error : Test
> name: ScExportTest::testRelativePathsODS
> assertion failed
> - Expression: aURL.startsWith("..")
> 
> As Jmux listed with the original description: on that setup, the URL becomes
> absolute - no idea why.

This is just one part of the brokenness / mystery. On other boxes the aURL.startsWith("..") succeeds. But this means it can't be an URL, as the aURL is just an OUString, so no magic overloaded startsWith, which could somehow ignore the URL scheme. So in reality it must be a system path.

But for your Windows box it starts with "file:///", which is a real URL. And your box has probably now an aURL of "file:///D:/lode/dev/core/sc/qa/unit/data/xlsx/databar.xlsx", which is at least a correct URL pointing to the correct document.

But my linux box here returns this as aURL: ../../../home/jmg/Development/libreoffice/symbols/sc/qa/unit/data/xlsx/databar.xlsx

That is - from my understanding - neither an URL nor a correct relative path to the xlsx from the CWD of the /home/jmg/Development/libreoffice/symbols/sc/qa/unit/data/ods/fdo79305.ods

Maybe I wasn't clear in the report, but from any kind of interpretation, aURL should either always be a valid URL or a system path. The current state is just crazy.
Comment 14 Eike Rathke 2019-07-09 11:18:37 UTC
You are confusing what the test does, it tests the xlink:href attribute value, not the final resulting URL. It passes on other platforms and the Jenkins Windows box (if that ran the test at all?) but mysteriously fails on this one Windows box.
Comment 15 Jan-Marek Glogowski 2019-07-09 12:59:25 UTC
So debugging it and with mst__ pointing it out on IRC based on my gdb session, at least my Linux path makes sense.

Reminder: this failing test is an export test!

So the document is loaded from 
* /home/jmg/Development/libreoffice/symbols/sc/qa/unit/data/ods/fdo79305.ods

but it is exported to
* file:///tmp/<some tmpdir>/dummyObjectName/content.xml

which now results in the correct relative link of (with erAck patch to the doc):
* ../../../home/jmg/Development/libreoffice/symbols/sc/qa/unit/data/xlsx/databar.xlsx

And reading the RfC 3986 "4.2. Relative Reference" it's also clear that for the relative reference the schema is omitted.

Now that leaves the Windows test case. I guess the TEMP directory for Windows is somewhere at file:///C:/... but the LO build directory is file:///D:/... Maybe that can't be expressed relative.
Comment 16 Eike Rathke 2019-07-09 13:12:01 UTC
Makes sense, in that Windows different drive letters case the href has to be absolute.
Comment 17 Commit Notification 2019-07-09 18:16:44 UTC
Jan-Marek Glogowski committed a patch related to this issue.
It has been pushed to "master":

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

tdf#126255 handle _WIN32 with different drive letters

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 18 Commit Notification 2019-07-09 23:49:27 UTC
Jan-Marek Glogowski committed a patch related to this issue.
It has been pushed to "master":

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

tdf#126255 compare the URL of the exported file

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 19 Jan-Marek Glogowski 2019-07-10 13:22:13 UTC
With the last patch the test seems to pass again. Thanks for all the input, so it could be fixed properly.