Last Comment Bug 255503 - tabbedbrowser progresslistener list grows forever instead of resizing when removeProgressListener is called
: tabbedbrowser progresslistener list grows forever instead of resizing when re...
Status: RESOLVED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- minor (vote)
: seamonkey2.1a1
Assigned To: Serge Gautherie (:sgautherie)
:
Mentors:
Depends on: 303075
Blocks: 523820
  Show dependency treegraph
 
Reported: 2004-08-13 05:29 PDT by timeless
Modified: 2010-04-20 16:03 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (1.19 KB, patch)
2004-08-13 05:43 PDT, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
Patch 2 (1.18 KB, patch)
2004-08-13 05:47 PDT, Frank Wein [:mcsmurf]
neil: superreview-
Details | Diff | Splinter Review
(Av3) Port bug 303075 (993 bytes, patch)
2009-09-22 16:56 PDT, Serge Gautherie (:sgautherie)
neil: review-
Details | Diff | Splinter Review
(Bv1) "Clone" the array and add/remove to the front, Iterate the array backward (15.80 KB, patch)
2009-09-23 22:31 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Bv1a) "Clone" the array and add to the front, Iterate the array backward (15.64 KB, patch)
2009-09-25 11:32 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Bv2) Use .forEach() to iterate and .filter() to remove [Checkin: See comment 22] (17.10 KB, patch)
2009-09-26 20:38 PDT, Serge Gautherie (:sgautherie)
neil: review+
kairo: approval‑seamonkey2.0+
Details | Diff | Splinter Review
(Cv1) Enable argument checks on m-1.9.2+ [Checkin: Comment 25] (1.58 KB, patch)
2009-10-23 19:40 PDT, Serge Gautherie (:sgautherie)
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description timeless 2004-08-13 05:29:42 PDT
 
Comment 1 Frank Wein [:mcsmurf] 2004-08-13 05:43:46 PDT
Created attachment 156038 [details] [diff] [review]
Patch
Comment 2 Frank Wein [:mcsmurf] 2004-08-13 05:47:16 PDT
Created attachment 156039 [details] [diff] [review]
Patch 2
Comment 3 neil@parkwaycc.co.uk 2004-08-13 07:32:10 PDT
Hmm... won't this get confused if a progress listener removes itself?
Comment 4 neil@parkwaycc.co.uk 2004-08-13 08:34:12 PDT
Comment on attachment 156039 [details] [diff] [review]
Patch 2

sr- as discussed on IRC
Comment 5 Robert Kaiser 2009-06-14 17:54:46 PDT
MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614
Comment 6 Nickolay_Ponomarev 2009-09-13 14:29:00 PDT
FWIW, a similar change was made in Firefox in bug 303075.
Comment 7 Serge Gautherie (:sgautherie) 2009-09-22 16:56:22 PDT
Created attachment 402225 [details] [diff] [review]
(Av3) Port bug 303075

Minimal fix.

Though I don't know what was wrong with the previous patch:
see your comment 3 and comment 4...
Comment 8 neil@parkwaycc.co.uk 2009-09-23 05:47:50 PDT
Comment on attachment 402225 [details] [diff] [review]
(Av3) Port bug 303075

>+                this.mProgressListener.splice(i, 1);
It depends on when the progresslistener is removed. If the tabbed browser is processing a progress notification then removing a progresslistener will upset the notification loop. Compare nsDocLoader::FireOnLocationChange (which isn't the only way to do it, but it's one way.)
Comment 9 Serge Gautherie (:sgautherie) 2009-09-23 07:03:54 PDT
(In reply to comment #8)

Ah. Then, here is how I understand it:

1) http://mxr.mozilla.org/comm-central/search?string=mProgressListeners&case=on&find=%2Ftabbrowser.xml%24

Frank's patch was indeed right at iterating backward.
But that needs to be done for all the other loops too, not this one only.

2) https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for...in#Description

We should also replace our
{
line 1069 -- for each (var p in this.mProgressListeners) {
line 1102 -- for each (var p in this.mProgressListeners) {
}
by |for (... this.mProgressListeners.length ...)|, like all the other loops.

3) While there,

I think we can remove all the |if (p)| checks too, now that we remove the |null| values in the array.

Am I right?
Comment 10 neil@parkwaycc.co.uk 2009-09-23 07:25:42 PDT
(In reply to comment #9)
> Frank's patch was indeed right at iterating backward.
> But that needs to be done for all the other loops too, not this one only.
Of course. (I wonder whether we should add listeners in reverse order.)

> We should also replace our
> {
> line 1069 -- for each (var p in this.mProgressListeners) {
> line 1102 -- for each (var p in this.mProgressListeners) {
> }
> by |for (... this.mProgressListeners.length ...)|, like all the other loops.
Good point. (But then, you had to do that anyway!)

> I think we can remove all the |if (p)| checks too, now that we remove the
> |null| values in the array.
Good idea.
Comment 11 Serge Gautherie (:sgautherie) 2009-09-23 12:16:09 PDT
(In reply to comment #10)

https://developer.mozilla.org/en/Core_JavaScript_1.5_Guide/Iterators_and_Generators

Actually, all syntaxes seem to fail to handle (additions and) removals seamlessly:
they can all potentially skip "1+" listener (or call it "twice" or when it should not be yet)... :-(
Try, for example:
var langs = ['0', '1',' 2', '3', '4', '5'];  var it = Iterator(langs);  for (var [i, lang] in it) {  if (i % 2 == 0) langs.splice(i, 1); else langs.push('added_'+i); alert(i + ': ' + lang);  }

1st solution would be to use some kind of lock (everywhere),
but I don't know how and I guess it might cause a deadlock?
I assume .push() and .splice() are safe by themselves. (Or do they actually need a lock anyway?)

2nd solution would be to clone the array before iterating it:
it would ignore additions, which is actually wanted,
it would ignore removals, which may (not) be a problem?

NB:
http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#1326
might (not) have this issue too? (I don't know that code...)
Comment 12 neil@parkwaycc.co.uk 2009-09-23 13:21:06 PDT
(In reply to comment #11)
> 1st solution would be to use some kind of lock (everywhere),
That doesn't make sense. Locks fix threadsafety, not reentrance.

> 2nd solution would be to clone the array before iterating it:
That seems wasteful, but what you could do is to make a local reference to the array, and clone the array before mutating it (so the old local array reference is the one that's still being iterated, not the new clone).

> it would ignore removals, which may (not) be a problem?
If it is, we could check to see whether the array had been cloned (because the field had changed from the local reference) and if so whether the listener was still in the array (which would indicate whether or not it had been removed).

> NB:
> http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#1326
> might (not) have this issue too? (I don't know that code...)
It removes the null elements at the end of each loop. (It has to do this because it keeps weak references so one of the elements might become null without being deliberately removed.)
Comment 13 Serge Gautherie (:sgautherie) 2009-09-23 22:31:56 PDT
Created attachment 402538 [details] [diff] [review]
(Bv1) "Clone" the array and add/remove to the front, Iterate the array backward

Per previous discussion.
Comment 14 neil@parkwaycc.co.uk 2009-09-24 01:28:16 PDT
(I haven't read the patch yet but if you clone it properly you don't have to reverse the iteration.)
Comment 15 Serge Gautherie (:sgautherie) 2009-09-24 09:10:43 PDT
(In reply to comment #14)
> (if you clone it properly you don't have to reverse the iteration.)

Right: I'm simply assuming backward iteration (code) is faster.
If you want to keep forward iterations, I could either keep current "for( .length )" code (minimal change) or use "for each ( Iterator() }" (newer syntax, though no idea if slower/faster).
Comment 16 neil@parkwaycc.co.uk 2009-09-25 09:50:25 PDT
Comment on attachment 402538 [details] [diff] [review]
(Bv1) "Clone" the array and add/remove to the front, Iterate the array backward

>+                  let p = pl[i];
>+                  try {
>+                    p.onProgressChange(aWebProgress, aRequest,
>+                                       aCurSelfProgress, aMaxSelfProgress,
>+                                       aCurTotalProgress, aMaxTotalProgress);
We used to need a temporary p because we had to check to see if it was null, but in most of these cases you're only using it once.

Maybe we should use .forEach on the array; then we wouldn't need a local for that either :-)

>+            // Ignore unsupported values.
>+            if (!aListener)
>+              return;
We'll never find a null listener anyway ;-)

>+                  function keepElement(element, index, array) {
>+                    return index != i;
Why not filter on element != aListener?
Comment 17 Serge Gautherie (:sgautherie) 2009-09-25 11:32:04 PDT
Created attachment 402878 [details] [diff] [review]
(Bv1a) "Clone" the array and add to the front, Iterate the array backward

Bv1, with comment 16 suggestion(s).

(In reply to comment #16)

> Maybe we should use .forEach on the array; then we wouldn't need a local for
> that either :-)

I considered it, but I'm loath to check and make the changes to use the callback syntax.

> We'll never find a null listener anyway ;-)
+
> Why not filter on element != aListener?

Here's a version that is more like
http://mxr.mozilla.org/comm-central/source/mozilla/uriloader/base/nsIWebProgress.idl#134
Maybe that's unwanted, but it's easy to discuss...
Comment 18 neil@parkwaycc.co.uk 2009-09-25 15:46:09 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > Maybe we should use .forEach on the array; then we wouldn't need a local for
> > that either :-)
> I considered it, but I'm loath to check and make the changes to use the
> callback syntax.
Well, you could at least append and iterate forwards, so that we could switch more easily in future...

> Here's a version that is more like
> http://mxr.mozilla.org/comm-central/source/mozilla/uriloader/base/nsIWebProgress.idl#134
> Maybe that's unwanted, but it's easy to discuss...
Assuming you don't want this in 2.0 then if it's a problem with extensions we'll have time to look at other possibilities.
Comment 19 Serge Gautherie (:sgautherie) 2009-09-26 20:38:21 PDT
Created attachment 403080 [details] [diff] [review]
(Bv2) Use .forEach() to iterate and .filter() to remove
[Checkin: See comment 22]

Bv1a, with comment 18 suggestion(s).
Comment 20 neil@parkwaycc.co.uk 2009-09-27 14:12:38 PDT
Comment on attachment 403080 [details] [diff] [review]
(Bv2) Use .forEach() to iterate and .filter() to remove
[Checkin: See comment 22]

>+                function oProgressC(element, index, array) {
Sorry, but I don't like these weird function names. If you don't like anonymous functions then something simple like function notifyChange(p) is fine. (You also don't need to bother with the optional arguments.) sr=me with those fixed.

>+                      function uCBbaroFA(feedElement, feedIndex, feedArray) {
So this would become function notifyFeed(feed) { p.onFeedAvailable(feed); }

>+            // push() does not disturb possibly ongoing iterations.
Isn't that a pleasant surprise :-) [I double-checked with MDC too.]

>+                function keepElement(element, index, array) {
At least it's not as bad as those previous names, and I know that the description of the function is that it returns true for the elements that we want to keep, but then again it's the other elements that we're trying to keep here, not the parameter to removeProgressListener ;-)
Comment 21 Serge Gautherie (:sgautherie) 2009-09-27 16:25:27 PDT
Comment on attachment 403080 [details] [diff] [review]
(Bv2) Use .forEach() to iterate and .filter() to remove
[Checkin: See comment 22]

"approval-seamonkey2.0=?":
improve array(s) handling, low risk.
Comment 22 Serge Gautherie (:sgautherie) 2009-09-28 08:15:58 PDT
Comment on attachment 403080 [details] [diff] [review]
(Bv2) Use .forEach() to iterate and .filter() to remove
[Checkin: See comment 22]


http://hg.mozilla.org/comm-central/rev/abb1249c6e06
Bv2, with (revised) comment 20 suggestion(s).
Comment 23 Serge Gautherie (:sgautherie) 2009-09-28 08:21:15 PDT
And this started as a "one liner" port :-]
Comment 24 Serge Gautherie (:sgautherie) 2009-10-23 19:40:31 PDT
Created attachment 408168 [details] [diff] [review]
(Cv1) Enable argument checks on m-1.9.2+
[Checkin: Comment 25]
Comment 25 Serge Gautherie (:sgautherie) 2009-10-24 16:17:54 PDT
Comment on attachment 408168 [details] [diff] [review]
(Cv1) Enable argument checks on m-1.9.2+
[Checkin: Comment 25]


http://hg.mozilla.org/comm-central/rev/ea671fe1dd5f
Comment 26 Serge Gautherie (:sgautherie) 2010-04-20 08:44:13 PDT
(In reply to comment #16)
> >+            // Ignore unsupported values.
> >+            if (!aListener)
> >+              return;
> We'll never find a null listener anyway ;-)

Fwiw, new FF bug 560512 seems to disagree with that.
Neil, do you confirm you don't want this for SM 2.0?
Comment 27 neil@parkwaycc.co.uk 2010-04-20 16:03:36 PDT
No, I think we're done here.

Note You need to log in before you can comment on or make changes to this bug.