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

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[William Durand [:willdurand]]

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.

Comment 1

[Rob Wu [:robwu]]

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

Comment 2

[Rob Wu [:robwu]]

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

Comment 3

[Rob Wu [:robwu]]

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.

Comment 4

[William Durand [:willdurand]]

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

Comment 5

[William Durand [:willdurand]]

Attachment: Bug 1740608 - Return actual errors in `scripting.executeScript()`. r?robwu!

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D151924	Bug 1740608 - Return actual errors in scripting.executeScript(). r?robwu!	willdurand	robwu: Inactive

:willdurand, could you please find another reviewer?

For more information, please visit auto_nag documentation.

Comment 7

[Suhaib Mujahid [:suhaib]]

Sorry for the noise, there was a problem with a Bugzilla API that caused the bot to think more people were inactive than normal (bug 1789646).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19a7766ef397
Return actual errors in `scripting.executeScript()`. r=robwu

Comment 9

[Cristian Tuns]

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

Comment 10

[William Durand [:willdurand]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7d7c836108c
Return actual errors in `scripting.executeScript()`. r=robwu

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c7d7c836108c

Comment 13

[rbloor]

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