| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0035089 | Lazarus | TAChart | public | 2019-02-15 02:32 | 2019-06-12 14:24 |
| Reporter | Marcin Wiazowski | Assigned To | wp | ||
| Priority | normal | Severity | minor | Reproducibility | random |
| Status | closed | Resolution | fixed | ||
| Product Version | 2.0 | Product Build | 60424 | ||
| Target Version | 2.2 | Fixed in Version | |||
| Summary | 0035089: TAChart: crash in IDE | ||||
| Description | Steps to reproduce: - load the attached Reproduce application in IDE, - in Object Inspector, select Chart1FieldSeries, go to the Source property, and try to change its value from Chart1FieldSeries.Builtin to ListChartSource. IDE (2.0.0) randomly crashes or not. | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | 60425, 60427, 60464, 60471, 60491 | ||||
| LazTarget | 2.2 | ||||
| Widgetset | Win32/Win64 | ||||
| Attached Files |
| ||||
|
|
Reproduce.zip (3,082 bytes) |
|
|
TFieldSeries requires data source having XCount >= 2 and Y >= 2. The crash is probably because TFieldSeries blindly assumes that this is always true. |
|
|
Fixed. Thanks for reporting. |
|
|
The GetXYCountNeeded() procedure, that you introduced, is great! Now it's much better, but crash is still possible: it's enough to assign a source having XCount = YCount = 2 to TFieldSeries, but decrease XCount and/or YCount *later*. I'm attaching a patch, that solves the problem. General ideas: - A series' source can be: a built-in source or some external, assigned source. - In TListChartSource.SetXCount() and SetYCount(), calls to "Notify()" have been added (apparently they have been lacking) - so source's user can verify that new XCount and YCount values are valid; if not valid, a built-in source is assigned to the series back. - But we must also prevent the built-in source from setting invalid XCount and/or YCount values. To achieve this, I added an additional constructor to TListChartSource: constructor TListChartSource.Create(AOwner: TComponent; AXCountMin, AYCountMin: Cardinal); begin Create(AOwner); <==== DEFAULT CONSTRUCTOR IS CALLED FXCountMin := AXCountMin; FYCountMin := AYCountMin; if FXCount < FXCountMin then FXCount := FXCountMin; if FYCount < FYCountMin then FYCount := FYCountMin; end; The newly introduced FXCountMin / FYCountMin values are used in SetXCount() / SetYCount() for verification - an exception is raised when trying to set too low values. The new constructor is now used in TChartSeries.Create: GetXYCountNeeded(nx, ny); FBuiltinSource := TListChartSource.Create(Self, nx, ny); so built-in source cannot be damaged anymore; a nice side effect is that all the initialization / verification code for XCount / YCount, in all TChartSeries descendants, can be just removed. Some other changes: A) XCount and YCount are Cardinals, so the GetXYCountNeeded() procedure has been modified to return also Cardinals (to avoid comparing signed Integers with unsigned Cardinals). B) Some cosmetic changes were made. C) In TListChartSource implementation, in some places, XCount is still assumed to be always equal 1. This is not true - XCount and YCount can have any values, including zero. I corrected some of these places, in some other I just added "?????" comments (I was not sure if and how the code should be corrected). Please verify the changes that I made for XCount and take a look at these "?????" comments. D) In TListChartSource.Create() andTRandomChartSource.Create, redundant assignments "FYCount := 1" have been removed. E) A message like: 'TFieldSeries requires a chart source with at least 2 x value(s)' was very confusing to me: I always read "2 x" as "2x" = "2 * of something" = "2 times". So I modified the message to: 'TFieldSeries requires a chart source with at least 2 "X" value(s) for each data point' |
|
|
test.diff (14,061 bytes) |
|
|
There's a problem: Chart sources and series are independent, and my gut feeling tells me that a series should not be allowed to tailor a source to its needs. This is because a chart source can be attached to several series. What is good for one series type may be bad for another series. Example: Suppose there is a chart source with financial data intended for an OHLC plot ("open"/"high"/"low"/"close" prices of some share value). When your code forces its minimum y count to 4 it will no longer be possible to assign this source to a line series to plot only the "open" prices - there will always be 4 curves now. The only occasion where I would accept this behavior is in relation to the built-in listsource which is owned by the series, and there is no (well, little) risk to interfere with another series. I fixed it like you by hooking into the notification which the series receives when the source is changed. However, I did not reactivate the ListSource; I did not see a necessity for it so far, and I guess that in weird cases the ListSource can have issues either, so, this is probably not a safe drawback. From your code I also took the idea to set the XCount and YCount of the build-in list source in the constructor of the series. ----------------- As for the other changes: (A) I don't like Cardinals: Although a "count" can never be negative physically, I always trap into cases where I want to set it to -1, for example to indicate that that the user did not yet enter any value for it). Changed all XCount and YCount occurances to Integer - and noticed that it breaks compilation of the visual fpspreadsheet components. Maybe there are other third-party chart sources out there. Breaking them for no real good reason is not nice, so I reverted my changes and applied your modification. ------------------- (C) XCount was introduced for something like a Gantt series (still unfinished sources lying around somewhere) which turned out to become quite complicated; therefore, I had decided to test it with the FieldSeries which so far is the only series with more than one x value. For providing multiple x values I adapted the chart sources which I could easily test. Lacking a good test series, I did not touch the TCalculatedChartSource because I cannot imagine that there is a use case with several x values, so far. I'd prefer to keep it this way until necessity arises. --------------- XCount and YCount cannot be zero. The "usual" x and y values of a data point are hard-coded elements of the TChartDataItem record which is provided by the chart source. XCount and YCount are defined such that these values are included (http://wiki.lazarus.freepascal.org/TAChart_documentation#Multi-valued_sources). And having no x and y data does not make any sense for plotting. There are, however, also series which do not have a chart source - they get their data from code or events (TFuncSeries). ------------------ TListChartSourceStrings: The ListChartSourceStrings are the strings in the grid populated by the DataPoints editor of the ListSource. For multi-valued series you first define XCount and YCount in the object inspector, then open the Datapoints editor to fill the data. I doubt whether the line if (FSource.XCount = 1) and (FSource.YCount + 3 < Cardinal(parts.Count)) then // ????? what about XCount <> 1 ? FSource.YCount := parts.Count - 3; really is needed. It will be removed when there is some hint that it causes misinterpretation of the DataPoints strings. (I refrain from doing unnecessary changes). --------------- Notify after SetXCount (and SetYCount)? I tend to keep it as it is because this happens usually before any data are available - so, nothing to update. Of course, somebody could play some risky games here like changing XCount/YCount on the fly... |
|
|
Thanks for your last, hard work. > Chart sources and series are independent, and my gut feeling tells me that a series should not be allowed to tailor a source to its needs. [...] Same standpoint here. > The only occasion where I would accept this behavior is in relation to the built-in listsource which is owned by the series, and there is no (well, little) risk to interfere with another series. Neither the current code, nor the test code that I attached, modifies series source's XCount or YCount - of course with the exception of built-in sources, that must be initialized to be usable. In the test code, FXCountMin and FYCountMin are used ONLY to guarantee, that THE BUILT-IN source will not be destroyed by decreasing its XCount or YCount. External source is NEVER modified - when external source's XCount or YCount become too low, series reverts from the external source to its built-in source (which, in this case, is guaranteed to be valid). > XCount and YCount cannot be zero Please take a look at the TDbChartSource.Create(): constructor TDbChartSource.Create(AOwner: TComponent); begin inherited Create(AOwner); ... FYCount := 0; // Set to 1 by inherited. <==== YCount is set to 0 here end; > And having no x and y data does not make any sense for plotting. Not quite sure - even for XCount = YCount = 0 there are still colors and labels - this may be usable (in other words - there is no need to artificially introduce a limitation that XCount and/or YCont must not be 0). |
|
|
constructor TRandomChartSource.Create(AOwner: TComponent); begin inherited Create(AOwner); ... FYCount := 1; <==== REDUNDANT - CAN BE REMOVED ... end; |
|
|
Ok, I made some mistake in the attached patch - the following lines should be removed: if ASource is TListChartSource then begin FXCountMin := Max(FXCountMin, TListChartSource(ASource).FXCountMin); FYCountMin := Max(FYCountMin, TListChartSource(ASource).FYCountMin); end; After removing these lines, external data source will never be modified, in any way. FXCountMin and FYCountMin will be - and remain - set only for built-in sources - for all other sources they will be always set to zero, thus inactive. |
|
|
> In the test code, FXCountMin and FYCountMin are used ONLY to guarantee, that > THE BUILT-IN source will not be destroyed by decreasing its XCount or YCount. Why should the built-in source be destroyed? It is only destroyed when its owning series is destroyed (unless the user does it intentionally...) Please provide a project in which the *current* version of TAChart still crashes the IDE due to incorrect usage of XCount/YCount. ---------- > Please take a look at the TDbChartSource.Create(): > > constructor TDbChartSource.Create(AOwner: TComponent); > begin > inherited Create(AOwner); > ... > FYCount := 0; // Set to 1 by inherited. <==== YCount is set to 0 here > end; This source is a bit different because it has a stringlist FFieldYList with the names of the y fields. YCount is set to the length of this list in SetFieldY later. Only when the user does not specify y field name(s) in the property FieldY the YCount remains 0 as defined here in the constructor. But this state does not leave the component because the ChartDataItem available in the Items[] property is set up in the GetDataItem method where the case FYCount=0 is handled by assigning default data. Therefore, the series will always get a data record from the db source even if FYCount = 0. BTW, TDbChartSource has not yet been updated to support multiple x values. Maybe I should do this now. |
|
|
> Why should the built-in source be destroyed? It is only destroyed when its owning series is destroyed (unless the user does it intentionally...) Apparently, I wasn't clear enough. By using a "destroyed" term, I was not thinking about freeing the source (by calling its destructor), but about damaging it by decreasing XCount and/or YCount in such way, that the source's client (i.e. series) starts to use out-of-bounds accesses. > Please provide a project in which the *current* version of TAChart still crashes the IDE due to incorrect usage of XCount/YCount. Here you are: I rebuilt IDE by using TAChart from r60463. Load Reproduce2 project and: 1) Side note first: there is a bug in loading ListChartSource - in Unit1.lfm there is: object ListChartSource: TListChartSource DataPoints.Strings = ( '1|1|2|2|?|' '3|3|4|4|?|' ) XCount = 2 YCount = 2 end but data points are loaded as: '1|0|1|2|?|' '3|0|3|4|?|' (ignore this problem now and just go to the next step) 2) ListChartSource has XCount = 2 and YCount = 2, and it is assigned to Chart1FieldSeries.Source initially. Now select ListChartSource in Object Inspector and set its XCount to 1 and YCount to 1. 3) Minimize the edited form and restore it again - so form (and thus the chart) will be repainted. IDE will most probably not crash immediately, but the whole form will become black - which is a first symptom of invalid memory accesses (see Reproduce2a.png). 4) Save the project and close Lazarus IDE. An exception will probably occur when exiting: "TFieldSeries requires a chart source with at least 2 x value(s) per data point." - close the message. 5) Load Lazarus IDE again and load the project. Form will not be loaded properly, and Unit1.lfm will be opened in the editor - probably three times. Try to close these Unit1.lfm's in the editor (Ctrl+F4) - when closing second of them, "List index (-1) out of bounds." exception will most probably be generated (see Reproduce2b.png). Then the IDE will most probably hang. This leads us to a conclusion, that - only by using IDE - we damaged the project. The cause is: currently, we CANNOT assign ListChartSource having XCount < 2 or YCount < 2 to TFieldSeries - but we CAN first assign ListChartSource having XCount = 2 and YCount = 2 and *LATER* decrease ListChartSource's XCount and/or YCount in the Object Inspector. Solution: apply patch2.diff. It will: A) add Notify() calls to TListChartSource.SetXCount() and SetYCount() - so series using the source will be notified about the change and will have a possibility of recovering in case of too low new source's XCount and/or YCount - i.e. in this case series will revert to its built-in source (that is guaranteed to be valid). B) guarantee, that the built-in source is always valid, i.e. cannot be "damaged" by setting its XCount and/or YCount to values smaller than allowed - this is internally achieved by using TListChartSource.FXCountMin and TListChartSource.FYCountMin. Although nobody will intentionally change built-in source's XCount and/or YCount, this may be done unintentionally, by calling SomeSeriesInstance.Source.XCount := ..., when the user thinks that SomeSeriesInstance.Source points to external source, but it in fact points to the built-in source. Or even more indirectly, by calling SomeSeriesInstance.ListSource.CopyFrom(SomeListChartSource). C) cosmetic change: exception message has been changed from '2 x value(s)' to '2 "X" value(s)', to make it (once again) more clear. > There's a problem: Chart sources and series are independent, and my gut feeling tells me that a series should not be allowed to tailor a source to its needs. This is an additional bonus of patch2.diff: built-in source will be *GUARANTEED* to have sufficient XCount and YCount values - so no checks/modifications will be needed. External source will either have its XCount and YCount values sufficient, or a change in the external source's XCount and YCount values will be immediately detected and - in case of too low values - the series will revert to its built-in source - which is, as already said, *GUARANTEED* to have sufficient XCount and YCount values. So, once again, no checks for/modifications of source's XCount and YCount will be needed. Some verification code has been removed recently: - in function TBubbleSeries.AddXY: "if ListSource.YCount < 2 then ListSource.YCount := 2;" - in function TBoxAndWhiskerSeries.AddXY: "if ListSource.YCount < 5 then ListSource.YCount := 5;" - in function TOpenHighLowCloseSeries.AddXOHLC: "if ListSource.YCount < 4 then ListSource.YCount := 4;" Removing this code prevented modifying the external data source, but also opened a possibility of a runtime crash; applying the patch will make this (already nonexistent) code needless, but will also prevent from crashes. I just applied patch2.diff and rebuilt Lazarus IDE - everything works properly now. In step 2) above, when changing ListChartSource's XCount or YCount to 1, IDE just informs that this source can no longer be used by TFieldSeries, and Chart1FieldSeries' Source is reverted to Chart1FieldSeries.Builtin. In step 3), the form does NOT become black. In step 4), project is NOT damaged. Loading project in step 5) does NOT cause any problems. |
|
|
Reproduce2a.png (4,299 bytes) |
|
|
Reproduce2b.png (14,056 bytes) |
|
|
Reproduce2.zip (3,128 bytes) |
|
|
patch2.diff (4,789 bytes) |
|
|
Thanks. Due to clear steps to reproduce I finally understood your point and your patch. Applied in r60464. There is a problem, though. When I tried to reproduce the issue before the patch I somehow ended with a project which still crashes now. I cannot reproduce the steps, but the effect is the same as when I edit the lfm file of your demo project "reproduce2.zip" and set the XCount and YCount to 1 (instead of 2). Obviously the streaming mechanism may lead to a situation where your patch is ignored. Of course, editing the lfm file is something for the tough guys only, but it may be indicative of a remaining issue somewhere. |
|
|
> Side note first: there is a bug in loading ListChartSource - in Unit1.lfm there is: > > object ListChartSource: TListChartSource > DataPoints.Strings = ( > '1|1|2|2|?|' > '3|3|4|4|?|' > ) > XCount = 2 > YCount = 2 > end > > but data points are loaded as: > > '1|0|1|2|?|' > '3|0|3|4|?|' Looks like that, when the DataPoint.Strings are read from the LFM, XCount and YCount are still 1. The line that you mentioned above if (* (FSource.XCount = 1) and *) (FSource.YCount + 3 < Cardinal(parts.Count)) then FSource.YCount := parts.Count - 3; (the first part, FSource.XCount = 1, here commented, is an addition by myself when I added multi-X support) just takes care of this: it calculates YCount from the items in the DataPointStrings lines - but when XCount > 1 this is no longer possible... No idea, yet. |
|
|
> When I tried to reproduce the issue before the patch I somehow ended with a project which still crashes now. Before applying the patch, it was possible to generate a crashing form - just by performing the steps 1) .. 4) above. > I cannot reproduce the steps This is because now - i.e. after applying the patch - such a crashing form cannot be created - by using IDE - anymore. > but the effect is the same as when I edit the lfm file of your demo project "reproduce2.zip" and set the XCount and YCount to 1 (instead of 2). Such a crashing form can still be created manually, by editing the LFM file - this cannot be avoided. > Obviously the streaming mechanism may lead to a situation where your patch is ignored. Before the patch, the streaming machanism just loaded the form with an invalid series' source, so many problems occured later (exceptions, hangs). Currently, the patch is not ignored: I) you cannot create a crashing form by using IDE anymore II) you can still create a crashing form manually, by editing the LFM file III) you can still load a project with crashing form, that has been created BEFORE applying the patch - this is in fact same as II) but, currently, a crashing form from II) and III) will no longer cause crashes/hangs in IDE - so it shouldn't be called "crashing" anymore - maybe an invalid form. Please note, that - after applying the patch - loading an invalid form in IDE no longer causes crashes / hangs / "List index (-1) out of bounds." exceptions - just a single message "TFieldSeries requires a chart source with at least 2 x value(s) per data point" is shown, and IDE is then still stable and working properly. So I don't think that there is anything more that should be done - please note, that you can damage the LFM in many ways, for example: Pen.Color = clBlue => Pen.Color = aaa Marks.LabelBrush.Style = bsClear => Marks.LabelBrush.AAA = bsClear Source = ListChartSource => Source = Chart1 In all these cases, IDE just displays an error message and remains stable - and this is same as it is now for an LFM file with invalid series' source. So, as for me, the problem is fixed. |
|
|
Since series' source is now guaranteed to be valid, so I'm attaching patch3.diff, that removes the obsolete checking/initialization code. It also patches a bug in TBoxAndWhiskerSeries.Draw(): there is: for i := 0 to ny-1 do DrawLabels(ADrawer, i) there should be: for i := 0 to ny-1 do DrawLabels(ADrawer, ny) It also changes uppercase to lowercase in the exception messages in TListChartSource.SetXCount and SetXCount, as you prefer. |
|
|
patch3.diff (3,956 bytes) |
|
|
patch3_updated.diff (4,175 bytes) |
|
|
I made some mistake in patch3.diff. Please ignore it and use patch3_updated.diff instead. |
|
|
I know a solution for the problem with loading TListChartSource from LFM. I'll post it here. |
|
|
> So, as for me, the problem is fixed. OK, then I mark the report as resolved. As for the DataPoints issue of the ListSource, I open a separate report. If you happen to find a solution attach it there. |
|
|
I can confirm, that all the problems described above are fixed now - with an exception of the problem with loading TListChartSource from LFM, which has been moved to 0035125. |
|
|
I found some issues in the last modified code: 1) There is: procedure TChartSeries.SetSource(AValue: TCustomChartSource); begin if FSource = AValue then exit; if FListener.IsListening then Source.Broadcaster.Unsubscribe(FListener); <==== old source unsubscribed CheckSource(AValue); <==== but old source will remain in use in case of exception here! FSource := AValue; Source.Broadcaster.Subscribe(FListener); SourceChanged(Self); end; New source should be checked BEFORE making any changes to the old source, so the code should be changed to: procedure TChartSeries.SetSource(AValue: TCustomChartSource); begin if FSource = AValue then exit; CheckSource(AValue); <==== MOVED HERE if FListener.IsListening then Source.Broadcaster.Unsubscribe(FListener); FSource := AValue; ... 2) Let's imagine a piece of code: procedure Test; var OrigSource: TCustomChartSource; begin OrigSource := SomeSeries.Source; try SomeSeries.Source := ListChartSource1; ... some operations here ... finally SomeSeries.Source := OrigSource; end; end; OrigSource is initialized to SomeSeries.FBuiltinSource. When we finally want to return to previous state, we in fact set an external source to FBuiltinSource. This confuses LFM streaming machinery - TChartSeries.IsSourceStored starts to return an invalid result. Solution is trivial: procedure TChartSeries.SetSource(AValue: TCustomChartSource); begin if AValue = FBuiltinSource then <==== ADDED AValue := nil; if FSource = AValue then exit; ... 3) Optimization: built-in source is guaranteed to be valid. So the code: procedure TChartSeries.SourceChanged(ASender: TObject); begin if ASender is TCustomChartSource then try CheckSource(TCustomChartSource(ASender)); ... can be changed to: procedure TChartSeries.SourceChanged(ASender: TObject); begin if (ASender <> FBuiltinSource) and (ASender is TCustomChartSource) then try CheckSource(TCustomChartSource(ASender)); ... This avoids executing both the "is" operator (it's time-consuming) and also the "CheckSource" call. The attached patch4.diff solves these problems. |
|
|
patch4.diff (958 bytes) |
|
|
Fix confirmed, thanks! |
