1204575 - Fix EventManager to prevent firing the same event too many times

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

The EventManager should register the "fireFunc" only once to prevents the above issue (or it will end up sending the same event as much times as the number of the callbacks registered)

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: fix.patch

This small patch contains the fix for the issue described above.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: testcase.patch

This patch introduces a test case which reproduce the issue (and pass with the fix patch)

Comment 3

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8660818 [details] [diff] [review]
fix.patch

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

Sorry for the lag on the reviews I was on PTO. I will be more responsive from now on.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8660821 [details] [diff] [review]
testcase.patch

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

::: browser/components/extensions/test/browser/browser_ext_contentscript_connect.js
@@ +38,5 @@
> +          browser.test.notifyPass("portPostMessage2");
> +        });
> +      });
> +
> +      browser.tabs.executeScript({ file: "script.js"});

missing ws after "

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: testcase-v2.patch

I've attached the testcase.patch with the above fixed nit.

the fix.patch is really small, but the testcase one is larger so I'd like to push it to try before adding the checkin-needed flag, but my account is currently disabled due to inactivity. (I tested it locally, but only on my platform, linux64)

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: testcase-v3.patch

I've just updated the testcase, this third revision contains the following minor tweaks:

- fix order of expected and actual assertion values
- change chrome.runtime into browser.runtime)

Comment 7

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8664260 [details] [diff] [review]
testcase-v3.patch

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

Thanks, I should have notices those...

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: bug-1204575-fix.patch

I'm attaching the previous fix.patch rebased on a recent mozilla-central.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: bug-1204575-testcase-v3.patch

I'm attaching the previous testcase-v3.patch rebased on a recent mozilla-central.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Follows the trybuild of the rebased patches:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=381a455d7b5a

There are 8 failures (most of them seems to be well known intermittents) but none of them seems to be related to this change.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Patches rebased and trypushed: "checkin-needed" keyword added.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/adecc5619363
https://hg.mozilla.org/integration/fx-team/rev/75ae8874b2ae

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/adecc5619363
https://hg.mozilla.org/mozilla-central/rev/75ae8874b2ae