Bug 88230 - cleanup solar mutex yielding ...
Summary: cleanup solar mutex yielding ...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
4.5.0.0.alpha0+ Master
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:4.5.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2015-01-09 10:22 UTC by Michael Meeks
Modified: 2015-12-15 23:58 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 Michael Meeks 2015-01-09 10:22:44 UTC
We often want to drop the recursive solar mutex something like this (from the Vista file-picker):

static void lcl_sleep( ::osl::Condition& aCondition,
                       ::sal_Int32       nMilliSeconds )
{
    sal_uLong nAcquireCount = Application::ReleaseSolarMutex();

... do something slow ...

    Application::AcquireSolarMutex( nAcquireCount );
}

however - this is a bit ugly - we should instead use:

include/vcl/svapp.hxx:class SolarMutexReleaser

which is also exception safe; and just 1x line ;-) so:

{
     SolarMutexReleaser aReleaser; // drop solar mutex

if you do:

git grep -3 AcquireSolarMutex 

you can see a load of places to cleanup here.

Thanks !
Comment 1 Pranav Kant 2015-03-01 07:41:15 UTC
I would like to fix this.
Comment 2 Pranav Kant 2015-03-01 08:20:39 UTC
However, I noticed that there are situations where we might need to call the destructor of SolarMutexReleaser explicitly.

Would that be okay ?
Comment 3 Michael Meeks 2015-03-09 09:38:44 UTC
> However, I noticed that there are situations where we might need to call the
> destructor of SolarMutexReleaser explicitly.

For that you should use:

class VCL_DLLPUBLIC SolarMutexClearableGuard

where you can explicitly release it earlier if necessary by calling clear().

Thanks !
Comment 4 Pranav Kant 2015-03-13 20:36:39 UTC
(In reply to Michael Meeks from comment #3)
> > However, I noticed that there are situations where we might need to call the
> > destructor of SolarMutexReleaser explicitly.
> 
> For that you should use:
> 
> class VCL_DLLPUBLIC SolarMutexClearableGuard
> 
> where you can explicitly release it earlier if necessary by calling clear().
> 
> Thanks !

The problem with using SolarMutexClearableGuard is that as soon as I create an object of it, it will acquire the Mutex, whereas what we need is to release it momentarily and then acquire it.
Comment 5 Michael Meeks 2015-03-13 22:12:59 UTC
Ah - so you want a clearable SolarMutexReleaser ? =) do create one if so.
Comment 6 Pranav Kant 2015-03-14 04:28:16 UTC
(In reply to Michael Meeks from comment #5)
> Ah - so you want a clearable SolarMutexReleaser ? =) do create one if so.

Is it necessary to create a separate class, can't I extend the existing SolarMutexReleaser class to have clear function ?
Comment 7 Gülşah Köse 2015-03-14 15:24:41 UTC
Hi Michael

I am trying to fix this bug. I sent a patch to test if I understand correctly this.

https://gerrit.libreoffice.org/#/c/14869/1/accessibility/source/standard/vclxaccessibleedit.cxx,cm

if you say that way is right way I will continue the correction for other files.

Thanks
Comment 8 Pranav Kant 2015-03-14 18:45:12 UTC
(In reply to Michael Meeks from comment #5)
> Ah - so you want a clearable SolarMutexReleaser ? =) do create one if so.

I extended SolarMutexReleaser class, and added a clear() method to support explicit solar mutex acquire.

Here are the patches (Successfully build on jenkins.)

https://gerrit.libreoffice.org/#/c/14867/2
https://gerrit.libreoffice.org/#/c/14868/2
Comment 9 Pranav Kant 2015-03-14 18:45:59 UTC
(In reply to Gülşah Köse from comment #7)
> Hi Michael
> 
> I am trying to fix this bug. I sent a patch to test if I understand
> correctly this.
> 
> https://gerrit.libreoffice.org/#/c/14869/1/accessibility/source/standard/
> vclxaccessibleedit.cxx,cm
> 
> if you say that way is right way I will continue the correction for other
> files.
> 
> Thanks

Hey, sorry, I was already working on it..
Comment 10 Gülşah Köse 2015-03-14 18:54:16 UTC
Sorry Pranav. I saw wrong your comment's date.
Comment 11 Commit Notification 2015-03-20 12:19:07 UTC
Pranav Kant committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=33de587e95dddfc8e3c89b84f90b3d1bca823dae

tdf#88230: cleanup solar mutex yielding

It will be available in 4.5.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 12 Michael Meeks 2015-03-23 13:59:43 UTC
Lovely cleanup - thanks Pranav. Stephan - I agree that the 'clear' method is not ideal - for sure; then again it hardly looks necessary in the current code-base.

Pranav - I'd love to get this guy closed; but I still see a few bits that could use some cleanup:

fpicker/source/win32/filepicker/SolarMutex.hxx:void AcquireSolarMutex(int nAcquireCount);

That thing seems torally bogus; there is no need for this fpicker wrapper - can we kill that ?

sw/source/core/view/viewsh.cxx-    // Creation, use and destruction of a VirtualDevice needs to be
sw/source/core/view/viewsh.cxx-    // protected by the SolarMutex, it seems.
sw/source/core/view/viewsh.cxx:    Application::AcquireSolarMutex(1);



And after that I'd love to make the 'AcquireSolarMutex' and release method private methods - and add a 'friend' class for the SolarMutexReleaser and other methods that have a legimitate use for that.
Comment 13 Michael Meeks 2015-03-23 14:01:43 UTC
The sw/ viewsh.cxx thing should be a stock SolarMutexGuard aGuard; I think =) rather than using this un-helpful API =)
Comment 14 Michael Meeks 2015-03-23 14:04:22 UTC
Oh; and while I'm here - forget the 'friend' thing - we should just kill the crazy Application:: yielding functions, and move their implementation to non-in-line methods in the Releaser - since they use the global VCL backend state anyway so - no need to associate them with that class =)
Comment 15 Pranav Kant 2015-03-23 23:00:30 UTC
Here's the change taking care of your comments above.

https://gerrit.libreoffice.org/14974
Comment 16 Commit Notification 2015-03-24 06:29:38 UTC
Pranav Kant committed a patch related to this issue.
It has been pushed to "master":

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

tdf#88230: Cleanup solar mutex yielding

It will be available in 4.5.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 17 Commit Notification 2015-03-24 06:29:42 UTC
Pranav Kant committed a patch related to this issue.
It has been pushed to "master":

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

tdf#88230: Remove bogus AcquireSolarMutex fpicker wrapper

It will be available in 4.5.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 18 Pranav Kant 2015-03-24 06:47:28 UTC
(In reply to Commit Notification from comment #16)
> Pranav Kant committed a patch related to this issue.
> It has been pushed to "master":
> 
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=b15b97ee6b21be18d4ba5df396d39b6d3dab57e1
> 
> tdf#88230: Cleanup solar mutex yielding
> 
> It will be available in 4.5.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.

I am sorry, I just realized that the way I have use Releaser here is not the right way to replace the pre-existing code. The previous code acquired the mutex and then released it, but the my commit releases it before and then acquire it again.

This needs to be reverted, if I am correct.
Comment 19 Commit Notification 2015-03-24 07:05:18 UTC
Pranav Kant committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=183f3791bad09cf95c2eee6e7bb043d7b00546a0

Reverts and correct "tdf#88230: Cleanup solar mutex yielding"

It will be available in 4.5.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 20 Commit Notification 2015-03-24 08:41:10 UTC
Stephan Bergmann committed a patch related to this issue.
It has been pushed to "master":

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

tdf#88230 Blind fix for IOS-only code

It will be available in 4.5.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 21 Joel Madero 2015-12-05 07:53:33 UTC
Is this one complete? If not - should we assign it to someone?
Comment 22 Michael Meeks 2015-12-05 12:40:42 UTC
Pranav - I was going to close this; but - can you look and see if this change is necessary to make this more correct ? =)

Thanks !

diff --git a/fpicker/source/win32/filepicker/asyncrequests.cxx b/fpicker/source/win32/filepicker/asyncrequests.cxx
index 089beca..87e53fa 100644
--- a/fpicker/source/win32/filepicker/asyncrequests.cxx
+++ b/fpicker/source/win32/filepicker/asyncrequests.cxx
@@ -28,6 +28,8 @@ namespace vista{
 static void lcl_sleep( ::osl::Condition& aCondition,
                        ::sal_Int32       nMilliSeconds )
 {
+    SolarMutexReleaser aReleaser;
+
     if (nMilliSeconds < 1)
         aCondition.wait(0);
     else
@@ -41,8 +43,6 @@ static void lcl_sleep( ::osl::Condition& aCondition,
 
 void Request::wait( ::sal_Int32 nMilliSeconds )
 {
-    SolarMutexReleaser aReleaser;
-
     lcl_sleep( m_aJoiner, nMilliSeconds );
 }
Comment 23 Pranav Kant 2015-12-07 09:44:35 UTC
(In reply to Michael Meeks from comment #22)
> Pranav - I was going to close this; but - can you look and see if this
> change is necessary to make this more correct ? =)
> 
> Thanks !
> 
> diff --git a/fpicker/source/win32/filepicker/asyncrequests.cxx
> b/fpicker/source/win32/filepicker/asyncrequests.cxx
> index 089beca..87e53fa 100644
> --- a/fpicker/source/win32/filepicker/asyncrequests.cxx
> +++ b/fpicker/source/win32/filepicker/asyncrequests.cxx
> @@ -28,6 +28,8 @@ namespace vista{
>  static void lcl_sleep( ::osl::Condition& aCondition,
>                         ::sal_Int32       nMilliSeconds )
>  {
> +    SolarMutexReleaser aReleaser;
> +
>      if (nMilliSeconds < 1)
>          aCondition.wait(0);
>      else
> @@ -41,8 +43,6 @@ static void lcl_sleep( ::osl::Condition& aCondition,
>  
>  void Request::wait( ::sal_Int32 nMilliSeconds )
>  {
> -    SolarMutexReleaser aReleaser;
> -
>      lcl_sleep( m_aJoiner, nMilliSeconds );
>  }

Looking at commit history around that code, I came across Bug 92460 fixed in c18f11587d37f285a95447dd8996c8b605732e00

Making this change would mean that we are trying to release a SolarMutex on a thread that doesn't ever lock it resulting UI freeze as reproduced by many in Bug 92460.

So, no this change shouldn't go in. Please correct me if I am wrong.
Comment 24 Michael Meeks 2015-12-07 10:23:36 UTC
Wonderful =) thanks Pranav.
Comment 25 Robinson Tryon (qubit) 2015-12-15 23:58:32 UTC
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup)
[NinjaEdit]