Closed Bug 255503 Opened 16 years ago Closed 11 years ago

tabbedbrowser progresslistener list grows forever instead of resizing when removeProgressListener is called

Categories

(SeaMonkey :: Tabbed Browser, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a1

People

(Reporter: timeless, Assigned: sgautherie)

References

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(2 files, 5 obsolete files)

 
Attached patch Patch (obsolete) — Splinter Review
Attached patch Patch 2 (obsolete) — Splinter Review
Attachment #156038 - Attachment is obsolete: true
Attachment #156039 - Flags: superreview?(neil.parkwaycc.co.uk)
Hmm... won't this get confused if a progress listener removes itself?
Comment on attachment 156039 [details] [diff] [review]
Patch 2

sr- as discussed on IRC
Attachment #156039 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Product: Core → SeaMonkey
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
Status: NEW → UNCONFIRMED
FWIW, a similar change was made in Firefox in bug 303075.
Ever confirmed: false
Attached patch (Av3) Port bug 303075 (obsolete) — Splinter Review
Minimal fix.

Though I don't know what was wrong with the previous patch:
see your comment 3 and comment 4...
Attachment #156039 - Attachment is obsolete: true
Attachment #402225 - Flags: review?(neil)
Severity: normal → minor
Depends on: 303075
OS: Windows XP → All
QA Contact: tabbed-browser
Hardware: x86 → All
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.)
Attachment #402225 - Flags: review?(neil) → review-
(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?
Assignee: bugzilla → sgautherie.bz
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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.
(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...)
(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.)
Per previous discussion.
Attachment #402225 - Attachment is obsolete: true
Attachment #402538 - Flags: review?(neil)
(I haven't read the patch yet but if you clone it properly you don't have to reverse the iteration.)
(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 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?
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...
Attachment #402538 - Attachment is obsolete: true
Attachment #402878 - Flags: review?(neil)
Attachment #402538 - Flags: review?(neil)
(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.
Bv1a, with comment 18 suggestion(s).
Attachment #402878 - Attachment is obsolete: true
Attachment #403080 - Flags: review?(neil)
Attachment #402878 - Flags: review?(neil)
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 ;-)
Attachment #403080 - Flags: review?(neil) → review+
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.
Attachment #403080 - Flags: approval-seamonkey2.0?
Attachment #403080 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
Attachment #403080 - Attachment description: (Bv2) Use .forEach() to iterate and .filter() to remove → (Bv2) Use .forEach() to iterate and .filter() to remove [Checkin: See comment 22]
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).
And this started as a "one liner" port :-]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
Blocks: 519216
Blocks: 523820
Attachment #408168 - Flags: superreview?(neil)
Attachment #408168 - Flags: superreview+
Attachment #408168 - Flags: review?(neil)
Attachment #408168 - Flags: review+
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
Attachment #408168 - Attachment description: (Cv1) Enable argument checks on m-1.9.2+ → (Cv1) Enable argument checks on m-1.9.2+ [Checkin: Comment 25]
Target Milestone: seamonkey2.0 → seamonkey2.1a1
No longer blocks: 519216
(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?
No, I think we're done here.
You need to log in before you can comment on or make changes to this bug.