fix: prevent Windows OTA vendor profile update race

src/libslic3r/GCode/GCodeProcessor.cpp

@@ -2562,43 +2562,6 @@ void GCodeProcessorResult::reset() {
     BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << boost::format(" %1%: this=%2% reset finished")%__LINE__%this;
 }
 
-bool GCodeProcessorResult::rebuild_lines_ends()
-{
-    // lock_guard rather than the lock()/unlock() pair reset() uses: this reads a file and grows a
-    // vector, so a throw between the two would leave the mutex held forever.
-    std::lock_guard<std::mutex> lock(result_mutex);
-    // A partially rebuilt map would have the G-code window slicing lines at wrong offsets all over
-    // again, so every failure below leaves lines_ends empty, which just hides the window.
-    lines_ends.clear();
-
-    FilePtr in{ boost::nowide::fopen(filename.c_str(), "rb") };
-    if (in.f == nullptr) {
-        BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": cannot open " << filename << " to rebuild the G-code line offsets.";
-        return false;
-    }
-
-    // Record the offset one past each '\n', matching what ExportLines emits during the export pass.
-    std::vector<char> buffer(65536, 0);
-    size_t            file_pos = 0;
-    for (;;) {
-        const size_t cnt_read = ::fread(buffer.data(), 1, buffer.size(), in.f);
-        if (::ferror(in.f)) {
-            BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": error while reading " << filename
-                                     << " to rebuild the G-code line offsets.";
-            lines_ends.clear();
-            return false;
-        }
-        for (size_t i = 0; i < cnt_read; ++i) {
-            if (buffer[i] == '\n')
-                lines_ends.emplace_back(file_pos + i + 1);
-        }
-        file_pos += cnt_read;
-        if (cnt_read < buffer.size())
-            break;
-    }
-    return true;
-}
-
 const std::vector<std::pair<GCodeProcessor::EProducer, std::string>> GCodeProcessor::Producers = {
     //BBS: OrcaSlicer is also "bambu". Otherwise the time estimation didn't work.
     //FIXME: Workaround and should be handled when do removing-bambu

src/libslic3r/GCode/GCodeProcessor.hpp

@@ -317,11 +317,6 @@ class Print;
 
         BedType bed_type = BedType::btCount;
         void reset();
-        // Re-scan this->filename and rebuild lines_ends from it. Needed whenever the exported G-code is
-        // modified in place after export (post-processing scripts), which shifts the byte offsets the
-        // G-code viewer uses to slice lines out of the memory-mapped file. Returns false and leaves
-        // lines_ends empty if the file cannot be read.
-        bool rebuild_lines_ends();
 
         //BBS: add mutex for protection of gcode result
         mutable std::mutex result_mutex;

src/slic3r/GUI/BackgroundSlicingProcess.cpp

@@ -261,13 +261,7 @@ void BackgroundSlicingProcess::process_fff()
         // GCodeProcessorResult, so m_gcode_result->nozzle_group_result (consumed by the H2C print-dispatch
         // nozzle mapping) survives post-processing. No preservation guard is needed on this path.
         if (m_fff_print->is_BBL_printer()) {
-            if (run_post_process_scripts(m_temp_output_path, false, "File", m_temp_output_path, m_fff_print->full_print_config()))
-                // The scripts/plugins rewrote the very file the G-code viewer memory-maps, so every byte
-                // offset in m_gcode_result->lines_ends (built while exporting the pre-processed G-code) is
-                // now stale and GCodeWindow would slice the file mid-line. Re-scan it. Note this only
-                // realigns the line framing: if a script inserts or removes lines, the moves' gcode_id
-                // still refers to the pre-processed numbering and the highlighted line stays shifted.
-                m_gcode_result->rebuild_lines_ends();
+            run_post_process_scripts(m_temp_output_path, false, "File", m_temp_output_path, m_fff_print->full_print_config());
         }
 
         BOOST_LOG_TRIVIAL(info) << __FUNCTION__ << boost::format(": export gcode finished");

src/slic3r/GUI/GCodeViewer.cpp

@@ -771,16 +771,10 @@ void GCodeViewer::SequentialView::GCodeWindow::render(float top, float bottom, f
     auto update_lines = [this](uint64_t start_id, uint64_t end_id) {
         std::vector<Line> ret;
         ret.reserve(end_id - start_id + 1);
-        // Orca: m_lines_ends indexes into a memory mapping, so it must be clamped to the mapping. If the
-        // file was modified behind our back (an in-place post-processing script that shrank it), an
-        // unchecked read is an access violation, which the caller's try/catch cannot catch on Windows.
-        const size_t file_size = m_file.size();
         for (uint64_t id = start_id; id <= end_id; ++id) {
             // read line from file
-            // Keep one entry per id: render() indexes m_lines by (id - start_id).
-            const size_t start = id == 1 ? 0 : std::min(m_lines_ends[id - 2], file_size);
-            const size_t end   = std::min(m_lines_ends[id - 1], file_size);
-            const size_t original_len = end > start ? end - start : 0;
+            const size_t start        = id == 1 ? 0 : m_lines_ends[id - 2];
+            const size_t original_len = m_lines_ends[id - 1] - start;
             const size_t len          = std::min(original_len, (size_t) 55);
             std::string  gline(m_file.data() + start, len);
 

src/slic3r/Utils/PresetUpdater.cpp

@@ -1,12 +1,14 @@
 #include "PresetUpdater.hpp"
 
 #include <algorithm>
+#include <boost/filesystem/directory.hpp>
 #include <boost/filesystem/operations.hpp>
+#include <boost/filesystem/path.hpp>
 #include <boost/nowide/fstream.hpp>
 #include <functional>
 #include <atomic>
-#include <mutex>
 #include <set>
+#include <string>
 #include <thread>
 #include <unordered_map>
 #include <ostream>
@@ -19,6 +21,7 @@
 #include <boost/lexical_cast.hpp>
 #include <boost/log/trivial.hpp>
 
+#include <vector>
 #include <wx/app.h>
 #include <wx/msgdlg.h>
 
@@ -204,7 +207,6 @@ struct PresetUpdater::priv
 
     // Per-vendor update checking
     std::set<std::string> checked_vendors;
-    std::mutex vendor_check_mutex;
     std::vector<std::thread> vendor_check_threads;
     std::atomic<bool> vendor_check_cancel{false};
 
@@ -221,10 +223,10 @@ struct PresetUpdater::priv
     priv();
 
 	void set_download_prefs(AppConfig *app_config);
-	bool get_file(const std::string &url, const fs::path &target_path) const;
-	//BBS: refine preset update logic
+    bool get_file(const std::string &url, const fs::path &target_path) const;
+    //BBS: refine preset update logic
     bool extract_file(const fs::path &source_path, const fs::path &dest_path = {});
-	void prune_tmps() const;
+    void prune_tmp(const std::string& vendor_id) const;
 	void sync_version() const;
 	void parse_version_string(const std::string& body) const;
     void sync_resources(std::string http_url, std::map<std::string, Resource> &resources, bool check_patch = false,  std::string current_version="", std::string changelog_file="");
@@ -371,14 +373,14 @@ bool PresetUpdater::priv::extract_file(const fs::path &source_path, const fs::pa
 	return true;
 }
 
-// Remove leftover paritally downloaded files, if any.
-void PresetUpdater::priv::prune_tmps() const
+// Remove a leftover partial archive for the vendor about to be synchronized.
+void PresetUpdater::priv::prune_tmp(const std::string& vendor_id) const
 {
-    for (auto &dir_entry : boost::filesystem::directory_iterator(cache_path))
-		if (is_plain_file(dir_entry) && dir_entry.path().extension() == TMP_EXTENSION) {
-			BOOST_LOG_TRIVIAL(debug) << "[Orca Updater]remove old cached files: " << dir_entry.path().string();
-			fs::remove(dir_entry.path());
-		}
+    boost::system::error_code ec;
+    const fs::path tmp_path = cache_path / (vendor_id + TMP_EXTENSION);
+    fs::remove(tmp_path, ec);
+    if (ec)
+        BOOST_LOG_TRIVIAL(warning) << "[Orca Updater]failed to remove " << tmp_path.string() << ": " << ec.message();
 }
 
 //BBS: refine the Preset Updater logic
@@ -1060,10 +1062,9 @@ void PresetUpdater::priv::check_installed_vendor_profiles() const
                         Semver resource_ver = get_version_from_json(file_path);
                         Semver vendor_ver = get_version_from_json(path_in_vendor.string());
 
-                        bool version_match = ((resource_ver.maj() == vendor_ver.maj()) && (resource_ver.min() == vendor_ver.min()));
-
-                        if (!version_match || (vendor_ver < resource_ver)) {
-                            BOOST_LOG_TRIVIAL(info) << "[Orca Updater]:found vendor "<<vendor_name<<" newer version "<<resource_ver.to_string() <<" from resource, old version "<<vendor_ver.to_string();
+                        if (vendor_ver < resource_ver) {
+                            BOOST_LOG_TRIVIAL(info) << "[Orca Updater]:found vendor " << vendor_name << " newer version "
+                                                    << resource_ver.to_string() << " from resource, old version " << vendor_ver.to_string();
                             bundles.insert(vendor_name);
                         }
                     }
@@ -1305,49 +1306,29 @@ PresetUpdater::~PresetUpdater()
 
 //BBS: change directories by design
 //BBS: refine the preset updater logic
-void PresetUpdater::sync(std::string http_url, std::string language, std::string plugin_version, PresetBundle *preset_bundle)
+void PresetUpdater::sync(std::string http_url, std::string language, std::string plugin_version, PresetBundle * /*preset_bundle*/)
 {
 	//p->set_download_prefs(GUI::wxGetApp().app_config);
 	if (!p->enabled_version_check && !p->enabled_config_update) { return; }
 
-	// Copy the whole vendors data for use in the background thread
-	// Unfortunatelly as of C++11, it needs to be copied again
-	// into the closure (but perhaps the compiler can elide this).
-    VendorMap vendors = preset_bundle ? preset_bundle->vendors : VendorMap{};
-
-    // Determine active vendor before entering the thread
-    std::string active_vendor;
-    if (preset_bundle) {
-        const Preset& printer = preset_bundle->printers.get_edited_preset();
-        if (printer.vendor)
-            active_vendor = printer.vendor->id;
-    }
-
-	p->thread = std::thread([this, vendors, active_vendor, http_url, language, plugin_version]() {
-		this->p->prune_tmps();
-		if (p->cancel)
-			return;
-		this->p->sync_version();
-		if (p->cancel)
-			return;
-        // Per-vendor config check for the active vendor at startup
-        if (!active_vendor.empty() && !vendors.empty()) {
-            this->p->sync_vendor_config(active_vendor);
-            if (p->cancel)
-                return;
-            {
-                std::lock_guard<std::mutex> lock(this->p->vendor_check_mutex);
-                this->p->checked_vendors.insert(active_vendor);
-            }
-        }
-		if (p->cancel)
-			return;
-        this->p->sync_plugins(http_url, plugin_version);
-        this->p->sync_printer_config(http_url);
-		//if (p->cancel)
-		//	return;
-		//remove the tooltip currently
-		//this->p->sync_tooltip(http_url, language);
+	p->thread = std::thread([this, http_url, language, plugin_version]() {
+		try {
+			this->p->sync_version();
+			if (p->cancel)
+				return;
+			// Vendor profile updates are triggered by check_vendor_update()
+			// after the startup printer preset has been restored.
+            this->p->sync_plugins(http_url, plugin_version);
+            this->p->sync_printer_config(http_url);
+			//if (p->cancel)
+			//	return;
+			//remove the tooltip currently
+			//this->p->sync_tooltip(http_url, language);
+		} catch (const std::exception &e) {
+			BOOST_LOG_TRIVIAL(error) << "[Orca Updater] background sync failed: " << e.what();
+		} catch (...) {
+			BOOST_LOG_TRIVIAL(error) << "[Orca Updater] background sync failed with an unknown exception";
+		}
 	});
 }
 
@@ -1356,16 +1337,17 @@ void PresetUpdater::check_vendor_update(const std::string& vendor_id)
     if (!p->enabled_config_update) return;
     if (vendor_id.empty()) return;
 
-    std::lock_guard<std::mutex> lock(p->vendor_check_mutex);
-
     if (!p->checked_vendors.insert(vendor_id).second)
         return;
 
     p->vendor_check_threads.emplace_back([this, vendor_id]() {
         try {
+            this->p->prune_tmp(vendor_id);
             this->p->sync_vendor_config(vendor_id);
         } catch (const std::exception& e) {
             BOOST_LOG_TRIVIAL(error) << "[Orca Updater] vendor update failed for " << vendor_id << ": " << e.what();
+        } catch (...) {
+            BOOST_LOG_TRIVIAL(error) << "[Orca Updater] vendor update failed for " << vendor_id << " with an unknown exception";
         }
     });
 }

tests/fff_print/CMakeLists.txt

@@ -8,7 +8,6 @@ add_executable(${_TEST_NAME}_tests
 	test_fill.cpp
 	test_flow.cpp
 	test_gcode_timing.cpp
-	test_gcodeprocessor.cpp
 	test_gcodewriter.cpp
 	test_model.cpp
 	test_multifilament.cpp

tests/fff_print/test_gcodeprocessor.cpp

@@ -1,105 +0,0 @@
-#include <catch2/catch_all.hpp>
-
-#include "libslic3r/GCode/GCodeProcessor.hpp"
-
-#include "test_utils.hpp"
-
-#include <boost/filesystem.hpp>
-#include <boost/nowide/fstream.hpp>
-
-#include <iterator>
-#include <string>
-#include <vector>
-
-using namespace Slic3r;
-
-namespace {
-
-// Writes the bytes verbatim: the G-code export is binary, so no newline translation must creep in
-// here either, otherwise the CRLF cases below would silently test LF.
-void write_gcode(const std::string &path, const std::string &content)
-{
-    boost::nowide::ofstream f(path, std::ios::binary);
-    f << content;
-}
-
-// Cuts line `id` (1-based) out of the file the way GCodeViewer's G-code window does: the text it
-// shows is bytes [lines_ends[id - 2], lines_ends[id - 1]). Reproducing that here is what makes these
-// assertions about the rendered result rather than about the offsets themselves.
-std::string line_at(const std::string &path, const std::vector<size_t> &lines_ends, size_t id)
-{
-    boost::nowide::ifstream f(path, std::ios::binary);
-    const std::string       bytes((std::istreambuf_iterator<char>(f)), std::istreambuf_iterator<char>());
-    const size_t            start = id == 1 ? 0 : lines_ends[id - 2];
-    return bytes.substr(start, lines_ends[id - 1] - start);
-}
-
-} // namespace
-
-TEST_CASE("rebuild_lines_ends realigns the line offsets after the G-code is rewritten in place", "[GCodeProcessor]")
-{
-    ScopedTemporaryFile  tmp(".gcode");
-    GCodeProcessorResult result;
-    result.filename = tmp.string();
-
-    write_gcode(tmp.string(), "G1 X1\nG1 X2\nG1 X3\n");
-    REQUIRE(result.rebuild_lines_ends());
-    REQUIRE(result.lines_ends.size() == 3);
-    CHECK(line_at(tmp.string(), result.lines_ends, 2) == "G1 X2\n");
-
-    // What a post-processing script that opens the file in Python text mode on Windows does: prepend a
-    // comment and translate every LF to CRLF. Every byte offset shifts, and cumulatively.
-    write_gcode(tmp.string(), ";EDITED\r\nG1 X1\r\nG1 X2\r\nG1 X3\r\n");
-    CHECK(line_at(tmp.string(), result.lines_ends, 2) != "G1 X1\r\n");
-
-    REQUIRE(result.rebuild_lines_ends());
-    REQUIRE(result.lines_ends.size() == 4);
-    CHECK(line_at(tmp.string(), result.lines_ends, 1) == ";EDITED\r\n");
-    CHECK(line_at(tmp.string(), result.lines_ends, 2) == "G1 X1\r\n");
-    CHECK(line_at(tmp.string(), result.lines_ends, 4) == "G1 X3\r\n");
-}
-
-TEST_CASE("rebuild_lines_ends maps a file longer than one read chunk", "[GCodeProcessor]")
-{
-    ScopedTemporaryFile  tmp(".gcode");
-    GCodeProcessorResult result;
-    result.filename = tmp.string();
-
-    const size_t line_count = 20000;
-    std::string  content;
-    for (size_t i = 0; i < line_count; ++i)
-        content += "G1 X" + std::to_string(i) + "\n";
-    // The scan reads the file in chunks, so it has to span at least one chunk boundary to be meaningful.
-    REQUIRE(content.size() > 65536);
-    write_gcode(tmp.string(), content);
-
-    REQUIRE(result.rebuild_lines_ends());
-    REQUIRE(result.lines_ends.size() == line_count);
-    CHECK(result.lines_ends.back() == content.size());
-    CHECK(line_at(tmp.string(), result.lines_ends, line_count) == "G1 X" + std::to_string(line_count - 1) + "\n");
-}
-
-TEST_CASE("rebuild_lines_ends ignores a trailing line that has no newline", "[GCodeProcessor]")
-{
-    ScopedTemporaryFile  tmp(".gcode");
-    GCodeProcessorResult result;
-    result.filename = tmp.string();
-
-    // Matches the export pass, which only ever records the offset one past a '\n'.
-    write_gcode(tmp.string(), "G1 X1\nG1 X2");
-    REQUIRE(result.rebuild_lines_ends());
-    CHECK(result.lines_ends.size() == 1);
-    CHECK(line_at(tmp.string(), result.lines_ends, 1) == "G1 X1\n");
-}
-
-TEST_CASE("rebuild_lines_ends empties the map when the G-code file cannot be read", "[GCodeProcessor]")
-{
-    // A partially rebuilt map would have the G-code window slicing lines at wrong offsets again; an
-    // empty one just hides the window.
-    GCodeProcessorResult result;
-    result.filename   = (boost::filesystem::temp_directory_path() / "orca-nonexistent-gcode-file.gcode").string();
-    result.lines_ends = {1, 2, 3};
-
-    CHECK_FALSE(result.rebuild_lines_ends());
-    CHECK(result.lines_ends.empty());
-}