Closed Bug 1204575 Opened 6 years ago Closed 6 years ago

Fix EventManager to prevent firing the same event too many times

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: rpl, Assigned: rpl)

Details

Attachments

(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]
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.
Attachment #8660818 - Flags: review?(gkrizsanits) → review+
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 "
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]
testcase-v3.patch

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:

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