The first headless import no longer crashes, and the suite stops reporting green runs it did not earn - #297
Merged
Conversation
The crash is Godot's own, and now that it is understood it can be avoided outright rather than retried around. EditorHelp::_gen_extensions_docs() dereferences the static DocTools without a null check. Discovering an extension mid-scan emits extensions_reloaded, which sends EditorNode off to regenerate the class reference on a worker thread; that thread's last act is to queue _gen_extensions_docs as a deferred call. --import quits before the message queue is flushed, so the call lands on the flush at the end of Main::cleanup() -- by which time EditorHelp::cleanup_doc() has freed the DocTools and set it to null. Naming the extension in .godot/extension_list.cfg before Godot starts means it is loaded at startup instead, extensions_reloaded never fires, and nothing is queued. Godot rewrites that file itself, so seeding it decides only the run in which it did not exist yet. The retry is gone with it. It was a second run standing in for an explanation; with the file seeded the import either works or has failed for some other reason, and a crash is a crash. Earlier commits blamed the pairing of godot-cpp master with Godot 4.7. That was wrong -- godot-cpp is not in the stack at all.
GDScript answers a bad call -- the wrong argument count, a method that is not there -- by printing SCRIPT ERROR and abandoning that one function, not by stopping the run. The case then left no assertion, no skip and no failure behind, and a suite whose failure count had not grown printed PASS. The suite has been reporting a case it never ran as green. A case that records nothing at all is now a failure, named as such, with a pointer to the SCRIPT ERROR above it. The check is three counters read before and after the call, so it costs nothing and cannot be forgotten by a suite author. only= took the runner down rather than filtering. _only_filter() returned an untyped Array from a function declared Array[String], which is a runtime error that aborts _init -- the tree then idles until --quit-after fires, about a minute, and the run ends with no marker and a status of 0. It reads as a hang. Array.assign() is the conversion that was wanted. The comments explaining --quit-after said a script error inside a case aborts _init. It does not, as above; corrected in all three files.
test_colour_and_cell_overrides_are_accepted_for_a_real_part passed a stray 1.0 between the blend operation and the priority, so every call in it failed at the first line and the case asserted nothing. The bound signature is (part_name, color, blend_op, priority) -- the rate the extra argument looks like is the colour's own alpha. Found by the check in the previous commit, which is what it is for.
The headless suite section blamed godot-cpp master paired with Godot 4.7, because that was the last axis left after six other suspects were ruled out. It was the wrong one: godot-cpp does not appear in the stack. The paragraph now names the function, the deferred call and the flush that runs it too late, and describes the seeding that avoids it instead of the retry that used to absorb it. Also states the counting rule the runner now enforces: a case that recorded nothing at all is a failure, not a pass.
"Condition !actions.custom_samplers.has(...) is true. Continuing." shows up once per player and reads like a defect. It is not one. RendererDummy::MaterialStorage builds its shader compiler with a default-constructed DefaultIdentifierActions, so custom_samplers is empty, while the real canvas renderer fills it with TEXTURE and its siblings. ss_blur.fs passes the builtin TEXTURE into ss_input_texture(), so headless cannot resolve a sampler for it and says so through ERR_CONTINUE -- compilation still returns OK. The same suite under a real renderer prints none of them, with the same 35 cases and 101 assertions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The first-scan crash is diagnosed and designed out, and three ways the suite
could report a pass it had not earned are fixed. Two of the three were found
while verifying the first.
The crash is Godot's, not godot-cpp's
EditorHelp::_gen_extensions_docs()dereferences the staticDocToolswithout a null check. The chain:
GDExtensionManager::extensions_reloadedEditorNode::_gdextensions_reloaded()callsEditorHelp::generate_doc(true, false)_gen_extensions_docsas a deferred call--importquits before the message queue is flushedEditorNode::~EditorNode()runsEditorHelp::cleanup_doc(), which joins thatthread and then frees the
DocToolsMain::cleanup()finally runs the deferred callNot a race: every other thread is parked in
condition_variable::waitwhen ithappens. The
0x8in the fault address isclass_list, theHashMapat offset8 of a null
DocTools.run-tests.*now names the extension in.godot/extension_list.cfgbeforestarting Godot, so it loads at startup,
extensions_reloadednever fires andnothing is queued. The retry is gone: with the file seeded the import either
works or has failed for some other reason.
The earlier explanation -- godot-cpp master paired with Godot 4.7 -- was the
last suspect standing, not the culprit. godot-cpp is not in the stack at all.
An aborted case read as a pass
GDScript abandons a single function on a bad call and carries straight on, so a
case that died left no assertion, no skip and no failure behind and the suite
printed PASS. A case that records nothing at all is now a failure.
It caught its own first defect immediately:
set_part_color_overridewas beingcalled with five arguments against a four-argument binding, so that case had
been asserting nothing.
only=was a hang_only_filter()returned an untypedArrayfrom a function declaredArray[String]. That aborts_init, the tree idles until--quit-afterfiresabout a minute later, and the run ends with no marker and a status of 0.
Verification
From a deleted
test_gdextension/.godot, on Godot 4.7.2-stable:Exit 0, no
handle_crash. Assertions are up from 97 because the override casenow actually runs. The guard was confirmed by breaking that case again on
purpose:
FAIL, exit 1, the case named in the failure list.The diagnosis itself came from a
dev_build=yesGodot, which is the only wayto get a symbolicated stack here -- the official build is signed with hardened
runtime so lldb cannot attach, and a stock local build is stripped.