Closed Bug 1290157 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

50 Branch
defect
Not set

Tracking

(firefox50 verified, firefox51 verified)

VERIFIED FIXED
mozilla51
Tracking Status
firefox50 --- verified
firefox51 --- verified

People

(Reporter: sixtyten, Assigned: robwu)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file)

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.
We only pass an array when `allTabs` is true. I don't think we can change this now without causing problems for existing extensions.
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?
querying to see what is most aligned to determine to break Fx or compat
Flags: needinfo?(amckay)
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.
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.
Flags: needinfo?(amckay)
Thanks for doing that analysis Rob. Agreed, let's change to chromes behaviour.
Assignee: nobody → rob
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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 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.
Attachment #8779226 - Flags: review?(kmaglione+bmo)
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 on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

See above comment.
Attachment #8779226 - Flags: review?(kmaglione+bmo)
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.
Attachment #8779226 - Flags: review?(kmaglione+bmo)
Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

https://reviewboard.mozilla.org/r/70272/#review68896
Attachment #8779226 - Flags: review?(kmaglione+bmo) → review+
Whiteboard: triaged
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/86c39a5b482d
Always pass an array to tabs.executeScript on success r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/86c39a5b482d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(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 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.
Attachment #8779226 - Flags: approval-mozilla-beta?
Hello there, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(sixtyten)
Comment on attachment 8779226 [details]
Bug 1290157 - Always pass an array to tabs.executeScript on success

Web extensions related, Beta50+
Attachment #8779226 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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)
Flags: needinfo?(sixtyten)
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.
Status: RESOLVED → VERIFIED
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.