Closed Bug 1339559 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ianbicking, Assigned: Tomislav)

References

Details

(Whiteboard: [triaged])

Attachments

(2 files)

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).
(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.
Summary: Error without information: "Script returned non-structured-clonable data" → Include script name in "Script returned non-structured-clonable data" error
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.
Priority: -- → P2
Whiteboard: [triaged]
Assignee: nobody → tomica
Status: NEW → ASSIGNED
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.
Attachment #8846921 - Flags: review?(kmaglione+bmo)
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.
Attachment #8846921 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8848498 - Flags: review?(kmaglione+bmo)
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
Attachment #8848498 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34cac2e12b45
https://hg.mozilla.org/mozilla-central/rev/9c152fdd4d55
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.