Unreliable viewer file generation
Closed this issue · 13 comments
CalChart 3.4.4 crashes upon generating viewer files for the 2018 ABBA show. We found that this did not work on most machines, but worked on one of STUNT's computers for whatever reason. I'll add more detail on what his computer OS was, etc. later on. Show file has been attached for reference.
Thanks a lot, Michael! I'll take a preliminary look now to see if I see anything obvious.
Yup, crashes as expected on my end. Here is a relevant portion of the crashlog:
Exception Type: EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: EXC_I386_GPFLT
Exception Note: EXC_CORPSE_NOTIFY
Termination Signal: Segmentation fault: 11
Termination Reason: Namespace SIGNAL, Code 0xb
Terminating Process: exc handler [0]
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 com.calband.CalChart 0x000000010c09224d CalChart::Sheet::toOnlineViewerJSON(CalChart::JSONElement&, unsigned int, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >, CalChart::AnimateSheet const&) const + 9053 (memory:3834)
1 com.calband.CalChart 0x000000010c0cd092 CalChart::Show::toOnlineViewerJSON(CalChart::JSONElement&, CalChart::Animation const&) const + 3826 (cc_show.cpp:525)
2 com.calband.CalChart 0x000000010bb5aa6d CalChartDoc::exportViewerFile(wxString const&) + 2589 (calchartdoc.cpp:270)
3 com.calband.CalChart 0x000000010bb1f7c6 FieldFrame::OnCmdExportViewerFile(wxCommandEvent&) + 726 (field_frame.cpp:449)
The following for loop appears to be problematic:
// Fill in 'sheets' with the JSON representation of each sheet
JSONDataArrayAccessor sheetsAccessor = showObjectAccessor["sheets"];
unsigned i = 0;
auto animateSheetIter = compiledShow.sheetsBegin();
for (auto iter = GetSheetBegin(); iter != GetSheetEnd(); iter++, i++, animateSheetIter++) {
sheetsAccessor->pushBack(JSONElement::makeNull());
(*iter).toOnlineViewerJSON(sheetsAccessor[i], i + 1, mPtLabels, *animateSheetIter);
}
The bounds of the for loop are controlled by an iterator which explores each stuntsheet in the show; but for each stuntsheet, it accesses a sheet in the animation at that same index. The assumption is that the number of stunt sheets in a show equals the number of sheets in an animation, but that is not always the case (0-length sheets don't get compiled into the animation, I believe).
We'll need to make sure that, for each sheet in the show, the correct sheet in the animation is selected. Sheets which are absent from the animation should be skipped in the viewer file.
I should have a chance to put together a fix for this tomorrow.
@kmdurand Great, thanks!
Fix for issue noted above on branch i271; however there appears to be another issue that is only affecting Release builds (Debug doesn't seem to have a problem). I'll look into that next week.
Just to provide an update on this:
I've had a chance to start looking into the secondary issue, but it notably more complicated. The crash appears to be induced by C++ compiler optimization, and can crash in a variety of different places within the JSON exporting code. One of the crashes even occurs while first building the object that represents the JSON file grammar.
The current code for building the JSON grammar makes use of the auto keyword in combination with Boost's Spirit library. It looks like that is unacceptable because the proto objects contain references to temporaries; my current guess is that optimization shortens the lives of those temporaries and then crashes when later code attempts to use them.
Here is a relevant stack overflow question that comments on problems with using auto and Boost proto expressions:
https://stackoverflow.com/questions/24318149/segmentation-fault-only-in-optimized-builds-in-boost-spirit-qi-grammar-initializ/24321511
I'll need to pick this up again next week; won't get to it this weekend.
I've been able to fix the crashes on my machine. In addition to the changes recommended by the stackoverflow question (mentioned above), I changed some grammar rules which were incorrectly accepting references as input. Opening up a Pull Request now...
@MichaelTamaki I'll try to put together Windows and Mac builds this week with the fix, so that you can try them on your end and verify that things are working.
@MichaelTamaki I now have macOS and Windows builds with this fix. Could you pass these off to Stunt for me and see if these fix the problem? I'll give Sarah a heads up about this too.
@kmdurand Just tested on my Macbook, and it worked! It was able to successfully export a viewer file and I could play it back in the web viewer for the ABBA show. STUNT will be testing it out tonight. Thank you for your help!
Fixed in 3.5.3