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)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 4.0b2

People

(Reporter: sgautherie, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

patch v3
29.00 KB, patch
Gavin
: review+
Details | Diff | Splinter Review 
      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 v3
        — 
          — Splinter 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: 14 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: