1197437 - Content script run_at parameter is broken

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

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

Setting the run_at option for a content script to "document_start" seems to cause the script to run three times:

    # manifest.json
    {
     
        "manifest_version": 2,
        "name": "LD",
        "version": "1.0",
     
        "applications": {
            "gecko": {
                "id": "[e-mail]"
            }
        },
     
        "content_scripts": [
            {
                "matches": [
                    "*://www.mozilla.org/*"
                ],
                "js": [
                    "app/inject.js"
                ],
                "run_at": "document_start"
            }
        ]
     
    }
    # app/inject.js
    console.log('test..', +new Date());

Comment 1

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

Attachment: patch

Simple one-line fix, but there are a lot of changes for tests.

Comment 2

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8652647 [details] [diff] [review]
patch

Review of attachment 8652647 [details] [diff] [review]:
-----------------------------------------------------------------

My only concern is that we might skip a state in some odd cases (for some special documents). I think we don't but I'm not familiar enough with these events... Anyway, looks good to me!

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +342,5 @@
>      }
>  
>      // TODO: Somehow make sure we have the right permissions for this origin!
>      // FIXME: Need to keep this around so that I will execute it later if we're not in the right state.
> +    // Right now we just execute it unconditionally.

This comment with the FIXME together became a bit difficult to follow. Could you please change it to something like: Script should be executed only if current state has already reached its run_at state, or we have to keep it around somewhere to execute later.

@@ +415,5 @@
>      for (let [extensionId, extension] of ExtensionManager.extensions) {
>        for (let script of extension.scripts) {
>          if (script.matches(window)) {
>            let context = this.getContext(extensionId, window);
> +          context.execute(script, scheduled => scheduled == state);

Is it guaranteed that this is always called for each possible state?

Comment 3

[[e-mail]]

Comment on attachment 8652647 [details] [diff] [review]
patch

>-    context.execute(script, scheduled => scheduled == state);
>+    // Right now we just execute it unconditionally.
>+    context.execute(script, scheduled => true);
Well, that answers that question...

>   trigger(when, window) {
>     let state = this.getWindowState(window);
[What's the difference between when and state?]

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a41d20fcf77

Comment 5

[Carsten Book [:Tomcat]]

sorry had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3416308a1151 since one of this changes caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13682808&repo=mozilla-inbound

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e097dabb3ae9

Comment 7

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

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