Bug 163697 - Inserting new lines in Basic IDE causes crash/triggers assert when assistive technology is active on Windows
Summary: Inserting new lines in Basic IDE causes crash/triggers assert when assistive ...
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
25.2.0.0 alpha0+
Hardware: All Windows (All)
: medium normal
Assignee: Mike Kaganski
URL:
Whiteboard: target:25.2.0
Keywords: accessibility
Depends on:
Blocks: a11y, Accessibility
  Show dependency treegraph
 
Reported: 2024-10-31 07:33 UTC by Michael Weghorn
Modified: 2024-10-31 18:36 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
Backtrace of the assert (3.83 KB, text/plain)
2024-10-31 07:33 UTC, Michael Weghorn
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Weghorn 2024-10-31 07:33:21 UTC
Created attachment 197313 [details]
Backtrace of the assert

Steps to reproduce:

1) start NVDA screen reader
2) start LO Writer
3) "Tools" > "Macros" > "Edit Macros"
4) Press Enter key
5) if nothing happens yet, repeat step 4 and/or keep the Enter key pressed

Result: In a debug build, an assert is triggered, s. attached backtrace.

Another potential way to misbehave seen is the one Mike describes in bug 160982 comment #8:
> (...) with some assistive technology
> active, I see in Basic IDE, that pressing and holding Enter to add multiple
> new paragraphs, the current line with the cursor goes beyond the window
> (previously, the cursor was always at the bottom, and the window expectedly
> scrolled to keep it visible).

As mentioned in bug 160982 comment #9, I think the underlying issue is preexisting, but in some setups (where an AT is active that doesn't set the SPI_GETSCREENREADER flag), this issue is triggered now where it wasn't without the fix for tdf#160982 in place. (It can also be seen in older LO with NVDA, which does set the flag).

I don't see this happening with gtk3 on Linux, also not when Orca is running.

Version: 25.2.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 0e05add86cfb996fe6a11c088c36b3fa010e2445
CPU threads: 12; OS: Windows 10 X86_64 (10.0 build 19045); UI render: default; VCL: win
Locale: en-GB (en_GB); UI: en-US
Calc: threaded
Comment 1 Michael Weghorn 2024-10-31 07:36:26 UTC
(In reply to Michael Weghorn from comment #0)
> Steps to reproduce:
> 
> 1) start NVDA screen reader
> 2) start LO Writer
> 3) "Tools" > "Macros" > "Edit Macros"
> 4) Press Enter key
> 5) if nothing happens yet, repeat step 4 and/or keep the Enter key pressed

This happens when the cursor is at the end of the document, i.e. between step 3) and 4), do

3a) Move cursor to the end of the text window
Comment 2 Mike Kaganski 2024-10-31 07:45:29 UTC
Note that there are two problems causing the crash.

1. In Document::handleParagraphNotifications, when SfxHintId::TextParaInserted is called (i.e., a new para is inserted), the call to m_xParagraphs->insert uses the height obtained from a call to m_rEngine.GetTextHeight for that paragraph. But that call would eventually cause a broadcast of SfxHintId::TextFormatPara - which means, that at this point, handleParagraphNotifications will be re-entered, and the handler (below) will try to access a yet-missing element in m_xParagraphs.

This likely should simply be refactored to (first) insert the new element with 0 height, and (second) set its height to the value obtained from m_rEngine.GetTextHeight.

2. The re-entry to handleParagraphNotifications will also use the iterators 'm_aVisibleBegin/End' and 'm_aFocused'; but they are invalidated by the call to m_xParagraphs->insert, so they need to be reset to valid values *before* calling m_rEngine.GetTextHeight.

In fact, storing iterators in the Document object is bad. It would make sense to drop them, and only store numerical offsets.
Comment 3 Mike Kaganski 2024-10-31 13:51:43 UTC
(In reply to Mike Kaganski from comment #2)
> 2. The re-entry to handleParagraphNotifications will also use the iterators

Should be fixed with https://gerrit.libreoffice.org/c/core/+/175852. Only #1 stays.
Comment 4 Michael Weghorn 2024-10-31 14:27:04 UTC
(In reply to Mike Kaganski from comment #2)
> Note that there are two problems causing the crash.
> 
> 1. In Document::handleParagraphNotifications, when
> SfxHintId::TextParaInserted is called (i.e., a new para is inserted), the
> call to m_xParagraphs->insert uses the height obtained from a call to
> m_rEngine.GetTextHeight for that paragraph. But that call would eventually
> cause a broadcast of SfxHintId::TextFormatPara - which means, that at this
> point, handleParagraphNotifications will be re-entered, and the handler
> (below) will try to access a yet-missing element in m_xParagraphs.
> 
> This likely should simply be refactored to (first) insert the new element
> with 0 height, and (second) set its height to the value obtained from
> m_rEngine.GetTextHeight.

Another alternative might possibly be to call TextEngine::GetTextHeight on demand instead of doing so in Document::handleParagraphNotifications and storing the value.

(In reply to Mike Kaganski from comment #3)
> (In reply to Mike Kaganski from comment #2)
> > 2. The re-entry to handleParagraphNotifications will also use the iterators
> 
> Should be fixed with https://gerrit.libreoffice.org/c/core/+/175852. Only #1
> stays.

Unassigning for now, as I see you also started working on this issue already. Feel free to self-assign :)
Comment 5 Mike Kaganski 2024-10-31 14:50:58 UTC
(In reply to Michael Weghorn from comment #4)
> Another alternative might possibly be to call TextEngine::GetTextHeight on
> demand instead of doing so in Document::handleParagraphNotifications and
> storing the value.

I am not sure what you mean. Especially, I have no idea about the point at which the demand may happen, and if it's OK to rely on that, and if it's OK to have some wrong intermediate height in the paragraph list after the handler finished.

> Unassigning for now, as I see you also started working on this issue
> already. Feel free to self-assign :)

I will fix the crash, but please take a look at it after that - exactly because you know the workflow, and can see what could be improved.

Additionally, the "non-scrolling to cursor" looks a separate issue, unfixed by the crash fix...
Comment 6 Michael Weghorn 2024-10-31 15:03:41 UTC
(In reply to Mike Kaganski from comment #5)
> I am not sure what you mean. Especially, I have no idea about the point at
> which the demand may happen, and if it's OK to rely on that, and if it's OK
> to have some wrong intermediate height in the paragraph list after the
> handler finished.

As `ParagraphInfo` is just a helper class, my idea was to possibly drop ParagraphInfo::m_nHeight, and replace all current uses of ParagraphInfo::getHeight by calling TextEngine::GetTextHeight with the corresponding paragraph number directly.

> I will fix the crash, but please take a look at it after that - exactly
> because you know the workflow, and can see what could be improved.

Thanks! Sure, I'm happy to test the use cases I'm aware of. (Checking that Accerciser highlights correct area for paragraph, and "select object under mouse" feature works.)
Comment 7 Mike Kaganski 2024-10-31 15:57:09 UTC
https://gerrit.libreoffice.org/c/core/+/175879
Comment 8 Mike Kaganski 2024-10-31 18:14:03 UTC
The crash should be fixed now.
Comment 9 Commit Notification 2024-10-31 18:14:27 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/9a07b6c34c12d998b0428a5f70a4833bade3ef1f

tdf#163697: avoid premature handling of following notifications

It will be available in 25.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 10 Michael Weghorn 2024-10-31 18:36:03 UTC
(In reply to Mike Kaganski from comment #8)
> The crash should be fixed now.

Indeed, thanks!

Identifying the object under the mouse generally seems to work fine on Windows as well, as printing information of a paragraph in the macro editor gives the correct information for a previously hovered-over paragraph, e.g.:

    >>> mouse.role
    <Role.PARAGRAPH: 47>
    >>> mouse.IAccessibleTextObject.text(0, -1)
    'hello world'

and the Accerciser cases with gtk3 on Linux still work as expected, as does some further testing there. (Didn't see the recursion problem with gtk3 either, however.)