Closed Bug 519216 Opened 10 years ago Closed 9 years ago

Removing a progress listener while it's being called affects subsequent listeners

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b2

People

(Reporter: sgautherie, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

No description provided.
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.
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: 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
Blocks: 303075
No longer depends on: 255503, 303075
Keywords: regression
See Also: → 255503
Attached patch patch (obsolete) — Splinter Review
Attachment #438721 - Flags: review?(gavin.sharp)
Attached patch patch (obsolete) — Splinter Review
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)
(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
The others are tabs progress listeners, where the first argument is expected to be a browser.
Attached patch patch (obsolete) — Splinter Review
fixed a typo
Attachment #438738 - Attachment is obsolete: true
Attachment #438794 - Flags: review?(gavin.sharp)
Attachment #438738 - Flags: review?(gavin.sharp)
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 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;
+                }
+              );
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #438794 - Attachment is obsolete: true
Attachment #439219 - Flags: review?(gavin.sharp)
Attachment #438794 - Flags: review?(gavin.sharp)
(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.
(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)
Depends on: 560512
No longer depends on: 560512
Attached patch patch v3Splinter Review
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)
Blocks: 573534
Gavin: ping? Any problems with this patch? It's pretty straightforward, I think.
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+
http://hg.mozilla.org/mozilla-central/rev/1e70fba7e663
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7b2
Blocks: 577320
Blocks: 577939
You need to log in before you can comment on or make changes to this bug.