1290157 - chrome.tabs.executeScript passes the wrong arguments to its callback

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[sixtyten]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

Platform: Windows 10
Version: Nightly 50.0a1 (2016-07-26)

execute the following code in the WebExtensions debugger:
chrome.tabs.executeScript(<current tab id>, { code: "'SUCCESS'" }, results => console.log(results));


Actual results:

console logged SUCCESS.


Expected results:

console should've logged ["SUCCESS"].

As documented in https://developer.chrome.com/extensions/tabs#method-executeScript, the arguments to the callback will be an array of results, not one result.

Comment 1

[Kris Maglione [:kmag]]

We only pass an array when `allTabs` is true. I don't think we can change this now without causing problems for existing extensions.

Comment 2

[sixtyten]

When you run the above snippet in Chrome, you get ["SUCCESS"], not SUCCESS.

I personally prefer the way Firefox does it, but as it is, it's incompatible with what happens in Chrome and breaks my existing extension that I am in the process of porting over. I can easily test for this - but compatibility is the point, isn't it?

Comment 3

[:shell escalante]

querying to see what is most aligned to determine to break Fx or compat

Comment 4

[Rob Wu [:robwu]]

I analyzed AMO's database, and searched for tabs.executeScript calls that include an inline function that expects a parameter.
In other words, something of the form tabs.executeScript(optionalTabId, injectionDetails, function(theVariableInQuestion)

https://dxr.mozilla.org/addons/search?q=regexp%3Atabs%5C.executeScript%5C(%5B%5E)%5D%2Bfunction%5C(%5B%5E)%5D&redirect=false
- 48 results in 30 addons
- Addon 682676: Relies on Chrome's behavior (executeScript's callback parameter undefined = injection failed).
- Addon 683911: Relies on Chrome's behavior.
- Addon 686201: Relies on Chrome's behavior.
- Addon 686921: Parameter is not used, but present in commented-out code, probably because "if(!data)" does not tell whether injection failed (so the original code may rely on Chrome's behavior, but because that cannot be emulated in Firefox, the code was commented out).
- Addon 688158: Relies on Chrome's behavior.
- Addon 693060: Relies on Chrome's behavior.
- Addon 695957: Relies on Chrome's behavior.
- Addon 698451: Relies on Chrome's behavior.

All 22 other addons do not rely on the type of the parameter (so fixing this bug doesn't affect them).
None of the addons in this sample relies on Firefox's non-array type.
8 of them are relying on Chrome's array type parameter.

This investigation is not exhaustive, but based on this data, I think that we should migrate to Chrome's format. Preferably sooner than later, and uplift it to 50 and 49 before addon devs start relying on Firefox' current behavior.

Comment 5

[Kris Maglione [:kmag]]

Agreed, if we're going to do this, we should do it immediately, and uplift to 50 and then 49 at 1-week-ish intervals.

Comment 6

[Andy McKay]

Thanks for doing that analysis Rob. Agreed, let's change to chromes behaviour.

Comment 7

[Rob Wu [:robwu]]

Attachment: Bug 1290157 - Always pass an array to tabs.executeScript on success

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

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

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

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

https://reviewboard.mozilla.org/r/70272/#review68158

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript.js:69
(Diff revision 2)
>          browser.tabs.executeScript({
>            code: "location.href;",
>            runAt: "document_end",
>          }).then(result => {
> -          browser.test.assertTrue(typeof(result) == "string", "Result is a string");
> +          browser.test.assertEq(1, result.length, "Expected callback result");
> +          browser.test.assertTrue(typeof(result[0]) == "string", "Result is a string");

`assertEq("string", typeof(result), ...)`

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript.js:146
(Diff revision 2)
>          browser.tabs.executeScript({
>            code: "location.href;",
>            frameId: frames[0].frameId,
>          }).then(result => {
> +          browser.test.assertEq(1, result.length, "Expected one result");
>            browser.test.assertTrue(/\/file_iframe_document\.html$/.test(result), `Result for frameId[0] is correct: ${result}`);

`result[0]`

::: toolkit/components/extensions/ExtensionContent.jsm:651
(Diff revision 2)
> -    return promises[0];
> +    if (options.jsCode || options.js.length) {
> +      // executeScript.
> +      return Promise.all(promises);
> +    }
> +    // insertCSS / removeCSS.
> +    return Promise.all(promises).then(() => {});

This can be handled on the caller side.

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

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

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

https://reviewboard.mozilla.org/r/70272/#review68840

::: toolkit/components/extensions/ExtensionContent.jsm:646
(Diff revision 3)
> -    return promises[0];
> +    if (options.jsCode || options.js.length) {
> +      // executeScript.
> +      return Promise.all(promises);
> +    }
> +    // insertCSS / removeCSS.
> +    return Promise.all(promises).then(() => {});

You still haven't addressed this issue.

Comment 12

[Rob Wu [:robwu]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

https://reviewboard.mozilla.org/r/70272/#review68158

> This can be handled on the caller side.

I'm doing it here because there is no point in passing data around that is never used anyway.

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

See above comment.

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

https://reviewboard.mozilla.org/r/70272/#review68158

> I'm doing it here because there is no point in passing data around that is never used anyway.

There's no harm in it either. This is much easier to follow if `insertCSS` handles discarding the return values.

Comment 15

[Rob Wu [:robwu]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

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

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

https://reviewboard.mozilla.org/r/70272/#review68896

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86c39a5b482d
Always pass an array to tabs.executeScript on success r=kmag

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86c39a5b482d

Comment 19

[Will Bamberg [:wbamberg]]

(In reply to Kris Maglione [:kmag] from comment #5)
> Agreed, if we're going to do this, we should do it immediately, and uplift
> to 50 and then 49 at 1-week-ish intervals.

Will this be uplifted to 50?

For the docs: the existing docs describe Chrome's behavior, so I suppose the docs ought to say "Before Firefox version XX, the callback is passed ... ".

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

Approval Request Comment
[Feature/regressing bug #]: Bug 1210583
[User impact if declined]: Extensions written for the newer/expected behavior will fail to work correctly in affected browsers.
[Describe test coverage new/current, TreeHerder]: This feature is extensively tested.
[Risks and why]: Low. We weren't able to find any add-ons that rely on the former behavior, but add-ons that rely on the new behavior do exist.
[String/UUID change made/needed]: None.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello there, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

Web extensions related, Beta50+

Comment 23

[Rob Wu [:robwu]]

Verified fixed in Nightly (2016-09-23) on Linux, as follows:

1. Create a folder with two files:
manifest.json:
{
    "name": "Manual test case for bugzil.la/1290157",
    "version": "1",
    "manifest_version": 2,
    "background": {
        "scripts": ["background.js"]
    },
    "browser_action": {
    },
    "permissions": [
        "tabs",
        "*://*/*"
    ]
}

background.js

chrome.browserAction.onClicked.addListener(function() {
    chrome.tabs.executeScript({
        code: '"SUCCESS"',
    }, function(results) {
        console.log(results);
    });
});

2. Visit about:debugging, click on "Load Temporary Add-on".
3. Select the directory from step 1 and observe that a new button is added to the toolbar.
4. Visit example.com
5. Click on the add-on button.
6. Open the global JavaScript console (Ctrl-Shift-J).

Expected: Array [ "SUCCESS" ]

(In Firefox 49.0.1, I only see "SUCCESS" - sans quotes, and without the "Array" prefix)

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8db62ddbcfc2

Comment 25

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 49.0.1 under Windows 10 64-bit.

Verified fixed on Firefox 50.0b6 (20161010144024), Firefox 51.0a2 (2016-10-12) and Firefox 52.0a1 (2016-10-11 )under Windows 10 64-bit. Array [ "SUCCESS" ] is successfully displayed in Browser Console.

Comment 26

[Will Bamberg [:wbamberg]]

I've added this to the compat notes for executeScript: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Tabs/executeScript#Browser_compatibility.

Marking as dev-doc-complete, but let me know if you see anything else needed here.