Repository navigation
fix: Fixed silent reload failures in Firefox - #3911
Open
MaxFreedomPollard wants to merge 1 commit into
Open
MaxFreedomPollard wants to merge 1 commit into
MaxFreedomPollard wants to merge 1 commit into
Conversation
The Firefox Desktop and Firefox for Android runners catch reload errors
and resolve with them in their ExtensionRunnerReloadResult, but
MultiExtensionRunner replaced every resolved value with a bare
{ runnerName }, so handleReloadResults never saw those errors. A reload
that Firefox rejected, for example after a broken manifest.json, printed
nothing at all.
MultiExtensionRunner now keeps the result each runner resolves with.
Also added the missing space in "Error occurred while reloading on ..."
when no source directory is given.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
web-ext runprints nothing when Firefox refuses to reload the extension. If manifest.json is broken while it runs, the watcher fires, Firefox rejects the reload and keeps the old version loaded, and the terminal shows no error, whether the reload came from a file change or from pressing R. The "Last extension reload" time just stops updating. Firefox for Android behaves the same way, including when rebuilding the package during a reload fails.Both Firefox runners catch their reload errors and resolve with them, as their docstrings describe:
reloadExtensionBySourceDirreturns[{ sourceDir, reloadError, runnerName }](src/extension-runners/firefox-desktop.js:105-115, src/extension-runners/firefox-android.js:178-189), andreloadAllExtensionswraps those in aMultiExtensionsReloadError.MultiExtensionRunnerthen discarded that value, because its fulfilment handlers returned a fresh{ runnerName }or{ runnerName, sourceDir }(src/extension-runners/index.js:80 and 113).handleReloadResultstherefore only saw errors from runners that reject, which in practice means Chromium alone. A broken manifest is already reported for Chromium, and tests/functional/test.cli.run-target-chromium.js:151-163 checks it.MultiExtensionRunnernow keeps the result each runner resolves with ({ ...result, runnerName }), so these errors reachhandleReloadResultsand get logged:The runners are not changed. The rejection path and the one-result-per-runner shape of the returned array also stay the same. The Chromium runner resolves with
[{ runnerName }]and logs its ownExtensions.loadUnpackedfailures (chromium.js:406), so nothing is logged twice.The same function was missing a space when no source directory is given, which is the R key path: the message read
Error occurred while reloadingon "Firefox Desktop". That is fixed too.Two unit tests are added next to the existing reload tests in tests/unit/test-extension-runners/test.extension-runners.js. They mirror "logs errors to the console while reloading all extensions" and "collects reload errors in results when reloading an extension", but the failing fake runner resolves with its error the way the Firefox runners do instead of rejecting. The first test also checks the "reloading on" spacing.
Testing: I could not run the mocha suite, eslint or prettier, because the npm dependencies could not be installed in my environment. Instead I ran the real test file against the real src modules under Node 22, with small local stand-ins for the third-party packages (mocha, chai, sinon, pino, es6-error and a few imports these tests never call). All 22 tests in test.extension-runners.js pass with this change. On master the two new tests fail with
expected 0 to equal 1on the error count, and each test also fails when its part of the change is reverted. I also ran the real FirefoxDesktopExtensionRunner, RemoteFirefox and RDP client against a local fake RDP server that rejectsreloadthe way Firefox does for an invalid manifest ({ error: "Error", message: "Extension is invalid" }, the packet Actor.writeError sends). On master neither path printed anything. With this change both print the error shown above.🤖 Generated with Claude Code