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 !
I would like to fix this.
However, I noticed that there are situations where we might need to call the destructor of SolarMutexReleaser explicitly. Would that be okay ?
> 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 !
(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.
Ah - so you want a clearable SolarMutexReleaser ? =) do create one if so.
(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 ?
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
(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
(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..
Sorry Pranav. I saw wrong your comment's date.
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.
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.
The sw/ viewsh.cxx thing should be a stock SolarMutexGuard aGuard; I think =) rather than using this un-helpful API =)
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 =)
Here's the change taking care of your comments above. https://gerrit.libreoffice.org/14974
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.
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.
(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.
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.
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.
Is this one complete? If not - should we assign it to someone?
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 ); }
(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.
Wonderful =) thanks Pranav.
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup) [NinjaEdit]