Closed Bug 1740608 Opened 1 year ago Closed 2 months ago

Implement proper error handling for `scripting.executeScript()`

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(firefox107 fixed)

RESOLVED FIXED
107 Branch
Tracking Status
firefox107 --- fixed

People

(Reporter: willdurand, Assigned: willdurand)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [mv3-m2])

Attachments

(1 file)

This came up during the review of Bug 1740601: the error handling is neither great nor consistent. Chrome's implementation has some TODOs about it as well, e.g., because errors are not necessarily returned to the extension.

The current version of the patch throws if any of the script injection requests fail (e.g. permission failure).
Any script execution failure itself (e.g. invalid syntax or just a runtime error) results in a null result with a message printed to the console.

The first behavior is somewhat problematic because the extension may receive errors despite partially successful injection.
This could potentially be resolved by checking the permissions before injecting scripts (which is what Chrome is doing), but that check can fail if the frame/document navigates elsewhere during the injection (after the check, before the actual execution).

Tentatively, I think that we should include errors in each result in the results array returned by scripting.executeScript. An example of resolving this bug is to pass some indication of an error to the caller via the results array, whether via a (boolean) flag, or via an explicit error property (potentially just normalized to an object with a message property, because Error instances are not really serializable).
Let's try to agree on consistent behavior with other browser vendors that implement the scripting API.

FYI Chrome's scripting.executeScript API is finally introducing support for async functions (not in tabs.executeScript though), and they're currently ignoring script errors: https://bugs.chromium.org/p/chromium/issues/detail?id=1195807#c3

I've created a bug on Chromium's issue tracker about this:
Issue 1271527: Propagate errors from scripting.executeScript to InjectionResult
https://crbug.com/1271527

Apparently there is work being done to support cloneeable errors in bug 1556604. With that being done, it is not strictly necessary to convert errors to plain objects with a message property.

If we want to return actual errors right now, we could possibly use ClonedErrorHolder as workaround: https://searchfox.org/mozilla-central/rev/4646b826a25d3825cf209db890862b45fa09ffc3/dom/chrome-webidl/ClonedErrorHolder.webidl#6-12

Depends on: 1556604
Assignee: nobody → wdurand
Status: NEW → ASSIGNED
Blocks: 1687764
Keywords: dev-doc-needed
Pushed by wdurand@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19a7766ef397
Return actual errors in `scripting.executeScript()`. r=robwu

Backed out for causing mochitest failures on test_ext_contentscript_fission_frame.html

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_contentscript_fission_frame.html | Promise rejected, expecting rejection to match '/does is not defined/', got 'Error: An unexpected error occurred': Got the expected rejection from tabs.executeScript
Flags: needinfo?(wdurand)

mm ok, I removed something that was actually still needed but I forgot.. /me updates the patch and adds a comment. Sorry!

Flags: needinfo?(wdurand)
Pushed by wdurand@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7d7c836108c
Return actual errors in `scripting.executeScript()`. r=robwu
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch

Documentation change made and ready for review in Error handling for scripting.executeScript() #22279.

You need to log in before you can comment on or make changes to this bug.