Bug 63398 - Form recordsource events triggered earlier in BASE
Summary: Form recordsource events triggered earlier in BASE
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
4.0.2.2 release
Hardware: All All
: medium major
Assignee: Lionel Elie Mamane
URL:
Whiteboard: target:4.1.0 target:4.0.4
Keywords:
: 63291 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-04-10 20:45 UTC by tim
Modified: 2013-05-30 17:11 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
A test database to demonstrate the problem (13.70 KB, application/vnd.oasis.opendocument.base)
2013-04-10 20:45 UTC, tim
Details

Note You need to log in before you can comment on or make changes to this bug.
Description tim 2013-04-10 20:45:31 UTC
Created attachment 77778 [details]
A test database to demonstrate the problem

In 4.0.2.2, the BASE form "After Record Change" event triggers before a form or record has completed loaded.  This is particularly apparent on a form being used for data entry only.

This can be seen by inspecting the value of bound controls when the event is triggered.  They are still 'unbound'.  

After the event has been processed, if another event is triggered, eg by a Button being pressed, one can inspect the same fields and find they are now bound to the correct database fields. 

Prior to 4 this worked correctly.  I have been using 3.6.5 for some time with no problem.

The attached TestDb.odb demonstrates this on 4.0.2 and 3.6.5.  When form TT is opened it reports on the contents of the listbox, and the status of text field at the "After Record Change" event.  On pressing the button next to the listbox it reports the same information.  On 4.0.2 the listbox is uninitialised and the textbox unbound in the first instance, and then bound when the button is pressed.

In 3.6.5 both events report the same data.

There is a small amount of Basic code in the TestDb.odb.

I used this event quite extensively to set things up on each record as it is brought up.  The change in behaviour has forced me back to 3.6.5.  I tried to overcome it using other event triggers but could not find a complete solution.

I have suspicions that there are other problems with the timing of this and possibly other events, but this is the one that has caused me the most grief so far.  I spent a lot of time suspecting other causes of this problem.  I created the attached self-contained demonstration since my own system uses a MySQL backend which I couldn't easily demonstrate to others.  The results are the same for MySQL and the embedded database in BASE.
Comment 1 Joel Madero 2013-04-11 21:08:07 UTC
@Lionel - you mind trying to confirm this one? I try on these database ones but they are often tricky :)
Comment 2 Robert Großkopf 2013-04-12 14:16:14 UTC
Have tested the database. 
Could be the same bug as https://bugs.freedesktop.org/show_bug.cgi?id=63291 . There is something broken with the events, not only "after record change", also "when loading". There seem to be no fields of the table of the HSQLDB been bounded to the form-fields, when the macro is running.

I change the status to "New", because it is another event, that doesn't work with LO 4.*
Also changed the platform, because the other bug is first reported on a windows-platform.
Comment 3 tim 2013-04-12 14:26:31 UTC
I agree that this is similar to the problem in #63291.  I would not be surprised if it is the same fault, albeit for a different event. I had tried many solutions, and 'Before Loading' was an event I tried to resolve my problem with 'After Change', without success.  I also suspected that there were problems with other events.

It's a bit more than annoying for me.  It makes LO 4 unusable for my purposes.
Comment 4 tim 2013-04-12 14:28:20 UTC
Sorry - I meant 'When Loading', not 'Before Loading'.
Comment 5 tim 2013-04-29 12:34:35 UTC
I'm just wondering whether this fault and #63291 are likely to get fixed over the next few months.  I want to work out my plan for upgrading to LO 4, or not, as the case may be.  The latest Ubuntu ships with LO 4 and I don't want to have to downgrade it if I can help it.
Comment 6 Lionel Elie Mamane 2013-05-04 03:37:58 UTC
Thorsten? This bug originates from this commit:

commit 90eac3e69749a9227c4b6902b1f3cef1e338c6d1
Author: Thorsten Behrens <tbehrens@suse.com>
Date:   Thu Nov 29 21:27:57 2012 +0100

    API CHANGE remove [oneway] method attributes


I don't know what oneway was supposed to achieve, but it had at least one working effect, that is that the com.sun.star.reflection.theTypeDescriptionManager service gave a type description whose isOneway() member returned true.

This in turn is used by svxform::FormScriptListener::impl_allowAsynchonousCall_nothrow in file svx/source/form/fmscriptingenv.cxx to return true. What is the link between oneway and "allow asynchronous call"? You tell me, I don't know.

This brings us to this bug, namely that the event fires "too soon". That's because of this code in svxform::FormScriptListener::firing:

  if ( !impl_allowAsynchronousCall_nothrow( _rEvent.ListenerType.getTypeName(), _rEvent.MethodName ) )
  {
      impl_doFireScriptEvent_nothrow( aGuard, _rEvent, NULL );
         return;
  }

  acquire();
  Application::PostUserEvent( LINK( this, FormScriptListener, OnAsyncScriptEvent ), new ScriptEvent( _rEvent ) );

So, in 3.x, this was taking the "Application::PostUserEvent" route, which means the event triggered after (about 20 frames up in the stacktrace) frm::ODatabaseForm::load is finished, and in particular the controls of the form are fully initialised.

But in 4.0, this takes the "impl_doFireScriptEvent_nothrow" route and the controls are not fully initialised yet.

We went even further down this road in master (4.1 alpha)... Since commit by Stephan, a *different* object is returned by
 ::comphelper::getProcessServiceFactory().getSingleton( "com.sun.star.reflection.theTypeDescriptionManager" )
namely a cppuhelper::TypeManager and not anymore a stoc_tdmgr::ManagerImpl.
Its getByHierarchicalName method returns a "(anonymous namespace)::MethodDescription", whose isOneway() method just harcodes "return false"...


FYI, in this specific case:
 _rListenerType == "com.sun.star.sdbc.XRowSetListener"
 _rMethodName   == "cursorMoved"
Comment 7 Stephan Bergmann 2013-05-06 13:01:14 UTC
The UNO "oneway" call concept is documented at <http://web.archive.org/web/20100711070220/http://udk.openoffice.org/common/man/execution.html> "UNO Execution Model" (<http://udk.openoffice.org/common/man/execution.html> appears to have been lost inadvertently).  It was only ever translated to asynchronous calls in the case of URP bridge implementations that did not override it to use synchronous calls, and this overriding has ubiquitously been done for ages, first explicitly when setting up bridges and then implicitly in the bridge code itself.

That is why it was deemed OK to drop the "oneway" keyword from UNOIDL with <http://cgit.freedesktop.org/libreoffice/core/commit/?id=90eac3e69749a9227c4b6902b1f3cef1e338c6d1> "API CHANGE remove [oneway] method attributes."  (And that in turn is the reason why the new TypeDescriptionManager implementation in cppuhelper/source/typemanager.cxx always returns false for isOneway.)  What has apparently been overlooked by commit 90eac3e69749a9227c4b6902b1f3cef1e338c6d1 is that there were calls to isOneway that required adaptation and/or could be cleaned up.

svxform::FormScriptListener::impl_allowAsynchonousCall_nothrow (svx/source/form/fmscriptingenv.cxx) apparently got added with <http://cgit.freedesktop.org/libreoffice/core/commit/?id=eae0f62d3f3d1c7b8882b3da9fcfc1cdd094c40b> "INTEGRATION: CWS dba205a (1.1.2); FILE ADDED," presumably to fix <https://issues.apache.org/ooo/show_bug.cgi?id=65420> "OO crashes when 'revieve focus' event switches on the Filter Mode."  It is unclear to me though why the decision whether to call listeners directly or to postpone that via Application::PostUserEvent should be based on whether the listener method is marked as "oneway."  "I have an idea, but the fix is risky, and checks need to be made whether existing macros are affected, so this will be for 2.0.5 only" in <https://issues.apache.org/ooo/show_bug.cgi?id=65420#c10> might be a hint that this was done in an attempt to reduce the risk for regressions.
Comment 8 Lionel Elie Mamane 2013-05-06 16:48:13 UTC
(In reply to comment #7)

> It is unclear to me
> though why the decision whether to call listeners directly or to postpone
> that via Application::PostUserEvent should be based on whether the listener
> method is marked as "oneway."

To me, it makes sense. If the listener method says "I can be called asynchronously" (that is, "I am a one-way method"), in particular it means "there can be an arbitrary delay between the moment my caller asks for me and the moment I actually execute; my caller is allowed to continue executing *before* I start/finish executing."

So, svxform::FormScriptListener::firing tests if it is "known safe" to continue executing while the event listener method is "pending", and if it finds that it is, then do it.

It is important that some events are postponed. For example, the "finished loaded" event of a control. Imagine it is bound to a macro that references *another* control in the same form. If the event is not postponed, the other control could not be initialised yet, and the reference wreaks havoc. If it is postponed to until *all* controls are initialised, then it is safer/simpler/... for the script author.

But it is out of the question to postpone *all* events. For example, consider offapi/com/sun/star/form/XApproveActionListener.idl; the calling code *must* wait to see whether the action is vetoed or not! The same for property listeners and their vetos.


Another possible heuristic would be to postpone events whose listener method returns void; but that smells "more unsafe" to me than using oneway...
Comment 9 Stephan Bergmann 2013-05-07 09:48:19 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > It is unclear to me
> > though why the decision whether to call listeners directly or to postpone
> > that via Application::PostUserEvent should be based on whether the listener
> > method is marked as "oneway."
> 
> To me, it makes sense. If the listener method says "I can be called
> asynchronously" (that is, "I am a one-way method"), in particular it means
> "there can be an arbitrary delay between the moment my caller asks for me
> and the moment I actually execute; my caller is allowed to continue
> executing *before* I start/finish executing."

But that does not match the semantics of UNO "oneway" (where, rather, the emphasis is on the fact that the oneway method may run in parallel to the caller proceeding, not on a potential delay until the oneway method executes---the intent always was that the delay until the oneway method finishes is bounded by the next non-oneway method call made by the caller, even if the design was flawed; given the UNO "oneway" semantics, it would look more natural if FormScriptListener::firing would rather chose to not postpone listener invocation for the oneway listener case).

To me, it looks like the heuristic of impl_allowAsynchronousCall_nothrow is just as bad as any other heuristic based on UNOIDL type information about the listener, but happened to work well for FormScriptListener::firing.
Comment 10 Lionel Elie Mamane 2013-05-08 09:38:32 UTC
*** Bug 63291 has been marked as a duplicate of this bug. ***
Comment 11 Michael Meeks 2013-05-08 12:29:55 UTC
As a workaround, can we not retain the original semantics by hard-coding and categorising events that used to be 'oneway' and calling them in-line exactly as we used to. IIRC 'oneway' was never asynchronous in-process anyway - just a straight synchronous C++ call (surely); so ... surely we just need to come up with a pseudo-oneway annotation in raw-code for those few methods in that module ?

Of course, perhaps I'm crack-smoking ;-) failing that we can simulate oneways (if this is some remote connection) by queueing until idle/dispatching in a thread - but I suspect that's horrible overkill and that the above will work [ but I'm an optimist ].
Comment 12 Lionel Elie Mamane 2013-05-08 15:27:02 UTC
(In reply to comment #11)

> As a workaround, can we not retain the original semantics by hard-coding and
> categorising events that used to be 'oneway'

Yes, if "someone" writes a function "wasOneWay", then sure, I'll adapt the code to call this new "wasOneWay" instead of the typedescriptor's "isOneway()" and at least we don't regress. As you write, we have to "hardcode" it.

The question is how to make that "wasOneWay". I imagine we'd have to write a big function like:

 bool wasOneWay()
 {
   if (event == "com.sun.star.foo" ||
       event == "com.sun.star.bar" ||
       event == "com.sun.star.qux" ||
       ... )
       return true;
   else
       return false;
 }

What is your suggestion? manually look at the diff of commit 90eac3e69749a9227c4b6902b1f3cef1e338c6d1? Hmm... maybe with some postprocessing of the diffstat... we get the list of changed files, strip .idl at end and "offapi" at the beginning, change "/" to "."... Now, we just have to extract which of these files are event listeners. Any idea for that? Or just put them all?

Yeah, this could work.


> so ... surely we just need to come up with a pseudo-oneway annotation in raw-code for those
> few methods in that module ?

If we could annotate the *event* with a boolean that says:
 - false -> caller needs to treat this event immediately and wait for its result
 - true  -> caller can fire the event and forget about it, or save the event for future batching with other events
rather than maintaining a separate list, then that "smells" better. But I don't have an idea for annotating the event without re-introducing a new flag in IDL (which we would initially give the same value as oneway had).
Comment 13 Commit Notification 2013-05-12 03:16:47 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9dfc3807dbff1a40f487d020446265bb85d0ac16

fdo#63398 hardcode former list of oneway method



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 14 tim 2013-05-12 07:42:37 UTC
Having raised the issue I'm willing to try this, but I need to know how to find out whether the fix is in a particular daily build or not.  I've so far failed to find the list.

Further, in daily builds (http://dev-builds.libreoffice.org/daily/libreoffice-4-0/) I can see Win and Mac versions, but not Linux .deb versions.  What am I missing?
Comment 15 Lionel Elie Mamane 2013-05-12 09:08:05 UTC
(In reply to comment #14)
> Having raised the issue I'm willing to try this, but I need to know how to
> find out whether the fix is in a particular daily build or not.  I've so far
> failed to find the list.

It is not yet in the LibreOffice 4.0 branch, only in master (4.1.0.alpha). You see that in the Whiteboard, which says "target:4.1.0". When it will be committed, the whiteboard will be updated with "target:4.0.4" and a comment similar to comment 13 will be posted to this bug.

I submitted it for approval to LibreOffice 4.0 branch (we need approval by a second developer for release/bugfix branch), see https://gerrit.libreoffice.org/3863

> Further, in daily builds
> (http://dev-builds.libreoffice.org/daily/libreoffice-4-0/)
> I can see Win and Mac versions, but not Linux .deb versions.
> What am I missing?

These two have *both* rpm and deb packages (the .debs are packed in a .tar.gz):

http://dev-builds.libreoffice.org/daily/libreoffice-4-0/Linux-x86_64@31-Release-Configuration-RHEL5-Baseline/current/
http://dev-builds.libreoffice.org/daily/libreoffice-4-0/Linux-x86@34-Release-Configuration-RHEL5-Baseline/current/

For the time being, you'll have to do the testing on a master daily. It would e very much appreciated if you could test it before we put in our release branch. Here's how to see if a particular commit/fix is in a daily build: Let's take for example http://dev-builds.libreoffice.org/daily/master/Linux-x86-64@8-SLED11/current/

The easiest is just to look at the date. Comment 13 says that the fix was committed to master on 2013-05-12 03:16:47 UTC, but right now the files there are of 03-May-2013, so no chance. Have to wait for a new build.

If the time is close and you are unsure, the file "*_build_info.txt" contains in its first lines:

tinderbox: git sha1s
core:485b1ac04c567d9d11cd395cf0a62cc43cabd8ad

The "core" line is the one you need.

On http://cgit.freedesktop.org/libreoffice/core/commit/?id=485b1ac04c567d9d11cd395cf0a62cc43cabd8ad, you see the last commit that is included in this build, and you can look in "log" whether it is before or after the commit that fixes / is supposed to fix the bug.
Comment 16 tim 2013-05-12 14:29:13 UTC
OK Thanks.

I will do so (or at least I will attempt to do so - I'm no expert at these matters).  I'll check in once a day or so to see if there's a new version.

When I said I couldn't find a Linux version what I should have said was that I couldn't find a recent version - the dates seemed relatively old - whereas MAC and Win seemed recent, which rather puzzled me.

Initially I'll try the test file attached to my initial report. If that's OK I'll try it out on my applications using mysql databases.  I have an instance or two of the load event as well (#63291).
Comment 17 tim 2013-05-12 16:52:11 UTC
Looking at the master directories, there appear to be no deb versions (I'm using Mint Debian and Ubuntu) only rpm, or am I missing something else?  

I see both rpm and deb types in the 4.0.4 directories, but as I understand it you would like me to try the master (4.1 alpha) version first, when rebuilt.

Should I try to use a converter like Alien? I've never attempted this.
Comment 18 Commit Notification 2013-05-14 08:33:09 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "libreoffice-4-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=73a2e8da11f3785428b97c2bf3fbb52b450fd9f9&h=libreoffice-4-0

fdo#63398 hardcode former list of oneway method


It will be available in LibreOffice 4.0.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 19 tim 2013-05-15 07:03:41 UTC
If there was a linux build I'd test it now.  However, it seems Windows is the only regular build.  I'm somewhat reluctant to try compiling it myself, since I really wouldn't know what I was doing in any detail.
Comment 20 tim 2013-05-27 19:31:08 UTC
Having been away for a few days I now find there is a linux build of 4.1 beta (I never did find a 4.0.4 version).

I have re-tested the test file I submitted as evidence, and indeed the bug seems to have been fixed.  I wanted to confirm this by testing my main database system against it that uses a variety of events, including the after change and load events.  However, I've encountered another problem that is blocking me (#65045), since many of my actions use dialogues (to set up sql filters and so on), so I cannot double check that this one is fixed on a fully functional database system.
Comment 21 tim 2013-05-30 17:11:07 UTC
I am posting here because I'm trying to retest this particular issue on my main database rather than a test rig.  I was diverted by another problem (#65045) which has now been fixed in 4.1 beta for linux 32 bit (although strangely there is no equivalent 4.1 beta for linux 64 bit - its a 4.0.4 release, so I haven't tested that).

My Base now crashes once I exit a dialogue and expect to see the next Base Form appear - the form shows up and then Base just disappears.  I have no idea how to diagnose this.  I can't raise a new fault or produce a test file to lodge on bugzilla since I don't know what is happening or whether there is a log somewhere I can look.

I am using the exact same database that I run on 3.6.5 on a 64 bit system and 3.5.4 on a 32 bit system.