Closed Bug 615954 Opened 14 years ago Closed 13 years ago

Intermittent Failure in tabview/browser_tabview_bug597248.js | Tab item is showing cached data and is already connected or | Tab item is not showing cached data anymore

Categories

(Firefox Graveyard :: Panorama, defect, P3)

Product:
Firefox Graveyard
Component:
Panorama
Platform:
x86_64
Linux
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 5

People

(Reporter: iangilman, Assigned: raymondlee)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 13 obsolete files)

Patch for check-in
8.86 KB, patch
Details | Diff | Splinter Review 
      No description provided.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1291231055.1291232184.364.gz&fulltext=1
Rev3 Fedora 12x64 mozilla-central opt test mochitest-other on 2010/12/01 11:17:35
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug597248.js | Tab item is showing cached data
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug597248.js | Tab item is showing cached data
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug597248.js | Tab item is not showing cached data anymore
}
OS: All → Linux
Hardware: All → x86_64
Whiteboard: [orange]
Version: unspecified → Trunk
Attached patch v1 (obsolete) — — Splinter Review 
Assignee: nobody → raymond

        Attachment #494670 -
        Flags: feedback?(ian)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

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

Let's do it.  It needs no approval since it's a test fix.

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for check-in (obsolete)
        — 
          — Splinter Review
      

    


  
Tests for debug build don't run on try except win debug (it's a try server problem), other than that, all tests passed.

        Attachment #494670 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/bedcf4735d6f
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8

    
    

    
      
  
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: Firefox 4.0b8 → ---

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  
Summary: Intermittent Failure in tabview/browser_tabview_bug597248.js | Tab item is showing cached data → Intermittent Failure in tabview/browser_tabview_bug597248.js | Tab item is showing cached data or | Tab item is not showing cached data anymore

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #494758 -
        Attachment is obsolete: true

        Attachment #495831 -
        Flags: review?(ian)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #495831 -
        Attachment is obsolete: true

        Attachment #495840 -
        Flags: review?(ian)

        Attachment #495831 -
        Flags: review?(ian)

    
    

    
  
Comment on attachment 495840 [details] [diff] [review]
v2

Let's give it a try.

        Attachment #495840 -
        Flags: review?(ian) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for check-in (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #495840 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/3171bf1a5109
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED

    
    

    
      
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Intermittent Failure in tabview/browser_tabview_bug597248.js | Tab item is showing cached data or | Tab item is not showing cached data anymore → Intermittent Failure in tabview/browser_tabview_bug597248.js | Tab item is showing cached data and is already connected or | Tab item is not showing cached data anymore

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
That's two fix attempts that haven't worked (though maybe it's failing less now than at the beginning... hard to say). Any theories on why it's still failing? Perhaps we could land some diagnostic code on it to help illuminate the situation?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #51)
> That's two fix attempts that haven't worked (though maybe it's failing less now
> than at the beginning... hard to say). Any theories on why it's still failing?
> Perhaps we could land some diagnostic code on it to help illuminate the
> situation?

It might be the code is calling undoClosedWindow too early.  I have changed it from "unload" to "xul-window-destroyed" now.  Also added some code to print out which tab is falling since it looks like the errors came from one of the two tabs.

        Attachment #495862 -
        Attachment is obsolete: true

        Attachment #496780 -
        Flags: review?(ian)

    
    

    
  
Comment on attachment 496780 [details] [diff] [review]
v3

Cool, let's do it.

        Attachment #496780 -
        Flags: review?(ian) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for check-in (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #496780 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/210237f7d626
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → Firefox 4.0b8

    
    

    
      
  

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Raymond, is it possible there's actually something wrong with the code this test is testing?

Also, do the new logs help?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v4 (obsolete)
        — 
          — Splinter Review
      

    


  
It's possible that about:blank is loaded which triggers the code in the onStateChange() when a tab is restored but before last visited url is loaded.  Therefore, an validation is added.

        Attachment #497209 -
        Attachment is obsolete: true

        Attachment #497860 -
        Flags: review?(ian)

    
    

    
      
  

    
    

    
  
Comment on attachment 497860 [details] [diff] [review]
v4

Let's do it.

        Attachment #497860 -
        Flags: review?(ian) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch for check-in (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #497860 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/822e23ac1f71
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED

    
  
Keywords: checkin-needed

    
    

    
      
  

    
    

    
      
  
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: Firefox 4.0b8 → ---

    
    

    
  
Still not fixed :(

    
    

    
  
Raymond, more evidence that this may be a real failure, not just a test issue: bug 619385. Note that he's running with an awful lot of tabs, which can slow things down a lot, and the test is running on the build server which is known to be very slow, so maybe there's some sort of timing issue?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v5 (obsolete)
        — 
          — Splinter Review
      

    


  
I suspect that TabItems._update() which hides cached thumbnail is called sometimes before the test checks whether the cached thumbnails are being displayed or not.  Therefore, the cached thumbnails may already be removed.  I've added a flag to prevent TabItems._update() gets called before checking the cached thumbnails.

After reading the comment on bug 619385 point 3, I've figured out that the content of restored tab wouldn't be loaded until the tab is selected.  In other words, if you open a window with restored tabs and then go into TabView, you would see empty thumbnails because those tabs are not loaded yet.  Now, saving all tabs' thumbnails on exit regardless TabView is displayed or not would solve this.

        Attachment #498009 -
        Attachment is obsolete: true

        Attachment #498395 -
        Flags: review?(ian)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 498395 [details] [diff] [review]
v5

I'll go ahead and land this, as Raymond's on vacation.

        Attachment #498395 -
        Flags: review?(ian) → review+

    
    

    
      
  

    
  
Blocks: 619385

    
    

    
      
  

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/d538a677628e
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED

    
    

    
      
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Is it just me, or has this gotten worse recently? Or maybe we just started landing a bunch on m-c after the holidays. :/

Raymond, any ideas? I'm happy to take a look at this if you'd like.
Keywords: helpwanted

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to comment #211)
> Is it just me, or has this gotten worse recently? Or maybe we just started
> landing a bunch on m-c after the holidays. :/
> 
> Raymond, any ideas? I'm happy to take a look at this if you'd like.

Raymond will be back tomorrow. We've already tried several times to plug this, though, so maybe fresh eyes would be good.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to comment #214)
> (In reply to comment #211)
> > Is it just me, or has this gotten worse recently? Or maybe we just started
> > landing a bunch on m-c after the holidays. :/
> > 
> > Raymond, any ideas? I'm happy to take a look at this if you'd like.
> 
> Raymond will be back tomorrow. We've already tried several times to plug this,
> though, so maybe fresh eyes would be good.

mitcho: yes, a pair of fresh eyes would be great! Thanks.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
http://brasstacks.mozilla.com/couchdb/orange_factor/_design/woo/orange.html?display=TopBugs&endday=2011-01-09&startday=2011-01-02&bugCount=10&dayCount=All

Looks like a big enough piece of the pie that it's probably time to disable the failing part, and only reenable it while someone's landing a possible fix.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to comment #277)
> http://brasstacks.mozilla.com/couchdb/orange_factor/_design/woo/orange.html?display=TopBugs&endday=2011-01-09&startday=2011-01-02&bugCount=10&dayCount=All
> 
> Looks like a big enough piece of the pie that it's probably time to disable the
> failing part, and only reenable it while someone's landing a possible fix.

Agreed. Raymond, can you please do this?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v6 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #281)
> (In reply to comment #277)
> > http://brasstacks.mozilla.com/couchdb/orange_factor/_design/woo/orange.html?display=TopBugs&endday=2011-01-09&startday=2011-01-02&bugCount=10&dayCount=All
> > 
> > Looks like a big enough piece of the pie that it's probably time to disable the
> > failing part, and only reenable it while someone's landing a possible fix.
> 
> Agreed. Raymond, can you please do this?

Disabled the failing part in the test.

        Attachment #502716 -
        Flags: review?(ian)

    
  

        Attachment #498395 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 502716 [details] [diff] [review]
v6

Looks good; I'm going to go ahead and land it now.

        Attachment #502716 -
        Flags: review?(ian) → review+

    
    

    
      
  

    
    

    
  
Ok, I've landed the disabling patch: 

http://hg.mozilla.org/mozilla-central/rev/c14e691ffac6

... but this bug isn't actually fixed yet until we can re-enable it without getting errors. I imagine we'll get back on that post-ff4.
Target Milestone: --- → Future

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Do not disable test, please.

This failure is not a test issue, it's a bug of the code of session store and tab view surely.

I could reproduce this test failure several times and found the reason of the failure.

In restoreHistoryPrecursor() in nsSessionStore.js, there is a setTimeout():

    // make sure that all browsers and their histories are available
    // - if one's not, resume this check in 100ms (repeat at most 10 times)
    for (var t = aIx; t < aTabs.length; t++) {
      try {
        if (!tabbrowser.getBrowserForTab(aTabs[t]).webNavigation.sessionHistory) {
          throw new Error();
        }
      }
      catch (ex) { // in case browser or history aren't ready yet 
        if (aCount < 10) {
          var restoreHistoryFunc = function(self) {
            self.restoreHistoryPrecursor(aWindow, aTabs, aTabData, aSelectTab, aIx, aCount + 1);
          }
          aWindow.setTimeout(restoreHistoryFunc, 100, this);
          return;
        }
      }

So the test fails when TabItem__reconnect() is called before session data is ready. (You can be easily reproduce this issue if the timeout value is bigger. 1000 must be enough.)

We need to call TabItem__reconnect() after the session data is surely restored.

Anyway, I do not think disabling test is good idea. Disabling test makes things worse. It veils real issues.
Target Milestone: Future → ---

    
  
No longer blocks: 619385

    
    

    
      
  

    
    

    
  
(In reply to comment #304)
> In restoreHistoryPrecursor() in nsSessionStore.js, there is a setTimeout():
> 
>     // make sure that all browsers and their histories are available
>     // - if one's not, resume this check in 100ms (repeat at most 10 times)
>     for (var t = aIx; t < aTabs.length; t++) {
>       try {
>         if
> (!tabbrowser.getBrowserForTab(aTabs[t]).webNavigation.sessionHistory) {
>           throw new Error();
>         }
>       }
>       catch (ex) { // in case browser or history aren't ready yet 
>         if (aCount < 10) {
>           var restoreHistoryFunc = function(self) {
>             self.restoreHistoryPrecursor(aWindow, aTabs, aTabData, aSelectTab,
> aIx, aCount + 1);
>           }
>           aWindow.setTimeout(restoreHistoryFunc, 100, this);
>           return;
>         }
>       }

zpao, can you eliminate this setTimeout() function? We can catch an event of ready of session? (I do not know there is a such event though.)
  
I think this timeout makes session storing unreliable and some test failures are caused by the timeout.

    
    

    
  
I agree that ultimately it would be best to get rid of that setTimeout (there are others I'd prefer to get rid of as well), but it's not going to happen for Firefox 4. Not by me anyway. You're more than welcome to look into finding an event to listen for instead and if it's safe we might take it (file a new bug if you're going to do that though).

As for the test, there are some new events added that panorama is using now (I think, Ian could confirm). Those should work regardless of this setTimeout. I'm not sure if that's the issue here or not though.

    
    

    
      
  

    
  
Blocks: 585689

    
  
Depends on: 628701

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v7 (obsolete)
        — 
          — Splinter Review
      

    


  
The patch for bug 628701 landed and the issue in this test seems to be fixed as well.  I've updated the test and it passed locally.  Sent it to try.

        Attachment #502716 -
        Attachment is obsolete: true

        Attachment #514724 -
        Flags: review?(ian)

    
  
Status: REOPENED → ASSIGNED

    
    

    
  
Comment on attachment 514724 [details] [diff] [review]
v7

It still didn't pass try, got the same error. :S

        Attachment #514724 -
        Flags: review?(ian)

    
  
Priority: -- → P3

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v8 (obsolete)
        — 
          — Splinter Review
      

    


  
Sent it to try and waiting for the results

        Attachment #514724 -
        Attachment is obsolete: true

        Attachment #515829 -
        Flags: review?(ian)

    
    

    
  
Comment on attachment 515829 [details] [diff] [review]
v8

Thanks for digging into this again. Perhaps one more try run just to be sure, considering its history?

        Attachment #515829 -
        Flags: review?(ian) → review+

    
    

    
  
(In reply to comment #313)
> Comment on attachment 515829 [details] [diff] [review]
> v8
> 
> Thanks for digging into this again. Perhaps one more try run just to be sure,
> considering its history?

Sent it to try and waiting for the results.

    
  

        Attachment #515829 -
        Attachment is obsolete: true

    
  
Keywords: helpwanted → checkin-needed
No longer blocks: 585689

    
    

    
  
bugspam

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/e7038a9c371d
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2

    
  
Target Milestone: Firefox5 → Firefox 5
Whiteboard: [orange]
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: