Bug Hunting Session
Bug 42982 - improve UNO API error reporting
Summary: improve UNO API error reporting
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
Master old -3.6
Hardware: Other All
: medium minor
Assignee: Not Assigned
URL:
Whiteboard: target:5.4.0 target:6.0.0 target:6.2.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicDebug
Depends on:
Blocks: UNO
  Show dependency treegraph
 
Reported: 2011-11-16 03:04 UTC by Michael Stahl (CIB)
Modified: 2018-10-27 21:15 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
Patch to fix namecont.cxx (1012 bytes, patch)
2012-04-11 12:08 UTC, Abeer Sethi
Details
This includes the suggested changes for namecont.cxx (1.02 KB, patch)
2012-04-12 05:39 UTC, Abeer Sethi
Details
A patch for copytablewizard.cxx (2.11 KB, patch)
2012-04-12 05:40 UTC, Abeer Sethi
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Stahl (CIB) 2011-11-16 03:04:28 UTC
There are many implementations of UNO APIs in LibreOffice,
and a lot of them have only very primitive error reporting.

In many cases the only form of error reporting used is
"throw RuntimeException;", which is completely unhelpful
to API users such as extension developers.

Such exceptions should be improved:

1. most importantly, an error message should be included in
   the exception so the API user may get some idea what
   is going wrong.
   for this, the newly introduced printf-style OSL_FORMAT
   macro may be useful.
   
2. in some cases a more specific exception than RuntimeException
   can be thrown.
   but beware that only the exceptions that are listed in the
   exception specification of the UNO API method may be thrown;
   subtypes of RuntimeException are always allowed, but there
   are surprising exceptions (IIRC IllegalArgumentException is
   one) that are not subtypes of RuntimeException.

the offending exceptions can easily be found with "git grep".
Comment 1 Abeer Sethi 2012-03-18 14:13:42 UTC
Can you tell me from my git-diff whether I'm on the right track?


http://pastebin.com/raw.php?i=XAR4s81e
Comment 2 Stephan Bergmann 2012-03-19 03:45:37 UTC
Re comment 1:

1  Lines are cut from the output in <http://pastebin.com/raw.php?i=XAR4s81e>.  (And better attach a patch to this issue than going via pastebin, anyway.)

2  For simple string messages like

  throw RuntimeException(OSL_FORMAT("XListener is not equal to 1"));

there is no need to go via OSL_FORMAT.  The standard idiom to create an rtl::OUString instance from an (ASCII) string literal for now is

  rtl::OUString(RTL_CONSTASCII_USTRINGPARAM("XListener is not equal to 1"))

3  RuntimeException constructors either take no arguments or two arguments (Message, a string; and Context, a com::sun::star::uno::XInterface reference to the relevant UNO object or a null reference).

So, in basic/source/uno/namecont.cxx you would need a second argument

  static_cast< cppu::OWeakObject * >(this)

(where the cast is necessary as this derives from XInterface multiple times), and in the later files you would need to move your new, third argument to be the first one instead, replacing the empty "rtl::OUString()"
Comment 3 Abeer Sethi 2012-04-11 12:08:43 UTC
Created attachment 59811 [details]
Patch to fix namecont.cxx
Comment 4 Stephan Bergmann 2012-04-12 00:45:56 UTC
(In reply to comment #3)
> Created attachment 59811 [details] [review]
> Patch to fix namecont.cxx

See <http://lists.freedesktop.org/archives/libreoffice/2012-April/029894.html> for a comment about that patch.  (In general, it is better to present a patch for review in one place only, either in a bug or on the mailing list.)
Comment 5 Abeer Sethi 2012-04-12 05:39:44 UTC
Created attachment 59853 [details]
This includes the suggested changes for namecont.cxx
Comment 6 Abeer Sethi 2012-04-12 05:40:45 UTC
Created attachment 59855 [details]
A patch for copytablewizard.cxx
Comment 7 Stephan Bergmann 2012-04-23 12:13:21 UTC
(In reply to comment #5)
> Created attachment 59853 [details] [review]
> This includes the suggested changes for namecont.cxx

pushed as <http://cgit.freedesktop.org/libreoffice/core/commit/?id=c9afb3f5a7f713d34f70b680c5d4ab3db4044d1c>
Comment 8 Stephan Bergmann 2012-04-23 12:14:43 UTC
(In reply to comment #6)
> Created attachment 59855 [details] [review]
> A patch for copytablewizard.cxx

pushed as <http://cgit.freedesktop.org/libreoffice/core/commit/?id=67d022ac0ce5e67565e0589f4cd9eb05a8fd5a3c>
Comment 9 Florian Reisinger 2012-05-18 09:33:34 UTC
Deleted "Easyhack" from summary.
Comment 10 Michael Stahl (CIB) 2012-06-29 14:10:36 UTC
Comment on attachment 59855 [details]
A patch for copytablewizard.cxx

i'll mark this as "obsolete" since it was integrated,
so that it doesn't show up in bugzilla searches
Comment 11 Björn Michaelsen 2013-10-04 18:48:12 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 12 Cédric Bosdonnat 2014-01-20 09:00:36 UTC
Restricted my LibreOffice hacking area
Comment 13 Tor Lillqvist 2015-04-14 08:13:15 UTC
Abeer, if you continue doing these patches (please do!), just a minor nit: Please avoid exclamation marks in the exception messages. (Just my personal taste...)
Comment 14 Robinson Tryon (qubit) 2015-12-13 10:58:07 UTC Comment hidden (obsolete)
Comment 15 Robinson Tryon (qubit) 2016-02-18 14:52:39 UTC Comment hidden (obsolete)
Comment 16 Sudarshan K 2016-10-12 17:05:51 UTC
I would like to take a stab at this (I'm assuming this is an EasyHack). Can someone tell me what to do here, Which module to look at and which files to make the changes in ? I'm new. Thanks.
Comment 17 Sudarshan K 2016-10-12 17:10:48 UTC
Can someone please assign this bug to me. Also whom should I contact here or on the IRC channel regarding this bug (#libreoffice-dev) .
Comment 18 jani 2016-10-12 17:23:04 UTC
(In reply to Sudarshan K from comment #17)
> Can someone please assign this bug to me. Also whom should I contact here or
> on the IRC channel regarding this bug (#libreoffice-dev) .

just assign it yourself. I am here to help,


source is in eg basic/uno (see comments)

it is good if you have a little experience with uno.


also please see
https://wiki.documentfoundation.org/Development/GetInvolved
Comment 19 Sudarshan K 2016-10-13 04:51:53 UTC
(In reply to jan iversen from comment #18)
> (In reply to Sudarshan K from comment #17)
> > Can someone please assign this bug to me. Also whom should I contact here or
> > on the IRC channel regarding this bug (#libreoffice-dev) .
> 
> just assign it yourself. I am here to help,
> 
> 
> source is in eg basic/uno (see comments)
> 
> it is good if you have a little experience with uno.
> 
> 
> also please see
> https://wiki.documentfoundation.org/Development/GetInvolved

Hey jan,
I have been looking into /basic/source/uno and here is what my understanding is -
I see a lot of errors being thrown (just a simple RuntimeException(), I did a grep for RuntimeException()) without them being descriptive. So I need to make them throw the error with bit more details, the way Abeer Sethi has done.
Am I correct in this ?
Also, you said a little experience with uno would be required, could you direct me to the relevant docs ( This is my first attempt at contributing to LibOffice, so I have no experience with uno).
Thanks.
Comment 20 jani 2016-10-13 06:42:21 UTC
Change to assigned

You are correct in your assumption, find the RunTimeException and add description, that is basically the task.

Go to our wiki.documentfoundation.org and search for UNO, but as you have already found the right way in the source you will not really need that.

Happy hacking
Comment 21 Sudarshan K 2016-10-13 13:26:30 UTC
jan iversen: This is the link for the patch - https://gerrit.libreoffice.org/#/c/29771/

I forgot to include this in the commit. I will remember that next time.
Comment 22 jani 2016-11-14 10:15:48 UTC Comment hidden (obsolete)
Comment 23 jani 2016-12-21 08:56:28 UTC
Unassing due to lack of work
Comment 24 Saurav S 2017-01-17 16:52:07 UTC
Hello. I'm a new contributor. I'd like to take a stab at this and make every plain RuntimeException in /basic/source/uno/* more descriptive.
Comment 25 Saurav S 2017-01-17 17:02:12 UTC
jan iversen: I've submitted a patch https://gerrit.libreoffice.org/#/c/33227/
Comment 26 jani 2017-01-17 18:37:00 UTC
(In reply to Saurav S from comment #25)
> jan iversen: I've submitted a patch https://gerrit.libreoffice.org/#/c/33227/

Welcome, I see Bergmann have already looked at your patch, when you correct it, please remember to use "git commit --amend" so you update the patch instead of making a new one.

Apart from that, welcome, we have made a step-by-step guide for new people:
https://wiki.documentfoundation.org/Development/GetInvolved

Most importantly is to mail your license statement:
https://wiki.documentfoundation.org/Development/Developers#Example_Code_Contributor_Statement

have fun.
jan I
Comment 27 Saurav S 2017-01-19 17:28:59 UTC
Hello jan, I've submitted a second patch set taking into account Bergmann's comments, and I've sent my license statement to libreoffice@lists.freedesktop.org like the wiki suggested.
Comment 28 Saurav S 2017-01-19 17:40:03 UTC
Oops, I'd left out a couple of changes from patch set 2. I've submitted one with all the updates. Sorry for the inconvenience!
Comment 29 Commit Notification 2017-02-13 20:29:16 UTC
Saurav Sachidanand committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2bcefa7aab176bf18b45e7038e91dd07ced6edf7

tdf#42982 Make UNO error reporting more descriptive

It will be available in 5.4.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 30 Aleksas Pantechovskis 2017-02-28 13:00:26 UTC
(In reply to Michael Stahl from comment #0)
>    subtypes of RuntimeException are always allowed, but there
>    are surprising exceptions (IIRC IllegalArgumentException is
>    one) that are not subtypes of RuntimeException.

http://gimli.documentfoundation.org/docs/idl/ref/exceptioncom_1_1sun_1_1star_1_1uno_1_1RuntimeException.html says that IllegalArgumentException is RuntimeException.
Comment 31 Stephan Bergmann 2017-02-28 14:29:37 UTC
(In reply to Aleksas Pantechovskis from comment #30)
> http://gimli.documentfoundation.org/docs/idl/ref/
> exceptioncom_1_1sun_1_1star_1_1uno_1_1RuntimeException.html says that
> IllegalArgumentException is RuntimeException.

...but only since <https://cgit.freedesktop.org/libreoffice/core/commit/?id=31170413ae3786bf44564e813d7291354e939a77> "API CHANGE: com.sun.star.lang.IllegalArgumentException"
Comment 32 Aleksas Pantechovskis 2017-02-28 17:36:25 UTC
Also looks like OSL_FORMAT was replaced by SAL_STREAM http://nabble.documentfoundation.org/Assertions-and-Logging-td3518719.html but git grep found only 2 usages of it.
As far as I can see currently string concatenation via + operator is usually used ("cannot get service instance \"" + rServiceName + "\"!").
Comment 33 Stephan Bergmann 2017-03-01 07:38:53 UTC
(In reply to Aleksas Pantechovskis from comment #32)
> Also looks like OSL_FORMAT was replaced by SAL_STREAM
> http://nabble.documentfoundation.org/Assertions-and-Logging-td3518719.html
> but git grep found only 2 usages of it.
> As far as I can see currently string concatenation via + operator is usually
> used ("cannot get service instance \"" + rServiceName + "\"!").

Yeah, comment 0 is a bit dated by now.  :)  SAL_STREAM should be rarely necessary, if at all (the two uses in sfx2/source/control/bindings.cxx could arguably be rewritten using string concatenation with + instead), and should never be necessary when generating useful exception messages in the context of this issue.  Always use string concatenation for that (use OUString::number to put numbers into the message).
Comment 34 Commit Notification 2017-03-03 20:51:08 UTC
Alex P committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7e8806cd728bf906e1a8f1d649bef7337f297b1c

tdf#42982 improve error reporting in sc unoobj

It will be available in 5.4.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 35 Shinnok 2017-08-28 12:17:05 UTC
Michael or Stephan, can you briefly verify that the two associated patches merged already completely resolve this issue?

https://gerrit.libreoffice.org/34762
https://gerrit.libreoffice.org/33227
Comment 36 Michael Stahl (CIB) 2017-08-28 12:25:59 UTC
i can briefly confirm that the patches from comment #35 have resolved at most 3% of the issue. 

git grep "throw .*RuntimeException *( *)" | wc -l
1582
Comment 37 Commit Notification 2017-09-07 18:51:17 UTC
Samuel Mehrbrodt committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=4b56d76e83fe062e39f5c00fa405f2a14a13b7ca

tdf#42982 Improve exceptions in text document api

It will be available in 6.0.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 38 Commit Notification 2018-10-27 21:15:04 UTC
Izabela Bakollari committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=0f190f50368816964b2a1b7bb58000ac1792d640

tdf#42982: added description on RuntimeException

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