Closed Bug 1197437 Opened 6 years ago Closed 6 years ago

Content script run_at parameter is broken

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file)

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": "ld@mozilla.org"
            }
        },
     
        "content_scripts": [
            {
                "matches": [
                    "*://www.mozilla.org/*"
                ],
                "js": [
                    "app/inject.js"
                ],
                "run_at": "document_start"
            }
        ]
     
    }
    # app/inject.js
    console.log('test..', +new Date());
Attached patch patchSplinter Review
Simple one-line fix, but there are a lot of changes for tests.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8652647 - Flags: review?(gkrizsanits)
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?
Attachment #8652647 - Flags: review?(gkrizsanits) → review+
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?]
https://hg.mozilla.org/mozilla-central/rev/e097dabb3ae9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.