212271 - [FIXr] more typesafety and such with arrays in nsScriptLoader

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Fixes a leak in the destructor if mPendingRequests is nonempty too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: patch

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 127425 [details] [diff] [review]
patch

- In nsScriptLoader::FireErrorNotification() and others:

   for (i = 0; i < count; i++) {
-    nsCOMPtr<nsISupports> sup(dont_AddRef(mObservers.ElementAt(i)));
-    nsCOMPtr<nsIScriptLoaderObserver> observer(do_QueryInterface(sup));
+    nsIScriptLoaderObserver* observer = mObservers[i];

     if (observer) {
       observer->ScriptAvailable(...);
     }
   }

I'd feel more comfortable with this if |observer| remained a strong references,
written like this, we're likely to crash if an observer removes itself as an
observer from one of these callbacks due to the lack of a strong refence that's
guaranteed to last the length of the observer call.

sr=jst

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 127425 [details] [diff] [review]
patch

> nsScriptLoader::~nsScriptLoader()
> {
>   mObservers.Clear();
>   
>-  PRUint32 i, count;
>-  mPendingRequests.Count(&count);
>+  PRInt32 i, count;
>+  count = mPendingRequests.Count();

Could you maybe assign into the declaration for both of these PRInt32s?  |i|
could be declared in the for() loop...

>   for (i = 0; i < count; i++) {


- In nsScriptLoader::ProcessScriptElement()

>     // If we've got existing pending requests, add ourselves
>     // to this list.
>-    if (pendingRequestCount) {
>+    if (mPendingRequests.Count()) {

Count() returns a PRInt32; should this check Count() > 0?


Same two comments apply in FireErrorNotification(), FireScriptAvailable(),
FireScriptEvaluated(), and ProcessPendingReqests() twice.  (I notice that you
_did_ use Count() > 1 for your assert in OnStreamComplete()).

r=caillon with nits addressed.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 127425 [details] [diff] [review]
patch

> nsScriptLoader::~nsScriptLoader()
> {
>   mObservers.Clear();
>   
>-  PRUint32 i, count;
>-  mPendingRequests.Count(&count);
>+  PRInt32 i, count;
>+  count = mPendingRequests.Count();

Could you maybe assign into the declaration for both of these PRInt32s?  |i|
could be declared in the for() loop...

>   for (i = 0; i < count; i++) {


- In nsScriptLoader::ProcessScriptElement()

>     // If we've got existing pending requests, add ourselves
>     // to this list.
>-    if (pendingRequestCount) {
>+    if (mPendingRequests.Count()) {

Count() returns a PRInt32; should this check Count() > 0?


Same two comments apply in FireErrorNotification(), FireScriptAvailable(),
FireScriptEvaluated(), and ProcessPendingReqests() twice.  (I notice that you
_did_ use Count() > 0 for your assert in OnStreamComplete()).

r=caillon with nits addressed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: updated to comments

Comment 6

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.4b.

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 96141 has been marked as a duplicate of this bug. ***