1254003 - chrome.tabs.executeScript within chrome.tabs.create outputs "No matching message handler".

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Sean C]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160306030215

Steps to reproduce:

Attempt to executeScript inside of a created tab within a background script. Example below:

chrome.tabs.create({ url: "http://example.com" }, function(tab) {
  chrome.tabs.executeScript(tab.id, {
    code: "alert('test');"
   }, function() {
    console.log(chrome.runtime.lastError);
   });
});


Actual results:

Returned Error: No matching message handler


Expected results:

The code should have been executed within the created tab.

Comment 1

[Sean C]

Tested in browser action popup, instead of background script, and same issue occurs.

Comment 2

[Mike Taylor [:miketaylr]]

Same issue occuring here: https://github.com/webcompat/webcompat-reporter-extensions/blob/master/firefox-webext/background.js#L13

If you set a breakpoint on the call to chrome.tabs.executeScript, then hit Resume, it works as expected.

Comment 3

[adrian.arroyocalle]

I can't reproduce the bug in every page, but I think is related to loading page times. 

Adding a delay using setTimeout makes this work as expected.

Comment 4

[Sean C]

(In reply to adrian.arroyocalle from comment #3)
> I can't reproduce the bug in every page, but I think is related to loading
> page times. 
> 
> Adding a delay using setTimeout makes this work as expected.

Appears you are right. Just wrapping the executesScript in a setTimeout with a 30ms delay caused it to succeed most of the time. Tested with the various runAt's and same behaviour.

Comment 5

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254003: Don't call tabs.create callback before the tab is ready. r?gabor

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

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8737669 [details]
MozReview Request: Bug 1254003: Don't call tabs.create callback before the tab is ready. r?gabor

https://reviewboard.mozilla.org/r/44065/#review40967

::: browser/components/extensions/ext-tabs.js:453
(Diff revision 1)
> +                onLocationChange() {
> +                  webProgress.removeProgressListener(listener);
> -            resolve(TabManager.convert(extension, tab));
> +                  resolve(TabManager.convert(extension, tab));

I think you want to check the location argument of onLocationChange. A redirect can occure but also if someone decides that the load takes too long and loads another page into the tab instead that might fire onLocationChange too with the new URL, no? I thought the first onLocationChange ones get is with about:blank but if your tests passes that should not be the case unless you're really lucky with the timing... anyway, if we check the location then this cannot be an issue.

Another thing that might make sense is to remove the listener when the tab is closed and onLocationChange has not been called yet so we don't leak the whole compartment.

Comment 7

[Kris Maglione [:kmag]]

I lost track of this patch while I was traveling. Sorry to take so long to
reply.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> I think you want to check the location argument of onLocationChange. A
> redirect can occure but also if someone decides that the load takes too long
> and loads another page into the tab instead that might fire onLocationChange
> too with the new URL, no?

Yes, but we don't actually guarantee what the URL will be when the callback
fires, and by the time the onLocationChange callback fires, the `url` property
of the tab will be correct.

With the current behavior, it points to the URL that we expect to eventually
load, but the inner window ID actually points to an about:blank document that
we're going to throw away. So even if you wait long enough for the content
process to load, so you can call APIs that interact with it, they don't do
what you'd expect.

> I thought the first onLocationChange ones get is with about:blank but if
> your tests passes that should not be the case unless you're really lucky
> with the timing... anyway, if we check the location then this cannot be an
> issue.

We don't get an onLocationChange for the first about:blank load. I'm not
entirely sure why that is, but if it ever changes, these tests will fail.

I'm worried that waiting for the load of the expected URL will cause problems.
If something goes wrong, or redirects in a way we don't expect, the callback
may never be called. I'd rather not even wait for the first onLocationChange,
since it depends on network activity, but I don't think we really have a
better option, since a lot of add-ons depend on this behavior.

> Another thing that might make sense is to remove the listener when the tab
> is closed and onLocationChange has not been called yet so we don't leak the
> whole compartment.

The listener is on the webprogress for the tab's <browser>, so it should
automatically be removed when the tab is closed. I can't think of a way for it
to leak.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8737669 [details]
MozReview Request: Bug 1254003: Don't call tabs.create callback before the tab is ready. r?gabor

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

Comment 9

[Kris Maglione [:kmag]]

I rearranged the code in this function a bit, since it was getting to the point where I couldn't follow it.

Comment 10

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8737669 [details]
MozReview Request: Bug 1254003: Don't call tabs.create callback before the tab is ready. r?gabor

https://reviewboard.mozilla.org/r/44065/#review44833

(In reply to Kris Maglione [:kmag] from comment #7)
> I lost track of this patch while I was traveling. Sorry to take so long to
> reply.

No worries :)

> I'm worried that waiting for the load of the expected URL will cause
> problems.
> If something goes wrong, or redirects in a way we don't expect, the callback
> may never be called. I'd rather not even wait for the first onLocationChange,
> since it depends on network activity, but I don't think we really have a
> better option, since a lot of add-ons depend on this behavior.

Yeah this makes sense... maybe mentioning in the documentation that create does
not guarantee that the specified page will be loaded and there won't be any
redirection, so for code injection one must be carefull.

> > Another thing that might make sense is to remove the listener when the tab
> > is closed and onLocationChange has not been called yet so we don't leak the
> > whole compartment.
> 
> The listener is on the webprogress for the tab's <browser>, so it should
> automatically be removed when the tab is closed. I can't think of a way for
> it
> to leak.

I don't know why I thought that we will leak here to be honest... but one thing
that I don't get is how add-on disabling/uninstalling work now. Do we nuke the
scope of the add-on? That would make sure that cases like this won't leak / 
keep the add-on alive in a broken state because nukeWindow/nukeSandbox replaces
all the cross compartment wrappers with dead object proxies.

Anyway, r=me unless you think the disable/uninstall scenario is something we
should worry about and address here.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)
> Yeah this makes sense... maybe mentioning in the documentation that create
> does not guarantee that the specified page will be loaded and there won't be
> any redirection, so for code injection one must be carefull.

Yeah, that's a good point. I'll add some notes.

> but one thing that I don't get is how add-on disabling/uninstalling work
> now. Do we nuke the scope of the add-on? That would make sure that cases
> like this won't leak / keep the add-on alive in a broken state because
> nukeWindow/nukeSandbox replaces all the cross compartment wrappers with dead
> object proxies.

We do nuke the scopes of the add-on, but we don't nuke the scopes of the API,
so I don't think that will help here. This listener should always be
short-lived enough that it doesn't worry me. In other places we do explicit
context unload checks.

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/8afadb96e97704a98492c2286fb92479deb3365f
Bug 1254003: Don't call tabs.create callback before the tab is ready. r=gabor

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/6f5e1ec74f4908a610bbe0274e03ff31abc3bd3c
Backed out changeset 8afadb96e977 for too many intermittent failures (bug 1254003).

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/a57938cd29d2ff6c3d6bc6efa853f01a800bb326
Bug 1254003: Don't call tabs.create callback before the tab is ready. r=gabor

Comment 15

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/0225961ad7ad - a few more candidate platforms this time, at least, Linux PGO and 10.6 opt and WinXP debug (and a sterling bunch of platforms that is).

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/a3fbed43aa5575de29603c40f418a4733b6deb5c
Bug 1254003: Don't call tabs.create callback before the tab is ready. r=gabor

Comment 17

[Guilherme Lima]

This got backed out right after it landed in https://hg.mozilla.org/integration/fx-team/rev/f4fd92a0496f

Comment 18

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/f42a04d822baa22a4741c4fff1aab2054857e398
Bug 1254003: Don't call tabs.create callback before the tab is ready. r=gabor

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f42a04d822ba