Bug 134779 - Entering a table with too many columns causing Writer to lock up
Summary: Entering a table with too many columns causing Writer to lock up
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
6.4.4.2 release
Hardware: x86-64 (AMD64) Windows (All)
: medium enhancement
Assignee: An-Kh
URL:
Whiteboard: target:7.1.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicUI
Depends on:
Blocks: Table CPU-AT-100%
  Show dependency treegraph
 
Reported: 2020-07-13 11:51 UTC by abdekker
Modified: 2020-12-14 10:49 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description abdekker 2020-07-13 11:51:36 UTC
Description:
Start a new document. Table > Insert Table. Set Columns to "9999" and click "Insert". Writer locks up (or at least did on my system).

Suggest limiting the number of columns to "99". Tables with 600 columns, for example, can be entered but are totally unreadable.

LibreOffice 6.4.5.2 (x64) on Windows 10

Steps to Reproduce:
1. New document
2. Insert table
3. Set columns to "9999"

Actual Results:
Locks up

Expected Results:
Either warn about the silly number of columns or not lock up.


Reproducible: Always


User Profile Reset: No



Additional Info:
Don't allow the user to enter so many columns. Define some sensible upper limit, say "99".
Comment 1 Telesto 2020-07-13 21:19:59 UTC Comment hidden (obsolete)
Comment 2 csongor 2020-07-15 11:20:21 UTC
I increased the page size from 21x29.7 to 600x29.7 (~30 A4 wide, the max width of a Writer document) so that I see the result. 

I created the table with 2000 columns. After a couple of seconds it has been created. LO worked well, just slightly more slowly than usual. But it was absolutely usable.

So limiting the number of columns to 600 is definitely something I would vote against. 

Creating a 4000x2 table was also quick but the navigation became painfully slow.  

I was able to create a 9999x2 table as well but LO was using a whole CPU core at 100% and it was practically unusable. 

So, it looks to be a performance issue, not a "doesn't work" issue.

I am not sure there is a real use case for this but if it works slowly then it is still better than not working at all. If I create something unusually large then an unusually slow response is acceptable.

On the other hand, I don't understand why it uses any CPU at all (beyond some very basic usage, like <1%). If there is no user action that changes the document then what is it calculating?

This seems to be a design flaw to me which can be very hard to fix. But fixing that design problem would solve many performance issues of LO, not just this edge case. So I think it would be better to figure out why LO is working after the table has been created and fix that issue. 

LO is a very versatile tool. You can use it for many-many things, not just editing five-page-long A4 documents. So limiting its capabilities to the most frequent use cases means we drop features that can be very valuable for some.
Comment 3 Justin L 2020-07-16 15:38:04 UTC
Just a note: .doc format has some limitations after 63 columns
Comment 4 Heiko Tietze 2020-07-17 09:35:40 UTC
We discussed the topic in the design meeting:
+ WPS limits to 63 (Patrick)
+ it's still possible to add rows/columns before/after (Heiko)
+ just a performance issue, limitations are bad (Csongor)
+ show a warning per coloring the spin edit or per confirmation box
=> go with the colored warning

Taking Justin's comment into account we should warn starting with 64 columns (and perhaps 255 rows?) with "Large tables might adverse performance and compatibility". 

Some code pointer:

sw/source/ui/table/instable.cxx
m_xColNF->set_message_type(weld::EntryMessageType::Error/Normal)
Comment 5 abdekker 2020-07-18 11:48:33 UTC
(In reply to Heiko Tietze from comment #4)
> We discussed the topic in the design meeting:
> + WPS limits to 63 (Patrick)
> + it's still possible to add rows/columns before/after (Heiko)
> + just a performance issue, limitations are bad (Csongor)
> + show a warning per coloring the spin edit or per confirmation box
> => go with the colored warning
> 
> Taking Justin's comment into account we should warn starting with 64 columns
> (and perhaps 255 rows?) with "Large tables might adverse performance and
> compatibility". 
> 
> Some code pointer:
> 
> sw/source/ui/table/instable.cxx
> m_xColNF->set_message_type(weld::EntryMessageType::Error/Normal)

Cool, you guys decide what to do, but I do have one comment. Csongor's implication that "limits are bad" is not justified. Of course, you might argue where that limit might be (say 99 or 200 or 1000 or 100 billion), but everything has a limit.

I doubt there is any realistic use case where a user will want more than a few hundred columns in LibreOffice Writer. If they *really* wanted to load a super-large spreadsheet, they'd use Calc not Writer.

In Writer, tables are totally unreadable after a few hundred columns. You'd need truely gargantuan resolution to read a single character with just a few hundred. Make a stand! Maybe set the limit higher (255 or 1000 or whatever), but if anyone complains, "I have a use case where I need twice as many columns" or no-one complains, you've learnt something. Setting no effective limit under the premise "limits are bad" and assuming someone, somewhere uses thousands of columns is "shotting in the dark"! :o)

Warnings are good, but why mask with a warning a feature that will have negative consequences ranging from minor (unreadable table) to serious (application crashes, potentially loses all unsaved work)?

Whatever you decide to do, good luck and thanks!
Alain
Comment 6 Heiko Tietze 2020-07-21 13:18:34 UTC
(In reply to abdekker from comment #5)
> I doubt there is any realistic use case...

Me too. But try with a page size of 6m (the maximum) and 1000 row/cols. You can use this to create a nice pixel graphic :-).
Comment 7 An-Kh 2020-08-27 20:41:58 UTC
(In reply to Heiko Tietze from comment #4)
> We discussed the topic in the design meeting:
> + WPS limits to 63 (Patrick)
> + it's still possible to add rows/columns before/after (Heiko)
> + just a performance issue, limitations are bad (Csongor)
> + show a warning per coloring the spin edit or per confirmation box
> => go with the colored warning
> 
> Taking Justin's comment into account we should warn starting with 64 columns
> (and perhaps 255 rows?) with "Large tables might adverse performance and
> compatibility". 
> 
> Some code pointer:
> 
> sw/source/ui/table/instable.cxx
> m_xColNF->set_message_type(weld::EntryMessageType::Error/Normal)

I have worked on adding the warning if the rows and columns are more than above specified limits.
Comment 8 An-Kh 2020-08-28 16:59:32 UTC
(In reply to Heiko Tietze from comment #4)
> We discussed the topic in the design meeting:
> + WPS limits to 63 (Patrick)
> + it's still possible to add rows/columns before/after (Heiko)
> + just a performance issue, limitations are bad (Csongor)
> + show a warning per coloring the spin edit or per confirmation box
> => go with the colored warning
> 
> Taking Justin's comment into account we should warn starting with 64 columns
> (and perhaps 255 rows?) with "Large tables might adverse performance and
> compatibility". 
> 
> Some code pointer:
> 
> sw/source/ui/table/instable.cxx
> m_xColNF->set_message_type(weld::EntryMessageType::Error/Normal)

I have added the warning message in case the above limits exceed.

Please review my patch : https://gerrit.libreoffice.org/c/core/+/101561
Comment 9 An-Kh 2020-08-28 17:02:41 UTC
(In reply to Heiko Tietze from comment #4)
> We discussed the topic in the design meeting:
> + WPS limits to 63 (Patrick)
> + it's still possible to add rows/columns before/after (Heiko)
> + just a performance issue, limitations are bad (Csongor)
> + show a warning per coloring the spin edit or per confirmation box
> => go with the colored warning
> 
> Taking Justin's comment into account we should warn starting with 64 columns
> (and perhaps 255 rows?) with "Large tables might adverse performance and
> compatibility". 
> 
> Some code pointer:
> 
> sw/source/ui/table/instable.cxx
> m_xColNF->set_message_type(weld::EntryMessageType::Error/Normal)

I have added the warning message in case the above limits exceed.

Please review my patch : https://gerrit.libreoffice.org/c/core/+/101561(In reply to An-Kh from comment #8)
> (In reply to Heiko Tietze from comment #4)
> > We discussed the topic in the design meeting:
> > + WPS limits to 63 (Patrick)
> > + it's still possible to add rows/columns before/after (Heiko)
> > + just a performance issue, limitations are bad (Csongor)
> > + show a warning per coloring the spin edit or per confirmation box
> > => go with the colored warning
> > 
> > Taking Justin's comment into account we should warn starting with 64 columns
> > (and perhaps 255 rows?) with "Large tables might adverse performance and
> > compatibility". 
> > 
> > Some code pointer:
> > 
> > sw/source/ui/table/instable.cxx
> > m_xColNF->set_message_type(weld::EntryMessageType::Error/Normal)
> 
> I have added the warning message in case the above limits exceed.
> 
> Please review my patch : https://gerrit.libreoffice.org/c/core/+/101561

Also, I am working on installation of Glade. As soon as it is done, I will add the warning to UI
Comment 10 Michael Warner 2020-08-28 17:25:55 UTC
(In reply to Heiko Tietze from comment #4)
> We discussed the topic in the design meeting:
> + WPS limits to 63 (Patrick)
> + it's still possible to add rows/columns before/after (Heiko)
> + just a performance issue, limitations are bad (Csongor)
> + show a warning per coloring the spin edit or per confirmation box
> => go with the colored warning
> 
> Taking Justin's comment into account we should warn starting with 64 columns
> (and perhaps 255 rows?) with "Large tables might adverse performance and
> compatibility". 
> 
> Some code pointer:
> 
> sw/source/ui/table/instable.cxx
> m_xColNF->set_message_type(weld::EntryMessageType::Error/Normal)

I humbly suggest that the warning be phrased like one of these:
"Large tables may adversely affect performance and compatibility"
"Large tables may have an adverse impact on performance and compatibility"
"Large tables may affect performance and compatibility"
Comment 11 An-Kh 2020-08-28 22:28:11 UTC
I have incorporated the changes in the patch.
Comment 12 Buovjaga 2020-10-19 05:13:02 UTC
New patch from Anshu: https://gerrit.libreoffice.org/c/core/+/103632
Comment 13 Commit Notification 2020-11-19 19:11:37 UTC
Anshu committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/3f8d9566cdf278d3412207aa15ac5a8c6a3757b4

tdf#134779 Warning generated if the table size exceeds a particular limit

It will be available in 7.1.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 14 Buovjaga 2020-12-14 07:06:29 UTC
Is this considered fixed now? Would be cool to have a clear state urgently because this is an easy hack.
Comment 15 Heiko Tietze 2020-12-14 10:49:32 UTC
The dialog shows a warning for cols>63 or rows>255. So far it's done. But the new label has a yellow background and using the theme colors leads to unreadable white on yellow text for dark themes. The code uses weld::LabelType::Warning so this is a more basic issue, filed in bug 138890.