Closed
Bug 255503
Opened 20 years ago
Closed 15 years ago
tabbedbrowser progresslistener list grows forever instead of resizing when removeProgressListener is called
Categories
(SeaMonkey :: Tabbed Browser, defect)
SeaMonkey
Tabbed Browser
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a1
People
(Reporter: timeless, Assigned: sgautherie)
References
Details
(Keywords: fixed-seamonkey2.0)
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
neil
:
review+
kairo
:
approval-seamonkey2.0+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
Comment 1•20 years ago
|
||
Comment 2•20 years ago
|
||
Attachment #156038 -
Attachment is obsolete: true
Attachment #156039 -
Flags: superreview?(neil.parkwaycc.co.uk)
Comment 3•20 years ago
|
||
Hmm... won't this get confused if a progress listener removes itself?
Comment 4•20 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-
Updated•16 years ago
|
Product: Core → SeaMonkey
Comment 5•15 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•15 years ago
|
||
FWIW, a similar change was made in Firefox in bug 303075.
Ever confirmed: false
Assignee | ||
Comment 7•15 years ago
|
||
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•15 years ago
|
Severity: normal → minor
Depends on: 303075
OS: Windows XP → All
QA Contact: tabbed-browser
Hardware: x86 → All
Comment 8•15 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•15 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
Comment 10•15 years ago
|
||
(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•15 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...)
Comment 12•15 years ago
|
||
(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•15 years ago
|
||
Per previous discussion.
Attachment #402225 -
Attachment is obsolete: true
Attachment #402538 -
Flags: review?(neil)
Comment 14•15 years ago
|
||
(I haven't read the patch yet but if you clone it properly you don't have to reverse the iteration.)
Assignee | ||
Comment 15•15 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 16•15 years ago
|
||
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•15 years ago
|
||
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)
Comment 18•15 years ago
|
||
(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•15 years ago
|
||
Bv1a, with comment 18 suggestion(s).
Attachment #402878 -
Attachment is obsolete: true
Attachment #403080 -
Flags: review?(neil)
Attachment #402878 -
Flags: review?(neil)
Comment 20•15 years ago
|
||
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•15 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•15 years ago
|
Attachment #403080 -
Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
Assignee | ||
Updated•15 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•15 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•15 years ago
|
||
And this started as a "one liner" port :-]
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed-seamonkey2.0
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
Assignee | ||
Comment 24•15 years ago
|
||
Attachment #408168 -
Flags: superreview?(neil)
Attachment #408168 -
Flags: review?(neil)
Updated•15 years ago
|
Attachment #408168 -
Flags: superreview?(neil)
Attachment #408168 -
Flags: superreview+
Attachment #408168 -
Flags: review?(neil)
Attachment #408168 -
Flags: review+
Assignee | ||
Comment 25•15 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•15 years ago
|
Target Milestone: seamonkey2.0 → seamonkey2.1a1
Assignee | ||
Comment 26•15 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?
Comment 27•15 years ago
|
||
No, I think we're done here.
You need to log in
before you can comment on or make changes to this bug.
Description
•