1213993 - Support frameId/allFrames/runAt in browser.tabs.executeScript and insertCSS

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

A number of add-on developers have asked for this.

Comment 1

[Kris Maglione [:kmag]]

Sorry, I'm already working on this.

Comment 2

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1213993: [webext] Support frameId/allFrames/runAt in browser.tabs.executeScript and insertCSS. r?billm

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

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8718672 [details]
MozReview Request: Bug 1213993: [webext] Support frameId/allFrames/runAt in browser.tabs.executeScript and insertCSS. r?billm

https://reviewboard.mozilla.org/r/34687/#review31985

I haven't looked at the tests yet, but I wanted to see your opinion about the awaitWhen issue.

::: browser/components/extensions/ext-tabs.js:510
(Diff revision 1)
> +        if (!((details.code !== null) ^ (details.file !== null))) {

if ((details.code !== null) == (details.file !== null)) seems equivalent to me, and simpler.

::: toolkit/components/extensions/ExtensionContent.jsm:58
(Diff revision 1)
> +const awaitWhen = {

I'm worried about having two separate mechanisms to monitor these events (the other being the event handlers added from the "document-element-inserted" observer. This mechanism seems a little more foolproof since it includes a pagehide handler to remove the listeners, but it's bad that they don't work the same way. Also, it introduces a new race if the extension is uninstalled before the script runs (although that's a pretty short window).

Could we instead do this by adding a one-off script to extension.scripts that would only match in the right circumstances? It could be marked as ephemeral or something so that we would remove it after it was run once.

::: toolkit/components/extensions/ExtensionContent.jsm:524
(Diff revision 1)
> +      let docShell = WebNavigationFrames.findFrame(options.frameId, global.docShell);



::: toolkit/modules/addons/WebNavigationFrames.jsm:89
(Diff revision 1)
>  function findFrame(windowId, rootDocShell) {

Can you call this findDocShell maybe? And fix up the comment since it no longer returns a FrameDetail. Also, this is now being passed a frameId rather than a windowId.

Comment 4

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34687/#review31985

> if ((details.code !== null) == (details.file !== null)) seems equivalent to me, and simpler.

I think it would have to be `(details.code === null) != (details.file === null)`. I guess that would work. I find using XOR easier to understand, since it says exactly what we're looking for. If not for the negation on the outside, anyway.

> I'm worried about having two separate mechanisms to monitor these events (the other being the event handlers added from the "document-element-inserted" observer. This mechanism seems a little more foolproof since it includes a pagehide handler to remove the listeners, but it's bad that they don't work the same way. Also, it introduces a new race if the extension is uninstalled before the script runs (although that's a pretty short window).
> 
> Could we instead do this by adding a one-off script to extension.scripts that would only match in the right circumstances? It could be marked as ephemeral or something so that we would remove it after it was run once.

I'm not really happy about it either, but it seemed like the best option.

I was originally planning on doing something like what you're suggesting (which is why I added the `deferred` property to the Script object rather than doing something simpler), but it added more corner cases than I was happy with, especially when dealing with frames.

To make allFrames work correctly, we somehow need to track when the script has executed on every frame in the tab. The only reasonable way I can think of to do that is to create a separate script for each frame, and I guess add a frameId to each one. We also have to cancel all of them if the page fails to load. Or if they fail to match the page. And we have to loop through all of them to check if the page is already at the right state, before deciding whether to inject them immediately, or add them to the pending list.

Maybe that's doable without making things too complicated... I guess I can try, but it didn't seem promising when I looked into it before.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Not sure I understand all the issues, but I'll try to propose ideas.

What if we add a property on Script called topInnerWindowID. In order for the Script to match, we require |getInnerWindowId(window.top) == this.topInnerWindowID|. I think that would handle the all_frames case?

However, now I'm thinking of a more serious problem. Say you use all_frames=true and runAt=document_end. In your patch, will the script run in a sub-frame before DOMContentLoaded has fired in that sub-frame, in which case it won't match? I wonder what Chrome does here.

Here's another question: say we defer running a script until document_idle and an iframe is injected through script or something. Do we still run the content script there?

Hope this makes sense.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #5)
> What if we add a property on Script called topInnerWindowID. In order for
> the Script to match, we require |getInnerWindowId(window.top) ==
> this.topInnerWindowID|. I think that would handle the all_frames case?

I don't think so, because we can't send the response to the executeScript
caller until the script has executed in all of the frames, so we'd need to do
some extra work to check whether there are any that haven't executed after
each one.

I think I have another idea that should work pretty well, though. I'll try it
out tomorrow.

> However, now I'm thinking of a more serious problem. Say you use
> all_frames=true and runAt=document_end. In your patch, will the script run
> in a sub-frame before DOMContentLoaded has fired in that sub-frame, in which
> case it won't match? I wonder what Chrome does here.

In my patch, the script runs at the time specified by runAt in each frame, and
the reply is sent when it's run (or failed to run) in all of them.

I'm not sure what Chrome does. I thought about testing it, but it's not an
easy thing to test.

> Here's another question: say we defer running a script until document_idle
> and an iframe is injected through script or something. Do we still run the
> content script there?

Yeah, I'm not sure what to do about that. I guess I should check what Chrome
does. At the moment I'm just executing it in whatever frames were present when
executeScript was called (or, really, when the message was received), which
seems like sensible enough behavior.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8718672 [details]
MozReview Request: Bug 1213993: [webext] Support frameId/allFrames/runAt in browser.tabs.executeScript and insertCSS. r?billm

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

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8718672 [details]
MozReview Request: Bug 1213993: [webext] Support frameId/allFrames/runAt in browser.tabs.executeScript and insertCSS. r?billm

https://reviewboard.mozilla.org/r/34687/#review33011

This looks awesome. Thanks.

::: browser/components/extensions/test/browser/browser_ext_tabs_insertCSS.js:16
(Diff revision 2)
> -        background: "rgb(0, 0, 0)",
> +        background: "transparent",
> -        foreground: "rgb(255, 192, 203)",
> -        promise: resolve => {
> -          browser.tabs.insertCSS({
> -            file: "file1.css",
> -            code: "* { background: black }",
> -          }, result => {
> -            browser.test.assertEq(undefined, result, "Expected callback result");
> -            resolve();
> -          });
> -        },
> -      },

Why was this test removed? I'm having trouble understanding what's happening here.

::: toolkit/components/extensions/schemas/extension_types.json:50
(Diff revision 2)
> +          "frameId": {

Chrome also uses |minimum: 0| here.

::: toolkit/modules/addons/WebNavigationFrames.jsm:93
(Diff revision 2)
> +  if (!docShell.sameTypeParent) {

I'm curious why you didn't use a window.top check. It doesn't matter either way; just wondering if I should stop using it.

::: toolkit/modules/addons/WebNavigationFrames.jsm:108
(Diff revision 2)
>   * @return {FrameDetail} the FrameDetail JSON object which represents the docShell.

Please fix the return type comment.

Comment 9

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34687/#review33011

> Why was this test removed? I'm having trouble understanding what's happening here.

We don't support insertCSS calls with both `file` and `code` properties now, so it doesn't work anymore.

> I'm curious why you didn't use a window.top check. It doesn't matter either way; just wondering if I should stop using it.

For sandboxed iframes, `top` points to the iframe itself, which means we could wind up with multiple frames with `frameId=0` if we use it here.

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/36d6bc68fe0f21d87b306c7712e939d8ae537b88
Bug 1213993: [webext] Support frameId/allFrames/runAt in browser.tabs.executeScript and insertCSS. r=billm

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36d6bc68fe0f