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

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Mischa Zurke]

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

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: 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.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: 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 ?

Comment 3

[Alexandre Poirot [:ochameau]]

I proposed in bug 642447 to set contentScriptWhen to ready by default.
This may lower issues with this bug.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 5

[Alexandre Poirot [:ochameau]]

I had to implement a fix around this platform bug in bug 642447, attachment 525144 [details] [diff] [review].

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: 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.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

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!

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Updated patch.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 529829 [details] [diff] [review]
Updated patch.

a=myk for commission during freeze

Comment 10

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/f9af7e5bcbc4ec56ac4a07fa1b9f2d39189541e7

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Cherry-picked to 1.0b5 branch:

https://github.com/mozilla/addon-sdk/commit/c514250ec7b1c7f87837fec5f6b52fee68127fca