NPE perhaps in !styles[i].equals(startStyle), used to be styles[i] != startStyle, before the 256 color patch

[Martin Dorey]

2009-10-15T22:04:47.594-0700 Terminator: Exception occurred during event dispatching.

Associated exception:

java.lang.NullPointerException

            at terminator.model.TextLine.getStyledTextSegments(TextLine.java:66)

            at terminator.view.TerminalView.getLineStyledText(TerminalView.java:761)

            at terminator.view.TerminalView.paintComponent(TerminalView.java:737)

            at javax.swing.JComponent.paint(JComponent.java:1027)

            at javax.swing.JComponent.paintToOffscreen(JComponent.java:5122)

            at javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:277)

            at javax.swing.RepaintManager.paint(RepaintManager.java:1217)

            at javax.swing.JComponent._paintImmediately(JComponent.java:5070)

            at javax.swing.JComponent.paintImmediately(JComponent.java:4880)

            at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:803)

            at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:714)

            at javax.swing.RepaintManager.seqPaintDirtyRegions(RepaintManager.java:694)

            at javax.swing.SystemEventQueueUtilities$ComponentWorkRequest.run(SystemEventQueueUtilities.java:128)

            at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)

            at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)

            at e.debug.EventDispatchThreadHangMonitor.dispatchEvent(EventDispatchThreadHangMonitor.java:191)

            at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)

            at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)

            at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)

            at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)

            at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)

            at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)

 

[Elliott Hughes]

i assume from the lack of steps to reproduce you can't reproduce it.

weird.

i've had a look, and i can't see any obvious way we'd get nulls in that
array. (and i can't think of any other way we'd see that NPE than if there
was a null in the array.)

a grep of my logs says i haven't seen this myself. (though presumably i'd
have noticed if a window stopped being able to redraw, without needing to
consult my logs!)

--elliott

--
Elliott Hughes, http://www.jessies.org/~enh/

[Martin Dorey]

Looking at my logs, I saw it in two instances of Terminator. I was catting full, partial and hacked logs of the screen problem, though I hadn't twigged that stty -onlcr was needed. I tried to reproduce it at the time but didn't succeed. I've tried again now but still no dice.

[Martin Dorey]

If we were trying to overwrite some style data for a line which was previously default-styled with styles == null and the region we wanted to overwrite didn't reach the end of the line, wouldn't we need to fill the new array to the end, not just to the start of the region to be overwritten?  The code which does the filling in the insertStyleData method looks like the patch I'm suggesting here - somewhat clumsily filling elements that it will immediately overwrite.

 

Index: src/terminator/model/TextLine.java

===================================================================

--- src/terminator/model/TextLine.java    (revision 1582)

+++ src/terminator/model/TextLine.java    (working copy)

@@ -210,7 +210,7 @@

            if (oldStyleData != null) {

                  System.arraycopy(oldStyleData, 0, styles, 0, oldStyleData.length);

            } else {

-                 Arrays.fill(styles, 0, offset, Style.getDefaultStyle());

+                 Arrays.fill(styles, 0, styles.length, Style.getDefaultStyle());

            }

            Arrays.fill(styles, offset, offset + count, value);

[Martin Dorey]

Before r1257 overwriteStyleData was called extendWithStyleData, which it was fit for. It was renamed, to what it perhaps should have done, but didn't need to and didn't do. Then r1259 assumed that it did what it now said on the tin.

---

From: terminat...@googlegroups.com <terminat...@googlegroups.com>
To: terminat...@googlegroups.com <terminat...@googlegroups.com>

Sent: Sat Oct 17 18:35:20 2009

[Elliott Hughes]

i think you're right, both about the fix and how i broke this, patch by
patch over the years.

we could make maybeResizeStyleData fill the whole array if oldStyleData ==
null (i.e. if it's just created a new array from scratch, default to the
default style rather than null, which is an error), if you think that
might be clearer/safer.

it took me a while to work out why removeStyleData doesn't need the "if
(oldStyleData != null)" that the others do. even though it would be
useless from the machine's point of view, adding the test might make that
code more obviously correct.

thanks for finding that!

--elliott

[Martin Dorey]

> if you think that might be clearer/safer

Yeah, I think so, though the chances of the horse bolting through the
same door again seem small.

> adding the test might make that code more obviously correct.

Duplication makes me uneasy. I worried that someone might remove the
earlier return if we did that, violating the precondition on
maybeResizeStyleData. Then again, if the earlier return became more
complicated, we might need the condition. So I settled for a
code-comment. I'm not attached to the words - I expect a better form
wouldn't be hard.

[Elliott Hughes]

the comment looks fine to me.

--elliott