page-mod match triggered on multiple load events if contentScriptWhen: 'ready not used

RESOLVED FIXED

Status

Add-on SDK
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Mischa Zurke, Assigned: ochameau)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cherry-pick-1.0b5])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.648.133 Safari/534.16
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

Topic: https://groups.google.com/forum/?fromgroups#!topic/mozilla-labs-jetpack/AZqCTJ5ymc8

    pageMod.PageMod({
      include: "*.org",
      contentScript: 'window.alert("Page matches ruleset");',
      //contentScriptWhen: 'ready'
    })

Every time matching page gets visited the alert happens multiple times (twice in my case, testing with http://www.openrightsgroup.org/).

Enabling  "contentScriptWhen: 'ready'" will fix the problem.

Reproducible: Always
(Reporter)

Updated

7 years ago
OS: Mac OS X → Windows 7
(Reporter)

Updated

7 years ago
OS: Windows 7 → Mac OS X
(Assignee)

Updated

7 years ago
Depends on: 642145
(Assignee)

Comment 1

7 years ago
Created attachment 519680 [details]
Unit test that highlight platform bug

First. Here is a unit test that highlights a platform bug behind this SDK bug!

There is two 'document-element-inserted' events fired on HTML5 document.
Then, on such document page-mod do not expect to have multiple events for one document ...

I filled a bug on platform side: bug 642145. I hope having choose the right product/component.
(Assignee)

Comment 2

7 years ago
Created attachment 519893 [details] [diff] [review]
Awful fix and unit test agains page-mod

This is a really awful fix that is failing when there is multiple page-mod instances. But this patch contains a unit test against page-mod, that highlights this bug.

We could have a list of already matched windows but it is going to be complex to avoid memory leaks. Or tag windows by adding an attribute, but this attribute would be accessible ouside of the SDK. Or wait for platform to fix this ... Or do you have any other idea ?
(Assignee)

Comment 3

7 years ago
I proposed in bug 642447 to set contentScriptWhen to ready by default.
This may lower issues with this bug.
I think we should wait for the platform to fix this bug and get the fix into the Firefox 4 Macaw update <http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/36387069e511fbf1>.  I've requested blocking2.0 status for the platform bug.

If that doesn't happen for some reason, however, then we can consider workarounds on the SDK side to make this API work correctly on Firefox 4.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 7 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → WONTFIX
(Assignee)

Comment 5

7 years ago
I had to implement a fix around this platform bug in bug 642447, attachment 525144 [details] [diff] [review].
(Assignee)

Comment 6

7 years ago
Created attachment 529447 [details] [diff] [review]
Fix duplicate content script in firefox 4.0

I raise this fix again. I think it's important for us to support old version especially on linux where automatic update is not as easy and fast applied than on windows. Saying that the SDK is not compatible with 4.0 is weird althought we prone to keep compatibility over time.

I hit this problem quite easily on linux as update is not automatical.
mcpel faced this problem too and I think you hit it too in bug 653109, comment 11.
On firefox 4.0, annotator example is not able to remove yellow background because of multiple content script. Instead all node overred get a yellow background.

If you think that it make sense, I can execute this piece of code only on Firefox <4.0.1. It could be a good first practice of such fix for old version support.
Attachment #519893 - Attachment is obsolete: true
Attachment #529447 - Flags: review?(myk)
Comment on attachment 529447 [details] [diff] [review]
Fix duplicate content script in firefox 4.0

(In reply to comment #6)
> If you think that it make sense, I can execute this piece of code only on
> Firefox <4.0.1. It could be a good first practice of such fix for old version
> support.

Yes, please only execute this code for Firefox < 4.0.1!
Attachment #529447 - Flags: review?(myk) → review+
Whiteboard: [cherry-pick-wanted]
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
(Assignee)

Comment 8

7 years ago
Created attachment 529829 [details] [diff] [review]
Updated patch.
Attachment #529447 - Attachment is obsolete: true
Comment on attachment 529829 [details] [diff] [review]
Updated patch.

a=myk for commission during freeze
Attachment #529829 - Flags: review+
(Assignee)

Comment 10

7 years ago
Landed:
https://github.com/mozilla/addon-sdk/commit/f9af7e5bcbc4ec56ac4a07fa1b9f2d39189541e7
Cherry-picked to 1.0b5 branch:

https://github.com/mozilla/addon-sdk/commit/c514250ec7b1c7f87837fec5f6b52fee68127fca
Assignee: nobody → poirot.alex
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [cherry-pick-wanted] → [cherry-pick-1.0b5]
You need to log in before you can comment on or make changes to this bug.