Closed Bug 757663 Opened 12 years ago Closed 12 years ago

AddonManagerInternal.callAddonlListeners() informs it's listeners unsafely, causing silent skips in the notification loop

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: xabolcs, Assigned: Unfocused)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #756960 +++

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120518 Firefox/14.0a2
Build ID: 20120518042008

Steps to reproduce:
Part 1:
1. got a Toolkit enabled App with at least one addon
2. open up Error Console
3. add the AddonListeners from the pastebin [1], they will listen for enabling / disabling
4. disable / enable an addon
5. notice the "foo says ...", "bar says ..." and "foobar says ..." messages

Part 2:
6. restart App to cleanup 
7. open Error Console
8. uncomment the removeAddonListener() related lines in listenerFoo.onDisabling()
9. evaluate this modified code in Error Console
10. disable / enable an addon
11. notice the missing "... disabling!" message from the second, listenerBar listner
12. disable an addon again
13. listenerFoo is now removed, so it doesn't modifies the Array while being in the loop, so all the listeners are notified



Actual results:
Second listener doesn't fire.




Expected results:
Second listener should fire.




[1] - http://pastebin.mozilla.org/1648327
The STR is for AMI_callAddonListeners, but AMI_callInstallListeners in rare cases 
could run into unsafe path too.
Summary: AMI_callInstallListeners (in AddonManager.jsm) informs it's listeners unsafely, causing silent skips in the notification loop → AMI_callAddonListeners (in AddonManager.jsm) informs it's listeners unsafely, causing silent skips in the notification loop
Hm, indeed - the array is changed while it's being iterated over. It really needs to take a snapshot of the array, and iterate over that. callManagerListeners has the same issue, as does callInstallListeners when aExtraListeners isn't passed in. I wouldn't expect typeListeners to be a problem, but better to be on the safe side.
Summary: AMI_callAddonListeners (in AddonManager.jsm) informs it's listeners unsafely, causing silent skips in the notification loop → AddonManagerInternal.callAddonlListeners() informs it's listeners unsafely, causing silent skips in the notification loop
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
No longer depends on: 756960
(In reply to Blair McBride (:Unfocused) from comment #2)
> Hm, indeed - the array is changed while it's being iterated over. It really
> needs to take a snapshot of the array, and iterate over that.

Would You mind to check the iterating of AddonManagerInternal.providers[] too?
callProvider() is feeded by providers[]' elements (by forEach loop), 
but IMO it isn't enough.

Thanks!
Attached patch Patch v1 (obsolete) — Splinter Review
This ends up making the code read a bit cleaner too, as a nice bonus.

Being a bit paranoid in inInstallEnabled etc, but just because you're paranoid doesn't mean they're not out to get you.
Attachment #628201 - Flags: review?(geoff)
Attachment #628201 - Flags: review?(dtownsend+bugmail)
Attached patch Patch v1.1Splinter Review
Adds a parameter guard to callProviders (bug 746908), and uses the new rest-args JS feature (bug 759446).
Attachment #628201 - Attachment is obsolete: true
Attachment #628201 - Flags: review?(geoff)
Attachment #628201 - Flags: review?(dtownsend+bugmail)
Attachment #628208 - Flags: review?(geoff)
Attachment #628208 - Flags: review?(dtownsend+bugmail)
Filed bug 759702 to check XPIProvider for this type of issue.
Attachment #628208 - Flags: review?(dtownsend+bugmail) → review+
Attachment #628208 - Flags: review?(geoff) → review+
https://hg.mozilla.org/integration/fx-team/rev/8cb94b792009
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla15
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.