Skip to content

Fix crashes in pan/zoom state, sampled series, listeners, markers and origin modes - #145

Merged
halfhp merged 10 commits into
masterfrom
fix-crashes
Sep 7, 2026
Merged

Fix crashes in pan/zoom state, sampled series, listeners, markers and origin modes#145
halfhp merged 10 commits into
masterfrom
fix-crashes

Conversation

@halfhp

@halfhp halfhp commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Eight crash / data-loss fixes, each with a regression test that was confirmed to fail on the unfixed code and pass after the fix. One commit per fix; release notes updated under # 1.6.0.

1. PanZoom.State restore crashes the render thread

Cause. State's fields default to null and getState() returned that never-populated object until the first gesture. Restoring it via setState() called plot.setDomainBoundaries(null, null, null), storing null boundary modes into XYConstraints; XYPlot.calculateMinMaxVals() then hit switch (mode) on the render thread. Trigger: TouchZoomExampleActivity saves getState() in onSaveInstanceState and restores it, so rotating without touching the plot crashed. A partial config (Pan.HORIZONTAL + Zoom.NONE) left the range modes null even after a gesture.

Change. getState() now snapshots the plot's actual user boundaries and boundary modes for each edge of both axes; State.apply() sets each edge independently and skips any edge whose mode is null, so a stale or legacy state cannot poison the plot. State gains 4-arg setDomainBoundaries / setRangeBoundaries overloads (lower mode / upper mode) mirroring XYPlot; the 3-arg ones delegate. XYPlot gains public getDomain/RangeLower/UpperBoundaryMode() and protected getUserMinX/MaxX/MinY/MaxY(). Serialization compatibility. State's private field layout changed, which would have changed its JVM-computed serialVersionUID and made a State serialized by an older library version (eg. held in an Activity's saved instance state across an app update) fail with InvalidClassException. The UID is therefore pinned to the value serialver computes for the class on master (-4221152827129598653L, with a comment that it must never change), so old streams still load; their legacy single-mode field is dropped, the per-edge modes deserialize as null, and apply() skips them, so restoring an old state is a no-op rather than a crash. Covered by state_serialVersionUID_matchesOriginalClass, state_javaSerialization_roundTrip_preservesBoundaries (ObjectOutputStream/ObjectInputStream round-trip of a mixed-mode state) and state_serializedByOlderVersion_deserializesAndAppliesAsNoOp (deserializes a stream captured from the master build of the class). Without the pin the latter two fail with InvalidClassException: ... stream classdesc serialVersionUID = -4221152827129598653, local class serialVersionUID = 3691914940198708979. Also note that since setXxxBoundary forces XYFramingModel.EDGE, a round-trip on an ORIGIN-framed plot ends up EDGE-framed (a pan gesture already did that).

Evidence. PanZoomTest: setState_withUnpopulatedState_leavesPlotBoundariesIntact, getState_setState_roundTrip_preservesFixedBoundaries, getState_setState_roundTrip_preservesMixedBoundaryModes.
Before: all three java.lang.NullPointerException: Cannot invoke "com.androidplot.xy.BoundaryMode.ordinal()" because "mode" is null at XYPlot.getCalculatedUpperBoundary(XYPlot.java:525). After: 12/12 pass (including the three serialization tests above).

2. SampledXYSeries crashes on its first draw

Cause. activeSeries was only assigned by setZoomFactor(), so size()/getX()/getY() NPE'd whenever the plot touched the series before the ZoomEstimator ran, eg. a FIXED domain smaller than the data rejects the precomputed bounds in SeriesUtils.minMax and walks the series point by point. bounds was only assigned inside the per-zoom-level sampler threads, so a series whose size / ratio was already below the threshold (150 pts, ratio 2, threshold 100) never got any and ZoomEstimator.calculateZoom() dereferenced null. Both run in notifyListenersBeforeDraw() outside the render exception guard.

Change. resample() starts the series on the raw data (1x) and computes bounds from the raw data (SeriesUtils.minMax(rawData)) when no zoom level is generated, so getBounds()/minMax() are never null after construction. ZoomEstimator.run() leaves the zoom untouched if a series has no bounds (setBounds(null) is public) and calculateZoom() returns 1 in that case.

Fixed slightly differently than described: the 150-point example from the report does not reach the size() NPE on the unfixed code because null bounds make SeriesUtils.minMax skip the series (continue); it reaches the ZoomEstimator NPE. The size() NPE needs a series whose sampler loop does run (bounds non-null, activeSeries still null) plus a FIXED domain; the tests cover both shapes.

Evidence. SampledXYSeriesTest: constructor_withNoSampledZoomLevels_isUsableBeforeSetZoomFactor, constructor_withSampledZoomLevels_isUsableBeforeSetZoomFactor, firstDraw_withFixedDomainSmallerThanData_doesNotThrow, firstDraw_withNoSampledZoomLevels_doesNotThrow; ZoomEstimatorTest.run_withNullSeriesBounds_leavesZoomUnchanged.
Before: three fail with NullPointerException: Cannot invoke "com.androidplot.xy.XYSeries.size()" because "this.activeSeries" is null at SampledXYSeries.size(SampledXYSeries.java:165); two fail with NullPointerException: Cannot invoke "com.androidplot.xy.RectRegion.getxRegion()" because "seriesBounds" is null at ZoomEstimator.calculateZoom(ZoomEstimator.java:21). After: 9/9 and 3/3 pass.

3. ConcurrentModificationException when a listener removes itself during draw

Cause. notifyListenersBeforeDraw/AfterDraw iterated the live ArrayList; addListener/removeListener/addSeries/removeSeries mutate it under the same monitor as renderOnCanvas, so a listener (or a SimpleXYSeries, which is auto-registered as a PlotListener) removing itself from a callback mutated the list mid-iteration. If it was the last listener the iterator threw a CME (uncaught on the render thread in background mode); otherwise ArrayList's iterator ended early and the remaining listeners were silently skipped.

Change. listeners is a CopyOnWriteArrayList; Plot.getListeners() (protected) now returns List<PlotListener> instead of ArrayList.

Evidence. PlotTest: renderOnCanvas_listenerRemovingItselfDuringDraw_doesNotThrow, renderOnCanvas_seriesRemovingItselfDuringDraw_doesNotThrow, renderOnCanvas_listenerRemovingItselfDuringDraw_stillNotifiesRemainingListeners.
Before: first two java.util.ConcurrentModificationException at Plot.notifyListenersAfterDraw(Plot.java:657); third AssertionFailedError: expected:<1> but was:<0> (following listener skipped). After: 23/23 pass.

4. centerOnRangeOrigin with FIXED / GROW / SHRINK throws every frame

Cause. updateRangeMinMaxForOriginModel() only implemented AUTO and threw UnsupportedOperationException for every other mode from calculateMinMaxVals(), while the domain version supports all four.

Change. Implemented the range version as an exact mirror of updateDomainMinMaxForOriginModel() (x -> y, prevMinX/MaxX -> prevMinY/MaxY).

Evidence. XYPlotTest: testRangeOriginFixedMode (50 +/- 20 -> 30..70), testRangeOriginGrowMode, testRangeOriginShrinkMode, mirroring the existing domain tests.
Before: UnsupportedOperationException: Range Origin Boundary Mode not yet supported: FIXED (resp. GROW, SHRINK) at XYPlot.updateRangeMinMaxForOriginModel(XYPlot.java:748). After: pass.

5. Region.intersects NPE on a null (unbounded) edge

Cause. RectRegion.intersects documents null as infinity, but Region.intersects(Number, Number) unboxed both params and this region's own min/max unconditionally. Reached from LineAndPointRenderer fill regions via bounds.intersects(formatter.getRegions().elements()). Two further problems on the same path: Region(v1, v2) swapped a null value to the opposite edge (new Region(null, 5) became min=5, max=null, ie. "everything above 5"), and after intersects accepted the region the renderer called bounds.transform(thisRegion, ...), which unboxed the null coordinates before the isFullyDefined() guard.

Change. Region.intersects substitutes -Infinity / +Infinity for null on either side, keeping the existing comparison structure (which also handles inverted min > max regions). The constructor only reorders when both values are non-null. LineAndPointRenderer.renderPath replaces a null edge with the corresponding edge of the plot's visible bounds before transforming (nothing beyond them is drawable anyway) and checks isFullyDefined() on the transformed region, so such regions now render clipped to the plot area.

Evidence. RegionTest: testConstructor_preservesNullPlacement, testIntersects_nullIsInfinity; RectRegionTest.testIntersects_nullBoundsAreTreatedAsInfinity (RectRegion(0,10,0,10).intersects(new RectRegion(null, 5, null, 5)) true, (null, -5, null, -5) false, plus the list form); LineAndPointRendererTest.renderPath_rendersRegionsWithUnboundedEdges.
Before: constructor test AssertionError: expected null, but was:<5.0>; intersects tests NullPointerException: Cannot invoke "java.lang.Number.doubleValue()" because "line2Min" is null at Region.intersects(Region.java:181); renderer test (run with only the Region fixes applied) NullPointerException: ... because "x" is null at RectRegion.transform(RectRegion.java:69). After: 12/12, 12/12, 7/7 pass.

6. Marker lists unsynchronized

Cause. addMarker/removeMarker/removeMarkers/removeXMarkers/removeYMarkers mutate plain ArrayLists that XYGraphWidget.drawMarkers iterates on the render thread through the live lists returned by getXValueMarkers()/getYValueMarkers().

Change. Both lists are CopyOnWriteArrayLists (field type List); the getters keep their semantics.

Evidence. XYPlotTest: addRemoveMarker_whileIteratingYValueMarkers_doesNotThrow, addRemoveMarker_whileIteratingXValueMarkers_doesNotThrow (iterate the returned list while calling addMarker/removeMarker/removeMarkers).
Before: java.util.ConcurrentModificationException. After: 25/25 pass.

7. SimpleXYSeries NPEs after useImplicitXVals()

Cause. useImplicitXVals() sets xVals = null; setModel() called xVals.clear() before its own null guard (dead code), and resize()/setX()/setXY() dereferenced xVals unguarded.

Change. setModel skips the clear and, for Y_VALS_ONLY, does not re-materialise x-vals when they are implicit (XY_VALS_INTERLEAVED recreates them as before); resize is driven by yVals.size() and only touches xVals when non-null; setXY sets only the y value on an implicit-x series (documented in the javadoc); setX throws IllegalStateException("Cannot set an x value on a series that uses implicit x-vals.").

Evidence. SimpleXYSeriesTest: setModel_afterUseImplicitXVals_yValsOnly, setModel_afterUseImplicitXVals_xyInterleaved, resize_afterUseImplicitXVals, setXY_afterUseImplicitXVals_setsYOnly, setX_afterUseImplicitXVals_throwsIllegalStateException.
Before: NPEs at SimpleXYSeries.setModel(:130), resize(:203), setXY(:229), and setX(:179) (expected IllegalStateException, got NullPointerException). After: 18/18 pass.

8. LayerListOrganizer.moveAbove / moveBeneath with an unknown reference

Cause. Both removed objectToMove before looking up the reference; with an unknown reference indexOf returned -1, so moveBeneath threw IndexOutOfBoundsException having already dropped the element, and moveAbove silently re-inserted it at the bottom.

Change. Both verify the reference is present before mutating and throw IllegalArgumentException otherwise, leaving the list unchanged.

Evidence. LinkedLayerListOrganizerTest: filled in the empty testMoveAbove/testMoveBeneath (normal case; pass before and after) and added testMoveAbove_unknownReference_throwsAndLeavesListUnchanged, testMoveBeneath_unknownReference_throwsAndLeavesListUnchanged.
Before: AssertionError: expected IllegalArgumentException (moveAbove silently succeeded) and IndexOutOfBoundsException: Index: -1, Size: 2 at LayerListOrganizer.moveBeneath(LayerListOrganizer.java:51). After: 10/10 pass.

Found, not fixed (out of scope)

XYConstraints.contains(RectRegion) passes rectRegion.getMinY() for both the x and y arguments of the first contains(x, y) call, so the SeriesUtils.minMax fast path is rejected more often than it should be. Left as-is; worth a separate fix.

Verification

./gradlew testDebugUnitTest lint :androidplot-core:assembleRelease :demoapp:assembleDebug succeeds. Unit tests: 254 (224 on master + 30 new), 1 pre-existing skip, 0 failures. CRLF files (XYPlot.java, SimpleXYSeries.java, LineAndPointRenderer.java and the CRLF test files) keep their line endings; diffs touch only the changed lines.

halfhp and others added 9 commits September 7, 2026 09:02
PanZoom.State fields default to null, and getState() returned that
never-populated object until the first gesture.  Restoring it via
setState() stored null boundary modes into the plot's constraints, which
XYPlot.calculateMinMaxVals() then dereferenced on the render thread.  A
partially configured PanZoom (eg. Pan.HORIZONTAL + Zoom.NONE) left the
range modes null even after a gesture.

getState() now snapshots the plot's actual user boundaries and boundary
modes for each edge of both axes, and State.apply() skips any edge whose
mode is null so a stale or legacy state cannot poison the plot.  XYPlot
exposes its boundary modes and user min/max values to support this.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
activeSeries was only assigned by setZoomFactor(), so size()/getX()/getY()
threw an NPE whenever the plot touched the series before the ZoomEstimator
had run, eg. when a FIXED domain rejected the precomputed bounds and
XYPlot.calculateMinMaxVals() walked the series point by point.  bounds was
only assigned inside the per-zoom-level sampler threads, so a series whose
size / ratio was already below the threshold never got any, and
ZoomEstimator.calculateZoom() then dereferenced null.  Both paths run in
notifyListenersBeforeDraw() outside the render exception guard.

resample() now starts the series on the raw data and computes bounds from
the raw data when no zoom level is generated; ZoomEstimator leaves the zoom
untouched if a series has no bounds.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ring draw

notifyListenersBeforeDraw / AfterDraw iterated the live listener ArrayList
while addListener / removeListener / addSeries / removeSeries (all held under
the same monitor as renderOnCanvas) mutate it.  A listener removing itself,
or a series (SimpleXYSeries is auto-registered as a PlotListener) removing
itself, threw a CME on the render thread if it was the last listener, or
silently skipped the following listeners otherwise.

The listener list is now a CopyOnWriteArrayList; getListeners() returns a
List.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
updateRangeMinMaxForOriginModel() only implemented BoundaryMode.AUTO and
threw UnsupportedOperationException for every other mode from within
calculateMinMaxVals(), ie. on every frame, even though the domain
equivalent supports all four modes.  The range implementation now mirrors
updateDomainMinMaxForOriginModel().

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
RectRegion.intersects() documents null as infinity, but Region.intersects()
unboxed both params (and this region's own min / max) unconditionally.
A fill region with an unbounded edge therefore crashed the
LineAndPointRenderer via bounds.intersects(formatter.getRegions()).

Region.intersects() now substitutes -Infinity / +Infinity for null on
either side, keeping the existing comparison structure.  The
Region(v1, v2) constructor also stopped swapping a null value to the
opposite edge, which turned "everything below 5" into "everything above 5".

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
addMarker / removeMarker / removeMarkers / removeXMarkers / removeYMarkers
mutate plain ArrayLists that XYGraphWidget.drawMarkers() iterates on the
render thread via the live lists returned by getXValueMarkers() /
getYValueMarkers(), throwing ConcurrentModificationException mid-frame.
Both lists are now CopyOnWriteArrayLists, keeping the getters' semantics.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
useImplicitXVals() sets xVals to null, but setModel() cleared xVals before
its own null guard (making that guard dead code), and resize(), setX() and
setXY() dereferenced xVals unconditionally.

setModel() and resize() now keep the x-vals implicit, setXY() sets only the
y value on an implicit-x series, and setX() throws an IllegalStateException
with an explanation instead of an NPE.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…Beneath

Both methods removed objectToMove before looking up the reference.  With an
unknown reference, indexOf() returned -1 so moveBeneath() threw
IndexOutOfBoundsException having already dropped the element, and
moveAbove() silently re-inserted it at the bottom.  Both now verify the
reference is present before mutating anything and throw
IllegalArgumentException otherwise, leaving the list unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
With Region.intersects() accepting null edges, a fill region such as
RectRegion(null, 5, null, 5) reached bounds.transform(), which still
unboxed the null coordinates; the isFullyDefined() guard sat after that
call.  The renderer now replaces a null edge with the corresponding edge
of the plot's visible bounds before transforming, so the region is drawn
clipped to the plot area as the "null means infinity" contract implies.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Sep 7, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 88.88889% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.82%. Comparing base (066828c) to head (4462187).

Files with missing lines Patch % Lines
.../java/com/androidplot/xy/LineAndPointRenderer.java 50.00% 0 Missing and 5 partials ⚠️
...core/src/main/java/com/androidplot/xy/PanZoom.java 84.61% 4 Missing ⚠️
...-core/src/main/java/com/androidplot/xy/XYPlot.java 90.90% 0 Missing and 3 partials ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #145      +/-   ##
============================================
+ Coverage     71.79%   72.82%   +1.02%     
- Complexity     1261     1318      +57     
============================================
  Files           109      109              
  Lines          5202     5270      +68     
  Branches        547      573      +26     
============================================
+ Hits           3735     3838     +103     
+ Misses         1111     1078      -33     
+ Partials        356      354       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Adding per-edge boundary modes changed the JVM-computed UID of
PanZoom.State, so a State serialized by an older library version (eg. in
an Activity's saved instance state that survives an app update) would have
failed to deserialize with InvalidClassException.  The UID is now pinned to
the value serialver computes for the class on master
(-4221152827129598653L); old streams load with the new per-edge fields
left null, which State.apply() already skips.

Tests assert the UID, round-trip a State through ObjectOutputStream /
ObjectInputStream, and deserialize a stream captured from the master
build of the class.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@halfhp
halfhp merged commit d28ed17 into master Sep 7, 2026
1 check passed
@halfhp
halfhp deleted the fix-crashes branch September 7, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants