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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Szabolcs Hubai (:xabolcs)]

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)


Components.classes["@mozilla.org/updates/update-service;1"]
.getService(Components.interfaces.nsIApplicationUpdateService)
.addDownloadListener({
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] - http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2943


Expected results:

The download listener should fired - "Hello from Console!".

Comment 1

[Szabolcs Hubai (:xabolcs)]

Attachment: patch v1 - Iterate on a shallow copy

Trivial patch. Using Array.concat() to create a shallow copy.

Comment 2

[Dave Townsend [:mossop]]

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?

Comment 3

[Szabolcs Hubai (:xabolcs)]

Yes, that's the problem.

OK. I will look through nsUpdateService.js for other unsafe Array iterations
and provide a list before starting hacking.

Comment 4

[Szabolcs Hubai (:xabolcs)]

Attachment: patch v2 - Iterate on shallow copies

I found that only Downloader() needed these protections. 
So this patch covers it's nsIRequestObserver.idl and nsIProgressEventSink.idl notifications.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 626150 [details] [diff] [review]
patch v2 - Iterate on shallow copies

Looks good to me

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0872c9ca51e1

Comment 7

[:Gavin Sharp [email: [e-mail]]]

You could have also just used _listeners.forEach, right?

Comment 8

[Szabolcs Hubai (:xabolcs)]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0872c9ca51e1

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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