Null pointer crash in Editor::Paint()

[Jeff Pohlmeyer]

I ran into a rather obscure crash in an application using FXScintilla.

It happens when I try to replace the entire content of a document with
some text that has the same number of lines as the existing text, but
it only crashes when the client rectangle is exactly one line shorter
than the overall height of the document.

The crash occurs in Editor.cxx on lines 3367/3368:

lineWidthMaxSeen = Platform::Maximum(
lineWidthMaxSeen, ll->positions[ll->numCharsInLine]);

because the ll pointer is NULL.

I'm guessing it could possibly be an error in the way FXScintilla
calculates the client rectangle, but I noticed just a few lines
above the crash, beginning on line 3303 of Editor.cxx there is a
block of code that is wrapped in:

if (ll) {
...
}

And I'm curious, why is that block protected against a null-pointer,
but the code on line 3367 is not?

If I change 3367 to this:

if (ll)
lineWidthMaxSeen = Platform::Maximum(
lineWidthMaxSeen, ll->positions[ll->numCharsInLine]);

it seems to solve the problem.

- Jeff

[Neil Hodgson]

Jeff Pohlmeyer:

> The crash occurs in Editor.cxx on lines 3367/3368:
>
>  lineWidthMaxSeen = Platform::Maximum(
>     lineWidthMaxSeen, ll->positions[ll->numCharsInLine]);
>
> because the ll pointer is NULL.

There have been issues with complex reentrance with the line layout
cache where a line had been placed into the cache, a notification
sent, a call into Scintilla made in the notification handler causing
eviction of that line from the cache which the code after the
notification then tried to use.

> I'm guessing it could possibly be an error in the way FXScintilla
> calculates the client rectangle, but I noticed just a few lines
> above the crash, beginning on line 3303 of Editor.cxx there is a
> block of code that is wrapped in:
>
>  if (ll) {
>  ...
>  }
>
> And I'm curious, why is that block protected against a null-pointer,
> but the code on line 3367 is not?

Because they were written at different times and the initial "if
(ll) {" should actually never trigger since that would stop the screen
from being painted. It would be better to work out why there is a null
layout and fix that but its OK to add some protection to
lineWidthMaxSeen since that is really a quite minor feature.

Neil

[Jeff Pohlmeyer]

On Sep 16, 2009 Neil Hodgson wrote:

> There have been issues with complex reentrance with the line layout
> cache where a line had been placed into the cache, a notification
> sent, a call into Scintilla made in the notification handler causing
> eviction of that line from the cache which the code after the
> notification then tried to use.

Now that sounds even stranger than I thought. But at any rate, it did
give me a clue, and after some experimenting I discovered that adding
a call to Fox's repaint() method immediately after replacing the text
makes the problem disappear. (or at least sends it into hiding, where
it can bite me again later).

> its OK to add some protection to lineWidthMaxSeen since that is
> really a quite minor feature.

Will you consider adding this for all platforms, or should I patch
the sources especially for Fox?

Thanks,
- Jeff

[Neil Hodgson]

Jeff Pohlmeyer:

> Will you consider adding this for all platforms, or should I patch
> the sources especially for Fox?

Moved the line into the if (ll) block.

Neil

[Jeff Pohlmeyer]

On Sep 20, 2009, Neil Hodgson wrote:

>   Moved the line into the if (ll) block.

Thanks!

- Jeff