Closed
Bug 519216
Opened 15 years ago
Closed 14 years ago
Removing a progress listener while it's being called affects subsequent listeners
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 4.0b2
People
(Reporter: sgautherie, Assigned: dao)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
29.00 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•14 years ago
|
||
I happened to notice last night that the 'list grows forever' part was fixed in bug 303075. This either means (a) That removing a listener inside a listener is broken and should be fixed [eg removing itself will skip the next listener] or (b) I can remove all those |if (p)| fragments now as they are not doing anything useful. The same also applies to mTabsProgressListeners of course.
Assignee | ||
Comment 2•14 years ago
|
||
Yes, removing a listener inside a listener is likely broken, which means we should port attachment 403080 [details] [diff] [review].
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dao
Severity: minor → normal
Summary: Port |Bug 255503 - tabbedbrowser progresslistener list grows forever instead of resizing when removeProgressListener is called| to Firefox → Removing a progress listener while it's being called affects subsequent listeners
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #438721 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 4•14 years ago
|
||
fixed the onLinkIconAvailable handler in browser.js -- it's not a "tabs progress listener", so passing the browser doesn't make sense.
Attachment #438721 -
Attachment is obsolete: true
Attachment #438738 -
Flags: review?(gavin.sharp)
Attachment #438721 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 5•14 years ago
|
||
(In reply to comment #4) > fixed the onLinkIconAvailable handler in browser.js -- it's not a "tabs > progress listener", so passing the browser doesn't make sense. Could you comment on the other cases too: http://mxr.mozilla.org/comm-central/search?string=onLinkIconAvailable&case=on while SM seems to use an url only (maybe pending a sync' with FF !?), FF uses either both or a browser only, never an url only yet...
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•14 years ago
|
||
The others are tabs progress listeners, where the first argument is expected to be a browser.
Assignee | ||
Comment 7•14 years ago
|
||
fixed a typo
Attachment #438738 -
Attachment is obsolete: true
Attachment #438794 -
Flags: review?(gavin.sharp)
Attachment #438738 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 8•14 years ago
|
||
Comment on attachment 438794 [details] [diff] [review] patch >+ gBrowser.removeProgressListener(this); >+ gBrowser.stop(); >+ finish(); I need to do addTab & removeCurrentTab here, as some test expects the tab to be blank. Passed the tryserver with that.
Comment 9•14 years ago
|
||
Comment on attachment 438794 [details] [diff] [review] patch >+ this.mProgressListeners.slice().forEach(function (p) { Can you avoid copying the array with slice() here, >+ this.mTabsProgressListeners.slice().forEach(function (p) { and here, by ensuring that a new array is created on every addProgressListener and removeProgressListener instead (similar to the SeaMonkey patch): - 1681 this.mProgressListeners.push(aListener); + // Create a new array, not to disturb possibly ongoing iterations. + this.mProgressListeners = this.mProgressListeners.concat(aListener); - 1706 for (var i = 0; i < this.mProgressListeners.length; i++) { - 1707 if (this.mProgressListeners[i] == aListener) { - 1708 this.mProgressListeners.splice(i, 1); - 1709 break; - 1710 } - 1711 } + // Create a new array, not to disturb possibly ongoing iterations. + this.mProgressListeners = + this.mProgressListeners.filter( + function removeListener(element) { + return element != aListener; + } + ); 1720 <method name="addTabsProgressListener"> 1721 <parameter name="aListener"/> 1722 <body> 1723 this.enterTabbedMode(); - this.mTabsProgressListeners.push(aListener); + // Create a new array, not to disturb possibly ongoing iterations. + this.mTabsProgressListeners = this.mTabsProgressListeners.concat(aListener); 1725 </body> 1726 </method> - 1732 var pos = this.mTabsProgressListeners.indexOf(aListener); - 1733 if (pos >= 0) - 1734 this.mTabsProgressListeners.splice(pos, 1); + // Create a new array, not to disturb possibly ongoing iterations. + this.mTabsProgressListeners = + this.mTabsProgressListeners.filter( + function removeListener(element) { + return element != aListener; + } + );
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #438794 -
Attachment is obsolete: true
Attachment #439219 -
Flags: review?(gavin.sharp)
Attachment #438794 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #9) Pushing new listeners doesn't seem to be a problem (I couldn't get the test to fail), so I didn't change this.
Comment 12•14 years ago
|
||
(In reply to comment #11) > (In reply to comment #9) > Pushing new listeners doesn't seem to be a problem (I couldn't get the test to > fail), so I didn't change this. Yes, sorry I should have checked. javascript:a=[1,2,3];a.forEach(function(l) {alert(l);a.push(7)});alert(a)
Assignee | ||
Comment 13•14 years ago
|
||
made _callProgressListeners' browser argument optional and eliminated it in mTabProgressListener
Attachment #439219 -
Attachment is obsolete: true
Attachment #442977 -
Flags: review?(gavin.sharp)
Attachment #439219 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 14•14 years ago
|
||
Gavin: ping? Any problems with this patch? It's pretty straightforward, I think.
Comment 15•14 years ago
|
||
Comment on attachment 442977 [details] [diff] [review] patch v3 The updateCurrentBrowser changes have potential to break things that depend on the relative ordering of calls to the three progress listener methods, but those are probably pretty unlikely to exist in practice. Similarly, behavior will change for listeners that change the selected tab (given the use of tabContainer.selectedIndex within the loop), but that's already kind of broken and this new behavior isn't any worse, I think.
Attachment #442977 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 16•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/1e70fba7e663
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7b2
You need to log in
before you can comment on or make changes to this bug.
Description
•