2041680 - "Script '<file>' result is non-structured-clonable data" error from scripting.executeScript

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Rob Wu [:robwu]]

The tabs.executeScript and scripting.executeScript (and userScripts.execute) methods return their result to the caller. When an error happens, there are two bugs though:

• if any of the script results are not structurably cloneable, the whole executeScript call rejects. In the scripting.executeScript API, the expectation is for errors to be captured in the individual results, and not for them to fail the whole call.
  • An earlier bug report (bug 1835058) also remarked this unexpected behavior (that a clone error causes the whole call to fail).
  • To fix this, we have to clone the results individually here, instead of below: https://searchfox.org/firefox-main/rev/1f7030c8de8f2b349c7d91d7b5a3253c109a1cc1/toolkit/components/extensions/ExtensionContent.sys.mjs#1500-1506,1519-1526
  • We'd also have to make sure that an error thrown by the script is cloneable (counterexample: throw document).
• the reported filename can be inaccurate. The assumption is that the serialization error came from the last executed script. This is usually accurate, except when multiple scripts are run and the error was thrown from one of the earlier scripts (e.g. throw document;).
  • This only affects scripting.executeScript and userScripts.execute (being developed in bug 1930776), not tabs.executeScript (the latter executes only one script).
  • Fixing this requires keeping track of the script that is currently being executed. I'm not sure if it is worth the effort, but documenting it here just in case.

Relevant history:

In bug 1339559, the last file name is reported when the result cannot be cloned: https://searchfox.org/firefox-main/rev/d4b3c257364526e1129a38d87d6c35e672dc10a5/toolkit/components/extensions/ExtensionContent.jsm#1136-1150
The fix tries to serialize the result of executeScript with cloneInto before returning the message via IPC, and reports the last file name otherwise. The "last file name" logic is fine here because tabs.executeScript supports only one file or code.

In bug 1317697, the implementation was refactored and now aggregates all results from the executions in the same process before cloning. This means that if any one script returns a clone error, all results are dropped.

Today, that logic is still largely the same: https://searchfox.org/firefox-main/rev/1f7030c8de8f2b349c7d91d7b5a3253c109a1cc1/toolkit/components/extensions/ExtensionContent.sys.mjs#1499-1525

Comment 1

[Rob Wu [:robwu]]

One aspect of the current approach is that we will fail fast(er) if there is a cloning error.

With executeScript, the same script is run repeatedly, and it is likely that a cloning error in any of them also happens consistently with the others.
One could argue that in this case it is preferable to reject the whole call instead of returning individual cloning errors, for the following reasons:

• scripting.executeScript (and userScripts.execute) aggregate errors across multiple executions. If an extension was careless enough to return a non-cloneable result, odds are that they don't check the returned errors either. Rejecting the whole scripting.executeScript can result in a more visible error encouraging devs to fix the issue.
• If the return value is consistent, there is no point in trying to clone all individual results - it would be a wasted effort.

Whether the above "benefits" outweigh the deviation from the shape of the scripting.executeScript definition is not clear.