Closed Bug 756960 Opened 10 years ago Closed 10 years ago

Downloader_onStopRequest (in nsUpdateService.js) informs it's listeners unsafely, causing this._listeners[i] is undefined


(Toolkit :: Application Update, defect)

Not set





(Reporter: xabolcs, Assigned: xabolcs)




(1 file, 1 obsolete file)

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

Steps to reproduce:

1. got an outdated Firefox/Aurora/Nightly
2. set app.update.log to true, and restart App
3. check for update and download it
4. open Error Console
5. add the downloadlistener below while downloading the update (hurry if You have a fast connection)

log: function(aMessage) {Components.utils.reportError(aMessage);dump(aMessage + "\n");},
onStartRequest: function(aRequest, aContext) {},
onStopRequest: function(aRequest, aContext, aStatusCode) {this.log("Hello from Console!");},
onStatus: function(aRequest, aContext, aStatus, aStatusArg) {},
onProgress: function(aRequest, aContext, aProgress, aProgressMax) {},

6. wait for notification

Actual results:

An error message - "this._listeners[i] is undefined" - logged into Error Console pointing to Downloader_onStopRequest [1].

[1] -

Expected results:

The download listener should fired - "Hello from Console!".
OS: Windows 7 → All
Hardware: x86_64 → All
Component: General → Application Update
Product: Core → Toolkit
QA Contact: general → application.update
Assignee: nobody → xabolcs
Trivial patch. Using Array.concat() to create a shallow copy.
Attachment #625727 - Flags: review?(dtownsend+bugmail)
I assume the issue is that listeners can remove themselves during the onStopRequest call. Could you add similar protections to the other cases like this?
Yes, that's the problem.

OK. I will look through nsUpdateService.js for other unsafe Array iterations
and provide a list before starting hacking.
I found that only Downloader() needed these protections. 
So this patch covers it's nsIRequestObserver.idl and nsIProgressEventSink.idl notifications.
Attachment #625727 - Attachment is obsolete: true
Attachment #625727 - Flags: review?(dtownsend+bugmail)
Attachment #626150 - Flags: review?(dtownsend+bugmail)
Blocks: 757663
Blocks: 657704
No longer blocks: 757663
See Also: → 757663
Comment on attachment 626150 [details] [diff] [review]
patch v2 - Iterate on shallow copies

Looks good to me
Attachment #626150 - Flags: review?(dtownsend+bugmail) → review+
Keywords: checkin-needed
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
You could have also just used _listeners.forEach, right?
(In reply to :Gavin Sharp (use for email) from comment #7)
> You could have also just used _listeners.forEach, right?

No, iterating on _listeners won't work! See related Bug 757663!
AddonManager.jsm uses forEach on it's listeners, but still fails.

By the way I could have migrate these for loops into forEach, 
but in nsUpdateService.js there are other sections which also uses for.
And I wouldn't like to convert them too in this bug.
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to Szabolcs Hubai from comment #8)
> No, iterating on _listeners won't work! See related Bug 757663!
> AddonManager.jsm uses forEach on it's listeners, but still fails.

You're right - the forEach documentation on this is somewhat misleading (it says "The range of elements processed by forEach is set before the first invocation of callback" and "elements that are deleted are not visited" - but by "elements" it means "indices", not values).
You need to log in before you can comment on or make changes to this bug.