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

RESOLVED FIXED in mozilla15

Status

()

Toolkit
Application Update
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: xabolcs, Assigned: xabolcs)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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!".
(Assignee)

Updated

6 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Updated

6 years ago
Component: General → Application Update
Product: Core → Toolkit
QA Contact: general → application.update
(Assignee)

Updated

6 years ago
Assignee: nobody → xabolcs
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 625727 [details] [diff] [review]
patch v1 - Iterate on a shallow copy

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?
(Assignee)

Comment 3

6 years ago
Yes, that's the problem.

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

Comment 4

6 years ago
Created attachment 626150 [details] [diff] [review]
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.
Attachment #625727 - Attachment is obsolete: true
Attachment #625727 - Flags: review?(dtownsend+bugmail)
Attachment #626150 - Flags: review?(dtownsend+bugmail)
(Assignee)

Updated

6 years ago
Blocks: 757663
(Assignee)

Updated

6 years ago
Blocks: 657704
No longer blocks: 757663
See Also: → bug 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+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0872c9ca51e1
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
You could have also just used _listeners.forEach, right?
(Assignee)

Comment 8

6 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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

6 years ago
https://hg.mozilla.org/mozilla-central/rev/0872c9ca51e1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.