[Libreoffice] [REVIEW][3-5] Prevent excessvie references to formula result tokens

[Eike Rathke]

Hi Kohei,

On Monday, 2012-01-30 16:35:46 -0500, Kohei Yoshida wrote:

> I'd like
> 
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=e2b11f4fd79dce4116badb0ecf6477546ca5d0d4
> 
> cherry-picked to the 3-5 branch.  It is probably too late for change
> like this to be in the 3-5-0 branch so I won't even try.

I'd like to ask for an improvement ;)  see below.


> To reproduce the problem, open the following file in Calc
> 
> http://people.freedesktop.org/~kohei/test-formula-fill-crash.ods
> 
> then
> 
> 1. select B1:B65536 (or just hit ctrl-shift-up)
> 2. fill down (or ctrl-D if you use the default key binding).
> 
> That will currently either crash, or do a totally wrong thing.  If it
> doesn't crash, try undo and redo and it will eventually crash.
> 
> The reason is that, during the fill, the formula token instance inside
> ScFormulaResult gets "copied" i.e. it re-uses the existing instance and
> increases its reference counter by one.  The problem is, this counter is
> unsigned 16-bit integer, and as soon as it goes above 65535 it rolls
> back to zero, and eventually the token instance gets deleted
> prematurely.

Argh, yes, right.

> The above change ensures that the formula result is cleared after each
> formula cell instance gets copied.  We don't need to copy the formula
> result during fill because they get re-interpreted once the copying is
> complete.

True. Though the patch fixes this only for the Fill case, the same crash
happens when copying B1 to clipboard, selecting B2:B65536 and paste.
That needs to be handled similar. Probably best would be to revert this
commit and change the ScFormulaResult copy-ctor to create a clone of the
token instead so one doesn't need to bother with it at various places.

> As an aside, although it's not necessary for this fix, on master we
> should probably use unsigned 32-bit integer to store reference counter
> for this just to future-proof ourselves.  16-bit integer seems a bit too
> small for this purpose.

I'd not recommend using 32-bit for the token reference count, that would
significantly increase memory footprint of every formula. Single tokens
weren't meant to be shared across formula cells.

  Eike

-- 
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20120131/f2602bc5/attachment.pgp>