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

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
Tabbed Browser
--
minor
RESOLVED FIXED
13 years ago
7 years ago

People

(Reporter: timeless, Assigned: sgautherie)

Tracking

({fixed-seamonkey2.0})

Trunk
seamonkey2.1a1
fixed-seamonkey2.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

13 years ago
 

Comment 1

13 years ago
Created attachment 156038 [details] [diff] [review]
Patch

Comment 2

13 years ago
Created attachment 156039 [details] [diff] [review]
Patch 2
Attachment #156038 - Attachment is obsolete: true
(Reporter)

Updated

13 years ago
Attachment #156039 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 3

13 years ago
Hmm... won't this get confused if a progress listener removes itself?

Comment 4

13 years ago
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

Comment 5

8 years ago
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

Comment 6

8 years ago
FWIW, a similar change was made in Firefox in bug 303075.
Ever confirmed: false
(Assignee)

Comment 7

8 years ago
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...
Attachment #156039 - Attachment is obsolete: true
Attachment #402225 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Severity: normal → minor
Depends on: 303075
OS: Windows XP → All
QA Contact: tabbed-browser
Hardware: x86 → All

Comment 8

8 years ago
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-
(Assignee)

Comment 9

8 years ago
(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.
(Assignee)

Comment 11

8 years ago
(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.)
(Assignee)

Comment 13

8 years ago
Created attachment 402538 [details] [diff] [review]
(Bv1) "Clone" the array and add/remove to the front, Iterate the array backward

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

Comment 15

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

Comment 17

8 years ago
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...
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.
(Assignee)

Comment 19

8 years ago
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).
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+
(Assignee)

Comment 21

8 years ago
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?

Updated

8 years ago
Attachment #403080 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
(Assignee)

Updated

8 years ago
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]
(Assignee)

Comment 22

8 years ago
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).
(Assignee)

Comment 23

8 years ago
And this started as a "one liner" port :-]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: fixed-seamonkey2.0
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
(Assignee)

Updated

8 years ago
Blocks: 519216
(Assignee)

Comment 24

8 years ago
Created attachment 408168 [details] [diff] [review]
(Cv1) Enable argument checks on m-1.9.2+
[Checkin: Comment 25]
Attachment #408168 - Flags: superreview?(neil)
Attachment #408168 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Blocks: 523820

Updated

8 years ago
Attachment #408168 - Flags: superreview?(neil)
Attachment #408168 - Flags: superreview+
Attachment #408168 - Flags: review?(neil)
Attachment #408168 - Flags: review+
(Assignee)

Comment 25

8 years ago
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]
(Assignee)

Updated

8 years ago
Target Milestone: seamonkey2.0 → seamonkey2.1a1

Updated

7 years ago
No longer blocks: 519216
(Assignee)

Comment 26

7 years ago
(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.