Closed Bug 1204575 Opened 6 years ago Closed 6 years ago

Fix EventManager to prevent firing the same event too many times


(WebExtensions :: Untriaged, defect)

Not set


(firefox44 fixed)

Tracking Status
firefox44 --- fixed


(Reporter: rpl, Assigned: rpl)



(2 files, 4 obsolete files)

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)
Assignee: nobody → luca.greco
Attached patch fix.patch (obsolete) — Splinter Review
This small patch contains the fix for the issue described above.
Attachment #8660818 - Flags: review?(gkrizsanits)
Attached patch testcase.patch (obsolete) — Splinter Review
This patch introduces a test case which reproduce the issue (and pass with the fix patch)
Attachment #8660821 - Flags: review?(gkrizsanits)
Comment on attachment 8660818 [details] [diff] [review]

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.
Attachment #8660818 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8660821 [details] [diff] [review]

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 "
Attachment #8660821 - Flags: review?(gkrizsanits) → review+
Attached patch testcase-v2.patch (obsolete) — Splinter Review
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)
Attachment #8660821 - Attachment is obsolete: true
Attached patch testcase-v3.patch (obsolete) — Splinter Review
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)
Attachment #8663806 - Attachment is obsolete: true
Attachment #8664260 - Flags: review?(gkrizsanits)
Comment on attachment 8664260 [details] [diff] [review]

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

Thanks, I should have notices those...
Attachment #8664260 - Flags: review?(gkrizsanits) → review+
I'm attaching the previous fix.patch rebased on a recent mozilla-central.
Attachment #8660818 - Attachment is obsolete: true
I'm attaching the previous testcase-v3.patch rebased on a recent mozilla-central.
Attachment #8664260 - Attachment is obsolete: true
Follows the trybuild of the rebased patches:


There are 8 failures (most of them seems to be well known intermittents) but none of them seems to be related to this change.
Patches rebased and trypushed: "checkin-needed" keyword added.
Keywords: checkin-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.