1339559 - Include script name in "Script returned non-structured-clonable data" error

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Ian Bicking (:ianbicking)]

In my WebExtension I'm getting this error on the console:

  Unchecked lastError value: Error: Script returned non-structured-clonable data (ExtensionCommon.jsm:265)

I believe this error is because of this (somewhat unexpected) behavior:

  > The result of the script is the last evaluated statement, which is similar to what would be output (the results, not any console.log() output) if you executed the script in the Web Console.

Note this does not result in the browser.tabs.executeScript promise failing.

If the error message included the script name that would be somewhat helpful.  If it included a reference to documentation that would be much more helpful.

The same code in a Chrome Extension did not have this behavior (maybe it just swallows the error, which might also be fine).

Comment 1

[Kris Maglione [:kmag]]

(In reply to Ian Bicking (:ianb) from comment #0)
> Note this does not result in the browser.tabs.executeScript promise failing.

It does, if you use the promise version of the API. "Unchecked lastError value" messages only happen when using the callback version of the API, and are equivalent to rejected promises.

> The same code in a Chrome Extension did not have this behavior (maybe it
> just swallows the error, which might also be fine).

Chrome simply returns null, without reporting an error, when the script returns a non-structured-clonable value, which I think is a confusing footgun that I don't want to emulate.

Comment 2

[Ian Bicking (:ianbicking)]

I also notice when the script returns a function (which I got by doing window.someFunction = function ();) that I get an Xray error which looks really scary, but I believe is another cloneability issue.

Comment 3

[Tomislav Jovanovic :zombie]

Attachment: Bug 1339559 - Identify script that resulted in non-structured-clonable data

Review commit: https://reviewboard.mozilla.org/r/119944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119944/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8846921 [details]
Bug 1339559 - Identify script that resulted in non-structured-clonable data

https://reviewboard.mozilla.org/r/119944/#review121816

::: toolkit/components/extensions/ExtensionContent.jsm:1022
(Diff revision 1)
>          // we try to send it back over the message manager.
>          Cu.cloneInto(result, target);
>        } catch (e) {
> -        return Promise.reject({message: "Script returned non-structured-clonable data"});
> +        const {js} = options;
> +        const name = js.length ? js[js.length - 1] : "<anonymous code>";
> +        return Promise.reject({message: `Script '${name}' result is non-structured-clonable data`});

Please also add a `fileName` property.

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8846921 [details]
Bug 1339559 - Identify script that resulted in non-structured-clonable data

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119944/diff/1-2/

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8846921 [details]
Bug 1339559 - Identify script that resulted in non-structured-clonable data

https://reviewboard.mozilla.org/r/119944/#review123096

::: toolkit/components/extensions/ExtensionCommon.jsm:237
(Diff revision 2)
>      if (error instanceof this.cloneScope.Error) {
>        return error;
>      }
> -    let message;
> -    if (instanceOf(error, "Object") || error instanceof ExtensionError) {
> -      message = error.message;
> +    let message, fileName;
> +    if (instanceOf(error, "Object") || error instanceof ExtensionError ||
> +        typeof error == "object" && this.principal.subsumes(Cu.getObjectPrincipal(error))) {

Please put parens around the `&&` clause. I'm pretty sure we have an ESLint rule that requires this.

Comment 7

[Tomislav Jovanovic :zombie]

Attachment: Bug 1339559 - Enable no-mixed-operators ESLint rule

Review commit: https://reviewboard.mozilla.org/r/121428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121428/

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8848498 [details]
Bug 1339559 - Enable no-mixed-operators ESLint rule

https://reviewboard.mozilla.org/r/121428/#review123436

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_runAt.html:89
(Diff revision 1)
>          browser.test.assertTrue(states[2] == "complete",
>                                  `document_idle state is valid: ${states[2]}`);
>  
>          // If we have the earliest valid states for each script, we're done.
>          // Otherwise, try again.
> -        success = (states[0] == "loading" || DEBUG &&
> +        success = ((states[0] == "loading" || DEBUG) &&

I was sceptical of the rule at first.

Comment 9

[Tomislav Jovanovic :zombie]

Comment on attachment 8848498 [details]
Bug 1339559 - Enable no-mixed-operators ESLint rule

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121428/diff/1-2/

Comment 10

[Tomislav Jovanovic :zombie]

Comment on attachment 8848498 [details]
Bug 1339559 - Enable no-mixed-operators ESLint rule

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121428/diff/2-3/

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8848498 [details]
Bug 1339559 - Enable no-mixed-operators ESLint rule

https://reviewboard.mozilla.org/r/121428/#review123616

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_executeScript_runAt.html:89
(Diff revision 3)
>          browser.test.assertTrue(states[2] == "complete",
>                                  `document_idle state is valid: ${states[2]}`);
>  
>          // If we have the earliest valid states for each script, we're done.
>          // Otherwise, try again.
> -        success = (states[0] == "loading" || DEBUG &&
> +        success = ((states[0] == "loading" || DEBUG) &&

... ouch

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34cac2e12b45
Identify script that resulted in non-structured-clonable data r=kmag
https://hg.mozilla.org/integration/autoland/rev/9c152fdd4d55
Enable no-mixed-operators ESLint rule r=kmag

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34cac2e12b45
https://hg.mozilla.org/mozilla-central/rev/9c152fdd4d55