Closed Bug 599626 Opened 14 years ago Closed 14 years ago

After perform close group in Panorama, the browser becomes unresponsive

Categories

(Firefox Graveyard :: Panorama, defect, P1)

Product:
Firefox Graveyard
Component:
Panorama
Type:
defect

Tracking

(blocking2.0 betaN+)

Status:
RESOLVED FIXED
Milestone:
Firefox 4.0
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: alice0775, Assigned: raymondlee)

References

Details

(Keywords: dataloss, hang)

Attachments

(2 files, 7 obsolete files)

Testcase
160 bytes, text/html
Details
Patch for check-in
26.49 KB, patch
Details | Diff | Splinter Review 
Build Identifier: http://hg.mozilla.org/mozilla-central/rev/71e8b5aee972
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100925
Firefox/4.0b7pre ID:20100925041313

If there are tab which is include onbeforeunload(such as Google Document),
After perform close group in Panorama,
The browser does not response.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open Testcase
3. Open adequate number of web pages in new tabs.
4. Go Panorama
5. Divide tabs into two groups
6. Close a group which include the testcase
7. Choose a remaining group
8. When popup confirmation prompts, you chose "Stay on Page" 
9. Close tab and you chose "Leave on Page" 

Actual Results:
 Browser hangs.
 And the chosen group will be lost.

Expected Results:
 Browser should not hang.
 the chosen group should be there.
Attached file Testcase — 

    
    

    
  
Error console at Step 9:

Error: browser.webProgress is undefined
Source file: chrome://browser/content/tabbrowser.xml
Line: 1465

              // Remove the tab's filter and progress listener.
              const filter = this.mTabFilters[aTab._tPos];
>>            browser.webProgress.removeProgressListener(filter);
               filter.removeProgressListener(this.mTabListeners[aTab._tPos]);
              this.mTabListeners[aTab._tPos].destroy();



A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.

Script: chrome://browser/content/tabbrowser.xml:1495

        <method name="_endRemoveTab">
          <parameter name="aTab"/>
          <body>
            <![CDATA[
              if (!aTab || !aTab._endRemoveArgs)
>>              return;
  
              var [aCloseWindow, aNewTab] = aTab._endRemoveArgs;
Assignee: nobody → raymond
Blocks: 598154
Priority: -- → P1

    
  
Status: NEW → ASSIGNED

    
  
OS: Windows 7 → All
Hardware: x86 → All

    
    

    
  

      
      
      
      

        Attached patch
          
          
        wip v1 (obsolete)
        — 
          — Splinter Review
      

    


  
This patch breaks some tests and is missing a test as well

    
  
Summary: After perform close group in Panorama,The browser does not response, → After perform close group in Panorama, the browser becomes unresponsive

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #480946 -
        Attachment is obsolete: true

        Attachment #481297 -
        Flags: feedback?(ian)

    
  
Blocks: 597043
No longer blocks: 598154

    
    

    
  
(In reply to comment #4)
> Created attachment 481297 [details] [diff] [review]
> v1

Passed Try

    
    

    
  
Comment on attachment 481297 [details] [diff] [review]
v1

+  // Function: _showHiddenGroup
+  // Shows the hidden group.

I find this name a little confusing, like this group has a hidden group, rather than that this group is hidden. Maybe a better name would be just _unhide()?

+  // Function: removeHiddenGroup

This name is similarly confusing. How about closeHidden()?

I think this routine is more complicated than it needs to be.  Rather than doing the removeSubscriber/shouldRemoveTabItems thing, just close all the tabs and then check to see if the group has automatically closed itself. If it hasn't, check to see if it still has tabs in it. If it does, unhide it, otherwise close it.

You may need to modify GroupItem.close so it does the right thing in the hidden state, but that's better than having what's essentially a copy of it in this routine. 

+        GroupItems.removeHiddenGroups();

Why did you move this out of hideTabView? Seemed like a better spot for it.

   removeHiddenGroups: function GroupItems_removeHiddenGroups() {
-    iQ(".undo").remove();
-    
     // ToDo: encapsulate this in the group item. bug 594863

Looks like you've done that ToDo; remove the comment and make a note in the bug to resolve it once this lands.

 close: function TabItem_close() {
     gBrowser.removeTab(this.tab);
-    this._sendToSubscribers("tabRemoved");
+    let tabNotClosed = 
+      Array.some(gBrowser.tabs, function(tab) { return tab == this.tab; }, this);
+    if (!tabNotClosed)
+      this._sendToSubscribers("tabRemoved");
 
     // No need to explicitly delete the tab data, becasue sessionstore data
     // associated with the tab will automatically go away
+    return !tabNotClosed;
   },

If you take my suggestion above on removeHiddenGroup, you don't need this modification, though it does improve the quality of the "tabRemoved" message. On the other hand, why do we have "tabRemoved" when we already have "close"? Seems like that should get sorted out.

        Attachment #481297 -
        Flags: feedback?(ian) → feedback-

    
  
Blocks: 594863

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #6)
> Comment on attachment 481297 [details] [diff] [review]
> v1
> 
> +  // Function: _showHiddenGroup
> +  // Shows the hidden group.
> 
> I find this name a little confusing, like this group has a hidden group, rather
> than that this group is hidden. Maybe a better name would be just _unhide()?
> 
> +  // Function: removeHiddenGroup
> 
> This name is similarly confusing. How about closeHidden()?
> 

Fixed

> I think this routine is more complicated than it needs to be.  Rather than
> doing the removeSubscriber/shouldRemoveTabItems thing, just close all the tabs
> and then check to see if the group has automatically closed itself. If it
> hasn't, check to see if it still has tabs in it. If it does, unhide it,
> otherwise close it.
> 
> You may need to modify GroupItem.close so it does the right thing in the hidden
> state, but that's better than having what's essentially a copy of it in this
> routine. 
> 

I've tried your suggestion however, it took me a while to figure the following flow.

When gBrowser.removeTab() is called, the "TabClose" event is fired.  "TabClose" only means that the browser tab is about to close, not already closed. It would trigger the  item "close" event is fired and then the browser tab would get closed. In other words, the group "close" event is fired before all browser tabs in the group are closed.  if I If I change the code based on your suggestion , several tests would be broken because those tests assume that all browser tabs in that group are closed before the group "close" event is fired.  

Therefore, I keep the code and just simplified it a bit to use GroupItem.close();.


> +        GroupItems.removeHiddenGroups();
> 
> Why did you move this out of hideTabView? Seemed like a better spot for it.
>

That's what I thought as well, however, putting GroupItems.removeHiddenGroups() would cause some undesired effects like the "Stay on page/ Leave page" dialog would appear twice.  Therefore, I keep it in the zoomIn method.
 

>    removeHiddenGroups: function GroupItems_removeHiddenGroups() {
> -    iQ(".undo").remove();
> -    
>      // ToDo: encapsulate this in the group item. bug 594863
> 
> Looks like you've done that ToDo; remove the comment and make a note in the bug
> to resolve it once this lands.
> 

Sure


>  close: function TabItem_close() {
>      gBrowser.removeTab(this.tab);
> -    this._sendToSubscribers("tabRemoved");
> +    let tabNotClosed = 
> +      Array.some(gBrowser.tabs, function(tab) { return tab == this.tab; },
> this);
> +    if (!tabNotClosed)
> +      this._sendToSubscribers("tabRemoved");
> 
>      // No need to explicitly delete the tab data, becasue sessionstore data
>      // associated with the tab will automatically go away
> +    return !tabNotClosed;
>    },
> 
> If you take my suggestion above on removeHiddenGroup, you don't need this
> modification, though it does improve the quality of the "tabRemoved" message.
> On the other hand, why do we have "tabRemoved" when we already have "close"?
> Seems like that should get sorted out.

When "TabClose" event is fired, the browser tab is about to close and our item "close" is fired.  Then, the browser tab get closed. Therefore, the "close" event doesn't mean the tab is already removed.

        Attachment #481297 -
        Attachment is obsolete: true

        Attachment #481783 -
        Flags: feedback?(ian)

    
    

    
  
Comment on attachment 481783 [details] [diff] [review]
v1

(In reply to comment #7)
> > +        GroupItems.removeHiddenGroups();
> > 
> > Why did you move this out of hideTabView? Seemed like a better spot for it.
> >
> 
> That's what I thought as well, however, putting GroupItems.removeHiddenGroups()
> would cause some undesired effects like the "Stay on page/ Leave page" dialog
> would appear twice.  Therefore, I keep it in the zoomIn method.

This worries me, as it means it'll have to be in two places (at the moment, maybe more in the future). Can you please look into why you're getting those undesired effects? I would think even if the routine got called twice, the first one would clean everything out (either closing or unhiding every group). Unless an exception is being thrown?

        Attachment #481783 -
        Flags: feedback?(ian) → feedback-

    
  
Depends on: 595521

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 (obsolete)
        — 
          — Splinter Review
      

    


  
* Require patch from Bug 595521

(In reply to comment #8)
> Comment on attachment 481783 [details] [diff] [review]
> v1
> 
> (In reply to comment #7)
> > > +        GroupItems.removeHiddenGroups();
> > > 
> > > Why did you move this out of hideTabView? Seemed like a better spot for it.
> > >
> > 
> > That's what I thought as well, however, putting GroupItems.removeHiddenGroups()
> > would cause some undesired effects like the "Stay on page/ Leave page" dialog
> > would appear twice.  Therefore, I keep it in the zoomIn method.
> 
> This worries me, as it means it'll have to be in two places (at the moment,
> maybe more in the future). Can you please look into why you're getting those
> undesired effects? I would think even if the routine got called twice, the
> first one would clean everything out (either closing or unhiding every group).
> Unless an exception is being thrown?

If the GroupItems.removeHiddenGroups() is placed inside the hideTabView(), the routine would be:
* zoomIn() => the target tab is selected > onTabSelect() > hideTabView() => tab with confirmation prompt is selected > onTabSelect() > hideTabView() => prompt is displayed > user selects "Stay on page" or "Leave page".

The hideTabView()and GroupItems.removeHiddenGroups() (inside the hideTabView()) are called twice in the above routine.  That's why the prompt is displayed twice as well as we try to close the same tab twice.

Another problem is that the target tab wouldn't be selected if user selects "Stay on page" in the above routine. The tab with prompt would remain selected.

        Attachment #481783 -
        Attachment is obsolete: true

        Attachment #483404 -
        Flags: feedback?(ian)

    
    

    
  
Comment on attachment 483404 [details] [diff] [review]
v1

(In reply to comment #9)
> If the GroupItems.removeHiddenGroups() is placed inside the hideTabView(), the
> routine would be:
> * zoomIn() => the target tab is selected > onTabSelect() > hideTabView() => tab
> with confirmation prompt is selected > onTabSelect() > hideTabView() => prompt
> is displayed > user selects "Stay on page" or "Leave page".
> 
> The hideTabView()and GroupItems.removeHiddenGroups() (inside the hideTabView())
> are called twice in the above routine.  That's why the prompt is displayed
> twice as well as we try to close the same tab twice.

Thank you for explaining this. 

The thing is, we're not in control of all of the times we get TabSelect events (for instance when pages open due to external links), so we really can't cover all cases unless we put it inside onTabSelect or hideTabView, but then you get the nesting you describe. I think the best approach is for GroupItems to set a "_removingHiddenGroups" flag on itself and guard against re-entry that way. 

> Another problem is that the target tab wouldn't be selected if user selects
> "Stay on page" in the above routine. The tab with prompt would remain selected.

I think this would actually be the correct behavior; if you chose to "stay on page", you probably have some unfinished business there.

        Attachment #483404 -
        Flags: feedback?(ian) → feedback-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 (obsolete)
        — 
          — Splinter Review
      

    


  
* This patch requires patch for bug 595521

(In reply to comment #10)> 
> The thing is, we're not in control of all of the times we get TabSelect events
> (for instance when pages open due to external links), so we really can't cover
> all cases unless we put it inside onTabSelect or hideTabView, but then you get
> the nesting you describe. I think the best approach is for GroupItems to set a
> "_removingHiddenGroups" flag on itself and guard against re-entry that way. 
> 

Done

> > Another problem is that the target tab wouldn't be selected if user selects
> > "Stay on page" in the above routine. The tab with prompt would remain selected.
> 
> I think this would actually be the correct behavior; if you chose to "stay on
> page", you probably have some unfinished business there.

Done

        Attachment #483404 -
        Attachment is obsolete: true

        Attachment #483913 -
        Flags: feedback?(ian)

    
    

    
  
Comment on attachment 483913 [details] [diff] [review]
v1

Thanks, looking good. One thing: 

>@@ -978,16 +1012,17 @@ GroupItem.prototype = Utils.extend(new I
>       .addClass("appTabIcon")
>       .attr("src", icon)
>       .data("xulTab", xulTab)
>       .appendTo(this.$appTabTray)
>       .click(function(event) {
>         if (Utils.isRightClick(event))
>           return;
> 
>+        GroupItems.removeHiddenGroups();
>         GroupItems.setActiveGroupItem(self);
>         UI.goToTab(iQ(this).data("xulTab"));
>       });

You can remove this .removeHiddenGroups call (now that you're not removing it from hideTabView).

        Attachment #483913 -
        Flags: feedback?(ian) → feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 (obsolete)
        — 
          — Splinter Review
      

    


  
Removed the unnecessary .removeHiddenGroups call

f+=Ian

        Attachment #483913 -
        Attachment is obsolete: true

        Attachment #484016 -
        Flags: review?(dolske)

        Attachment #484016 -
        Flags: feedback?(ian)

    
  

        Attachment #484016 -
        Flags: feedback?(ian)

    
  
Blocks: 599623

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 (require patch in bug 595521) (obsolete)
        — 
          — Splinter Review
      

    


  
The last patch couldn't be applied cleanly.  Here is a new one.

        Attachment #484016 -
        Attachment is obsolete: true

        Attachment #484243 -
        Flags: review?(dolske)

        Attachment #484016 -
        Flags: review?(dolske)

    
    

    
  
So, I'm missing something there. Comment 0 describes a problem caused by an onbeforeunload handler, which is commonly used to either (1) warn the user they're closing a page that has unsaved data or (2) annoy the user into staying on the site.

If you try and close a window with such a tab in it, we cancel the close, focus the tab with the problem, and show a prompt.

What should Panorama be doing if you close such a group? Nothing? Exiting Panorama to show the tab+prompt? Something else?

    
    

    
  
(In reply to comment #15)
> So, I'm missing something there. Comment 0 describes a problem caused by an
> onbeforeunload handler, which is commonly used to either (1) warn the user
> they're closing a page that has unsaved data or (2) annoy the user into staying
> on the site.
> 
> If you try and close a window with such a tab in it, we cancel the close, focus
> the tab with the problem, and show a prompt.
> 
> What should Panorama be doing if you close such a group? Nothing? Exiting
> Panorama to show the tab+prompt? Something else?

In Panorama view, if user closes the group and then close the "Undo Close group", the group would disappear in the UI and the prompt would display and ask whether you want to stay on the page or leave the page.  If user stays on the page, the group would re-appear again.  If user leaves the page, the group wouldn't appear.

In comment 0, step 7 - Choose a remaining group which means user picks a tab and exit the Panorama view.  When exiting Panorama, all the hidden groups would be removed.  Since a group contains a page with onbeforeunload handler, that tab+prompt would be shown.  If user stays on the page, the tab would remain selected.  If user decides to leave the page, the tab would be closed and the tabs in the original selected appropriate group would be displayed.

    
  

        Attachment #484243 -
        Attachment description: v1 → v1 (require patch in bug 595521)

    
  
Blocks: 596781

    
    

    
  
Requesting blocking as this now blocks a blocker (bug 599623).
blocking2.0: --- → ?
blocking2.0: ? → betaN+

    
  
Blocks: 610934

        Attachment #484243 -
        Flags: review?(dolske) → review+

    
    

    
  


  
r=dolske, a=blocking2.0

        Attachment #484243 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
Looks like it's been a while since this patch has had a try run (comment 5), and it's hard for me to tell how much has changed.  Does it need another?

    
    

    
  
(In reply to comment #19)
> Looks like it's been a while since this patch has had a try run (comment 5),
> and it's hard for me to tell how much has changed.  Does it need another?

Sent it to try 52985d23f93e and waiting for the result

    
    

    
  
(In reply to comment #20)
> Sent it to try 52985d23f93e and waiting for the result

Passed try

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/c5e14ccf7622
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: