fix(document): fix encoding cache poisoning and skip redundant re-detection

Two follow-up correctness fixes for the encoding auto-detection added in the
previous commit, plus the perf optimization that surfaced them:

- Skip re-running EncodingDetector when a file's encoding is already known
  (File instances are cached/reused per path for the session), so reopening a
  tab doesn't re-read + re-detect every time.

- That skip-check needed to be more careful than a plain truthiness check:
  several unrelated features (ProjectManager's "Download" command,
  AIChatPanel's image-attach, MediaViewer, etc.) read a File instance with
  {encoding: fs.BYTE_ARRAY_ENCODING} and no doNotCache for their own non-text
  purposes - which, per File.read()'s own caching, leaves that sentinel value
  cached in file._encoding even though the file was never opened as a
  document. Added EncodingDetector.isKnownTextEncoding() to tell a real text
  codec apart from that sentinel (notably still present in
  fs.SUPPORTED_ENCODINGS, so that list alone can't distinguish them).

- Found a deeper, pre-existing bug while writing the regression test for the
  above: File.read()'s cache-hit check only compares options.encoding to
  this._encoding - it doesn't invalidate _contents/_stat when something
  bare-reassigns file._encoding (which _doOpen has always done for the
  explicit-option and stored-preference branches too, not just detection).
  So: a prior raw-byte read followed by reassigning _encoding to the correct
  detected value could make the next real open cache-hit on stale raw bytes
  instead of re-reading as text. Fixed via a _setFileEncoding() helper that
  clears the stale cache whenever the encoding actually changes.

Extends EncodingDetector-test.js and file-encoding-integ-test.js with
regression coverage for both.

Author: abose

src/document/DocumentCommandHandlers.js

@@ -522,22 +522,50 @@ define(function (require, exports, module) {
                     });
             }
 
+            // File.read() caches _contents/_stat keyed together with whatever _encoding was in
+            // effect at the time of that read (see File.js). Bare-reassigning file._encoding
+            // without invalidating that cache would let the imminent real open - which reads with
+            // this same newly-assigned encoding - incorrectly cache-hit and hand back stale
+            // content cached under the OLD encoding (worse, raw bytes, if that prior read used a
+            // byte-array encoding, eg via the "Download" command or an image-attach feature)
+            // instead of doing a real re-read. Always route encoding changes through here so that
+            // can never happen.
+            function _setFileEncoding(newEncoding) {
+                if (file._encoding !== newEncoding) {
+                    file._clearCachedData();
+                    file._encoding = newEncoding;
+                }
+            }
+
             if (options && options.encoding) {
-                file._encoding = options.encoding;
+                _setFileEncoding(options.encoding);
                 _openFileInPane();
             } else {
                 const encoding = PreferencesManager.getViewState("encoding", PreferencesManager.STATE_PROJECT_CONTEXT);
                 if (encoding && encoding[fullPath]) {
-                    file._encoding = encoding[fullPath];
+                    _setFileEncoding(encoding[fullPath]);
+                    _openFileInPane();
+                } else if (EncodingDetector.isKnownTextEncoding(file._encoding)) {
+                    // File instances are cached/reused per path for the session (FileSystem._index),
+                    // so a known-text _encoding here means we've already read (and so already
+                    // detected or defaulted) this exact file once before as text - eg it's being
+                    // reopened after a close. Re-running detection would mean re-reading the whole
+                    // file from disk a second time for no new information, so just reuse what we
+                    // already know. (isKnownTextEncoding - rather than a plain truthiness check -
+                    // matters here: other code paths read this same File instance for non-text
+                    // reasons, eg downloading it or attaching it as a chat image, and can leave a
+                    // non-text sentinel encoding cached on it even though it was never opened as a
+                    // document - we must not mistake that for "already detected".)
                     _openFileInPane();
                 } else {
-                    // No explicit or previously chosen encoding for this file - see if it self-declares
-                    // a non-UTF-8 charset (eg a legacy HTML file with <meta charset="windows-1252">) so we
-                    // don't silently and irreversibly corrupt it by force-decoding as UTF-8. See EncodingDetector.
+                    // No explicit, previously chosen, or already-known encoding for this file - see if
+                    // it self-declares a non-UTF-8 charset (eg a legacy HTML file with
+                    // <meta charset="windows-1252">) so we don't silently and irreversibly corrupt it
+                    // by force-decoding as UTF-8. See EncodingDetector.
                     EncodingDetector.detectFileEncoding(file).then(function (detectedEncoding) {
-                        if (detectedEncoding) {
-                            file._encoding = detectedEncoding;
-                        }
+                        // Always land on a definite, known-text value - never leave file._encoding as
+                        // whatever a prior non-text read (see above) may have left it as.
+                        _setFileEncoding(detectedEncoding || "utf8");
                         _openFileInPane();
                     });
                 }

src/document/EncodingDetector.js

@@ -244,7 +244,25 @@ define(function (require, exports, module) {
         return result.promise();
     }
 
+    /**
+     * True if `encoding` is a real text codec name, as opposed to the non-text sentinel value
+     * (`fs.BYTE_ARRAY_ENCODING`, i.e. "byte_array" - notably still present in
+     * `fs.SUPPORTED_ENCODINGS`, so that list alone can't be used to tell them apart) that plenty of
+     * *other* call sites across the codebase pass to `File.read()` for legitimate non-text reasons
+     * (downloading a file, attaching an image, exporting a zip, etc) - and, unless they also pass
+     * `doNotCache: true`, leave cached in `file._encoding` as a side effect of File.read()'s
+     * caching (see File.js). A File instance touched that way before ever being opened as a
+     * document would otherwise look "already known" to a naive truthiness check, silently
+     * defeating both detection and the re-open-skip optimization in DocumentCommandHandlers.
+     * @param {?string} encoding
+     * @return {boolean}
+     */
+    function isKnownTextEncoding(encoding) {
+        return !!encoding && encoding !== window.fs.BYTE_ARRAY_ENCODING;
+    }
+
     exports.SNIFFABLE_EXTENSIONS      = SNIFFABLE_EXTENSIONS;
     exports.detectEncodingFromBytes   = detectEncodingFromBytes;
     exports.detectFileEncoding        = detectFileEncoding;
+    exports.isKnownTextEncoding       = isKnownTextEncoding;
 });

test/spec/EncodingDetector-test.js

@@ -271,5 +271,33 @@ define(function (require, exports, module) {
             });
 
         });
+
+        describe("isKnownTextEncoding", function () {
+
+            it("should reject falsy values", function () {
+                expect(EncodingDetector.isKnownTextEncoding(null)).toBe(false);
+                expect(EncodingDetector.isKnownTextEncoding(undefined)).toBe(false);
+                expect(EncodingDetector.isKnownTextEncoding("")).toBe(false);
+            });
+
+            it("should reject the byte-array sentinel used for non-text reads", function () {
+                // Regression test: other code paths (download-file, attach-image-to-chat, etc)
+                // read a File instance with {encoding: fs.BYTE_ARRAY_ENCODING} and no
+                // doNotCache, which - per File.read()'s own caching - leaves that sentinel value
+                // cached in file._encoding even though the file was never opened as text. Note
+                // fs.BYTE_ARRAY_ENCODING is deliberately included in fs.SUPPORTED_ENCODINGS (so
+                // reads can request it), which is exactly why this needs its own explicit check
+                // rather than relying on SUPPORTED_ENCODINGS membership.
+                expect(EncodingDetector.isKnownTextEncoding(window.fs.BYTE_ARRAY_ENCODING)).toBe(false);
+                expect(window.fs.SUPPORTED_ENCODINGS.indexOf(window.fs.BYTE_ARRAY_ENCODING)).not.toBe(-1);
+            });
+
+            it("should accept real text codec names", function () {
+                expect(EncodingDetector.isKnownTextEncoding("utf8")).toBe(true);
+                expect(EncodingDetector.isKnownTextEncoding("windows1252")).toBe(true);
+                expect(EncodingDetector.isKnownTextEncoding("utf16le")).toBe(true);
+            });
+
+        });
     });
 });

test/spec/file-encoding-integ-test.js

@@ -34,6 +34,7 @@ define(function (require, exports, module) {
         DocumentManager,
         PreferencesManager,
         CommandManager,
+        FileSystem,
         testWindow,
         brackets;
 
@@ -50,6 +51,7 @@ define(function (require, exports, module) {
             DocumentManager    = brackets.test.DocumentManager;
             PreferencesManager = brackets.test.PreferencesManager;
             CommandManager     = brackets.test.CommandManager;
+            FileSystem         = brackets.test.FileSystem;
 
             await SpecRunnerUtils.loadProjectInTestWindow(testPath);
         }, 30000);
@@ -60,6 +62,7 @@ define(function (require, exports, module) {
             DocumentManager    = null;
             PreferencesManager = null;
             CommandManager     = null;
+            FileSystem         = null;
             testWindow = null;
             brackets = null;
             // comment out below line if you want to debug the test window post running tests
@@ -202,5 +205,49 @@ define(function (require, exports, module) {
             expect(text.indexOf("�")).toBe(-1);
             expect(EditorManager.getActiveEditor().document.file._encoding).toBe("windows1252");
         });
+
+        it("Should still auto-detect correctly even if the file was previously read as raw bytes (eg via Download)", async function () {
+            // Regression test: several unrelated features (the project tree's "Download" command,
+            // attaching a file as a chat image, etc) read a File instance with
+            // {encoding: fs.BYTE_ARRAY_ENCODING} for their own non-text purposes, and - since they
+            // don't pass doNotCache - that read leaves the non-text "byte_array" sentinel cached in
+            // file._encoding as a side effect of File.read()'s own caching (see File.js), even
+            // though the file was never opened as a document. If a file gets touched that way
+            // *before* it's ever opened, detection/open logic must not mistake that sentinel for an
+            // already-known real encoding - see EncodingDetector.isKnownTextEncoding and its use in
+            // DocumentCommandHandlers.
+            const path = testPath + "/meta-charset-windows1252.html";
+
+            const encodingPrefs = PreferencesManager.getViewState("encoding", PreferencesManager.STATE_PROJECT_CONTEXT) || {};
+            delete encodingPrefs[path];
+            PreferencesManager.setViewState("encoding", encodingPrefs, PreferencesManager.STATE_PROJECT_CONTEXT);
+
+            const openDoc = DocumentManager.getOpenDocumentForPath(path);
+            if (openDoc) {
+                await awaitsForDone(CommandManager.execute("file.close", {file: openDoc.file, _forceClose: true}));
+            }
+
+            // simulate the "Download" command's raw-byte read, BEFORE this file is ever opened as
+            // a document - this is what poisons file._encoding with the non-text sentinel.
+            const file = FileSystem.getFileForPath(path);
+            await new Promise(function (resolve, reject) {
+                file.read({encoding: testWindow.fs.BYTE_ARRAY_ENCODING}, function (err) {
+                    err ? reject(err) : resolve();
+                });
+            });
+            expect(file._encoding).toBe(testWindow.fs.BYTE_ARRAY_ENCODING);
+
+            await awaitsForDone(
+                FileViewController.openAndSelectDocument(path, FileViewController.PROJECT_MANAGER));
+
+            await awaitsFor(function () {
+                const text = EditorManager.getActiveEditor().document.getText();
+                return text.indexOf("café supermarché") !== -1;
+            }, "windows-1252 html auto-detected despite prior raw-byte read", 5000);
+
+            const text = EditorManager.getActiveEditor().document.getText();
+            expect(text.indexOf("�")).toBe(-1);
+            expect(EditorManager.getActiveEditor().document.file._encoding).toBe("windows1252");
+        });
     });
 });