CollaboraOnline / online

Collabora Online is a collaborative online office suite based on LibreOffice technology. This is also the source for the Collabora Office apps for iOS and Android.
https://collaboraonline.com
Other
1.79k stars 682 forks source link

iOS and Online in Safari: LibreOffice crashes when closing the Spellcheck dialog #6010

Open plubius opened 1 year ago

plubius commented 1 year ago

In either the iOS app or in Safari connected to an Online server, closing the Spellcheck dialog while the "Spellcheck is complete" child dialog is displayed will hit an assert and crash in the LibreOffice code.

To reproduce, open a new Writer document, type in a misspelled word (I used "thier's" in an English document) and select Review > Spelling in the iOS app or Tools > Spelling in the Online webpage. In the dialog that appears, select one of the suggestions and press the "Correct" button.

A "Spellcheck is complete" dialog will appear on top of the Spellcheck dialog. Press the "X" or "Close" button in the Spellcheck dialog without closing the "Spellcheck is complete" dialog. Then, close the "Spellcheck is complete" dialog and the iOS app and the Online server will crash.

plubius commented 1 year ago

The partial stack trace from the iOS app below indicates it is in the LIbreOffice code. It appears to me that closing a dialog also deletes (but not closes) its child dialogs. My guess is that taps and clicks in the parent dialog should be ignored if a modal child dialog is displayed:

0 0x0000000108f5253c in SwSpellDialogChildWindow::LockFocusNotification(bool) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/sw/source/uibase/dialog/SwSpellDialogChildWindow.cxx:824

https://github.com/CollaboraOnline/online/pull/1 0x0000000108f51648 in SwSpellDialogChildWindow::GetNextWrongSentence(bool) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/sw/source/uibase/dialog/SwSpellDialogChildWindow.cxx:415 https://github.com/CollaboraOnline/online/pull/2 0x000000010160ec04 in svx::SpellDialog::GetNextSentence_Impl(std::1::unique_ptr<UndoChangeGroupGuard, std::1::default_delete >, bool, bool) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/cui/source/dialogs/SpellDialog.cxx:1003 https://github.com/CollaboraOnline/online/pull/3 0x000000010160dc6c in svx::SpellDialog::SpellContinue_Impl(std::1::unique_ptr<UndoChangeGroupGuard, std::1::default_delete >, bool, bool) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/cui/source/dialogs/SpellDialog.cxx:363 https://github.com/CollaboraOnline/online/pull/4 0x000000010161022c in svx::SpellDialog::ChangeHdl(weld::Button&) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/cui/source/dialogs/SpellDialog.cxx:553 https://github.com/CollaboraOnline/online/pull/5 0x000000010160c244 in svx::SpellDialog::LinkStubChangeHdl(void, weld::Button&) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/cui/source/dialogs/SpellDialog.cxx:542 https://github.com/CollaboraOnline/online/pull/6 0x000000010a9982e0 in Link<weld::Button&, void>::Call(weld::Button&) const at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/include/tools/link.hxx:111 https://github.com/CollaboraOnline/online/pull/7 0x000000010ac480b8 in weld::Button::signal_clicked() at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/include/vcl/weld.hxx:1431 https://github.com/CollaboraOnline/online/pull/8 0x000000010a8c7940 in SalInstanceButton::ClickHdl(Button) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/vcl/source/app/salvtables.cxx:2701 https://github.com/CollaboraOnline/online/pull/9 0x000000010a8c6df0 in SalInstanceButton::LinkStubClickHdl(void, Button) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/vcl/source/app/salvtables.cxx:2689 https://github.com/CollaboraOnline/online/pull/10 0x000000010a312714 in Link<Button, void>::Call(Button) const at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/include/tools/link.hxx:111 https://github.com/CollaboraOnline/online/pull/11 0x000000010a3126c8 in Button::Click()::$_0::operator()() const at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/vcl/source/control/button.cxx:130 https://github.com/CollaboraOnline/online/pull/12 0x000000010a312684 in decltype(static_castButton::Click()::$_0&(fp)()) std::1::invokeButton::Click()::$_0&(Button::Click()::$_0&) at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.2.sdk/usr/include/c++/v1/type_traits:3918 https://github.com/CollaboraOnline/online/pull/13 0x000000010a31263c in void std::1::invoke_void_return_wrapper<void, true>::callButton::Click()::$_0&(Button::Click()::$_0&) at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.2.sdk/usr/include/c++/v1/functional/invoke.h:61 https://github.com/CollaboraOnline/online/pull/14 0x000000010a312614 in std::1::function::alloc_func<Button::Click()::$_0, std::1::allocatorButton::Click()::$_0, void ()>::operator()() at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.2.sdk/usr/include/c++/v1/functional/function.h:178 https://github.com/CollaboraOnline/online/pull/15 0x000000010a310f60 in std::1::function::func<Button::Click()::$_0, std::1::allocatorButton::Click()::$_0, void ()>::operator()() at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.2.sdk/usr/include/c++/v1/functional/function.h:352 https://github.com/CollaboraOnline/online/pull/16 0x00000001006e63a0 in std::1::function::value_func<void ()>::operator()() const at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.2.sdk/usr/include/c++/v1/functional/function.h:505 https://github.com/CollaboraOnline/online/pull/17 0x00000001006cdb14 in std::1::function<void ()>::operator()() const at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.2.sdk/usr/include/c++/v1/functional/function.h:1182 https://github.com/CollaboraOnline/online/pull/18 0x000000010a3391e8 in Control::ImplCallEventListenersAndHandler(VclEventId, std::__1::function<void ()> const&) at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/vcl/source/control/ctrl.cxx:313 https://github.com/CollaboraOnline/online/pull/19 0x000000010a2f8e54 in Button::Click() at /Volumes/LOBuilds/lode/dev/ios-co-22.05.10-6/vcl/source/control/button.cxx:130

plubius commented 1 year ago

I have submitted a patch to LibreOffice that stops the crash:

https://gerrit.libreoffice.org/c/core/+/149970

Note: the patch does not prevent you from closing a dialog while a modal child dialog is running. It only aborts closing the dialog in cases where it will lead to this crash.

plubius commented 1 year ago

Despite trying several different approaches, I have found no way to safely prevent leaking memory.

Many of the modal dialogs are highly dependent on each other and closing the non-topmost modal dialog corrupts not only the LibreOffice internal modal dialog execution stack, but also confuses the Collabora Online JavaScript code as that code does not recognize modality at all.

Unassigning myself as I think the only real solution is for the JavaScript code to implement modality like a desktop. That is, when a modal JavaScript dialog is displayed (whether the dialog is tunneled or not), it blocks all events to parent windows and dialogs

plubius commented 1 year ago

Reassigning to myself as I finally found a way to prevent leaking of dialogs. Interestingly, when Dialog::EndDialog() is called and it deletes itself, the deletion is actually done in a timer for many dialogs. So I added a timer to delete the closed non-topmost dialogs after the topmost dialog has finally been closed and deleted.

plubius commented 1 year ago

Unassigning myself as the leak fixes proved illusory. Closing a document too quickly subverted my leak fixes and causes crashing in debug builds.

plubius commented 1 year ago

I did some debugging and have a possible different approach. It appears to me that all dialog events funnel through jsdialog::ExecuteAction() which is in LibreOffice's vcl code so possibly we could check if the event's dialog has a child dialog in the SVData executing dialog list. If yes, ignore the event. Don't know if that will cause havoc for the JavaScript code though:

Screen Shot 2023-04-10 at 7 31 01 PM
Ezinnem commented 2 weeks ago

I tested using: COOLWSD version: 24.04.6.3snapshot (git hash: e06f245) LOKit version: Collabora Office 24.04.6.20240827 (git hash: c7dc432)

It appears the issue no longer occurs in recent builds. @plubius please can you update your version and test again?

plubius commented 2 weeks ago

This bug still occurs in my local build that I just updated today (LibreOffice distro/collabora/co-24.04 branch and Online master branch).

The bug does not occur if you click the close button in the parent modal dialog's title bar. But if you click on the "Close" button in the lower right of the parent dialog while a child modal dialog is displayed, the parent modal dialog will close but the child modal dialog will still be displayed.

Then, when you press any button in the child modal dialog, the iOS app will crash.