Closed Bug 570192 Opened 14 years ago Closed 13 years ago

Intermittent timeout in browser_394759.js

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 7

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: intermittent-failure, Whiteboard: [fixed by bug 595292])

Attachments

(1 file, 1 obsolete file)

disable the test
1.42 KB, patch
ehsan.akhgari
: review-
Details | Diff | Splinter Review 
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275672883.1275674743.28939.gz

This happens because the _setNewlineHandling method in textbox.xml (which is called upon construction) does not check if this.editor is null (which it can be, for example, if we don't have a frame).

Patch+test forthcoming.
Blocks: 498339
Attached patch Patch (v1) (obsolete) — — Splinter Review 

        Attachment #449311 -
        Flags: review?(roc)

    
    

    
  
When does this actually happen? If it's possible for the constructor to be firing while this.editor is null, then the newline handling needs to be set up at some other point...

    
    

    
  
(In reply to comment #2)
> When does this actually happen?

See the test case in the patch for at least one way in which this could happen (not to say that the test case is realistic!  But this has happened intermittently on the unit test machines.)

> If it's possible for the constructor to be
> firing while this.editor is null, then the newline handling needs to be set up
> at some other point...

It's kind of tricky, because the widget has no way of knowing when an editor object becomes available.  One good place to set newline handling is right before pasting, but I'm not sure if it's possible to hook into cmd_paste that way.

    
    

    
  
Comment on attachment 449311 [details] [diff] [review]
Patch (v1)

I can't review toolkit

        Attachment #449311 -
        Flags: review?(roc)

        Attachment #449311 -
        Flags: review?(gavin.sharp)

        Attachment #449311 -
        Flags: review?(gavin.sharp)

    
    

    
  
Isn't this just bug 498339? Is with that bug, I don't think the error message has anything to do with the timeout...

    
    

    
  
(In reply to comment #5)
> Isn't this just bug 498339?

Yes.  Except that this patch is solving the real problem (which is a code problem, not a test problem).

> Is with that bug, I don't think the error message
> has anything to do with the timeout...

Well, if I understand correctly, the error message might be causing the execution to be halted, and therefore finish never being called, hence the timeout.

    
    

    
  
gavin: ping?

    
    

    
  
(In reply to comment #6)
> > Is with that bug, I don't think the error message
> > has anything to do with the timeout...
> 
> Well, if I understand correctly, the error message might be causing the
> execution to be halted, and therefore finish never being called, hence the
> timeout.

It's in a different window and asynchronous, so similar to event listeners and timer callbacks, a halted xbl constructor isn't going to affect other JS, as I understand it.

    
    

    
      
  

    
  
OS: Mac OS X → All

    
    

    
  
Comment on attachment 449311 [details] [diff] [review]
Patch (v1)

I filed bug 592528 on fixing the real issue - I'm doubtful that this will do anything but silence the exception, but since it doesn't really do any harm, I suppose it's fine.

Does the test fail without the code change? I wouldn't expect it to... I'm not sure that it's useful.

        Attachment #449311 -
        Flags: review?(gavin.sharp) → review+

    
    

    
  
Please leave the bug open when landing this -- as noted earlier, the patch shouldn't help browser_394759.js at all.

    
    

    
  
(In reply to comment #10)
> Does the test fail without the code change? I wouldn't expect it to... I'm not
> sure that it's useful.

Yes, it does.  That's why it can test the patch!  :-)

    
    

    
  
(In reply to comment #11)
> Please leave the bug open when landing this -- as noted earlier, the patch
> shouldn't help browser_394759.js at all.

So, what does happen when you open a new window and an XBL constructor on that window throws?  Do we get a load event like normal?

    
  

        Attachment #449311 -
        Flags: approval2.0?

        Attachment #449311 -
        Flags: approval2.0? → approval2.0+

    
    

    
  
(In reply to comment #13)
> (In reply to comment #11)
> > Please leave the bug open when landing this -- as noted earlier, the patch
> > shouldn't help browser_394759.js at all.
> 
> So, what does happen when you open a new window and an XBL constructor on that
> window throws?  Do we get a load event like normal?

Why wouldn't we? We get the "this.editor is null" error message even when the test passes: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283423979.1283425603.15720.gz&fulltext=1

    
  
Assignee: ehsan → nobody

    
    

    
      
  

    
  
Status: ASSIGNED → NEW
Component: XUL Widgets → Session Restore
Product: Toolkit → Firefox
QA Contact: xul.widgets → session.restore
Summary: Intermittent timeout in browser_394759.js with "JavaScript error: chrome://global/content/bindings/textbox.xml, line 140: this.editor is null" → Intermittent timeout in browser_394759.js

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
I would guess this latest timeout explosion means "and there's also timeout potential if there's no access to the network, because this test is bad and hits it instead of just stuff on localhost."

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298416270.1298419410.28800.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitest-other on 2011/02/22 15:11:10
s: talos-r3-fed64-017

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_overflowScroll.js | Scrolled one tab to the left with a single click - Got 154, expected 54
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | Found a browser window after previous test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_bug577990.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_bug577990.js | Found unexpected add-ons manager window still open
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_bug577990.js | Should not get category when manager window is not loaded - Didn't expect null, but got it
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_relative.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_relative.js | Found a tab after previous test timed out: about:blank

    
    

    
  
Fix landed a while ago, closing as fixed.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

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

    
  

        Attachment #449311 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
  
I want to point out that the above failure looks suspiciously like bug 498339; in particular, we have the "textbox is null" error before the timeout.

Running chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js...
TEST-INFO | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | Console message: [JavaScript Error: "textbox is null" {file: "chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js" line: 70}]
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | Timed out
Running chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js...
TEST-PASS | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_394759_privatebrowsing.js | sessionstore.js was removed

    
    

    
  
(In reply to comment #40)
> I want to point out that the above failure looks suspiciously like bug 498339;
> in particular, we have the "textbox is null" error before the timeout.

Well, it's bug 498339 in fact!  I'll comment there and reopen.

    
    

    
  
Except that I won't reopen, since it happened on 3.5!

    
    

    
  
Dao: do you think I can close this bug now?  I think by this time we know that the orange was fixed by my patch here...

    
    

    
  
I'm not sure what exactly you're saying. browser_394759.js keeps timing out intermittently, but without the textbox error message, as predicted before your patch landed.

    
    

    
  
(In reply to comment #44)
> I'm not sure what exactly you're saying. browser_394759.js keeps timing out
> intermittently, but without the textbox error message, as predicted before your
> patch landed.

You're right.  Please ignore my comment!

    
  
Whiteboard: [orange] → [orange][not-ready-for-cedar]

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Depends on: 595292

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
And to add to your fun figuring out why this exploded, it failed four times (Mac Mac Win Win) on TraceMonkey, during this morning's downtime.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to comment #24)
> I would guess this latest timeout explosion means "and there's also timeout
> potential if there's no access to the network, because this test is bad and
> hits it instead of just stuff on localhost."

Looks like the culprit might be right here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser/browser_394759.js#164

> let url = "http://window" + windowsToOpen.length + ".example.com";

    
    

    
  
I also see in the log:

TEST-INFO | chrome://mochitests/content/browser/browser/components/sessionstore/test/browser/browser_394759.js | Console message: [JavaScript Error: "ASSERTION: browser.js host is inconsistent. Content window has <window4.example.com> but cached host is <>.
" {file: "chrome://browser/content/browser.js" line: 9533}]

Is that relevant?  Looking at browser.js's onSecurityChange, it looks like this assertion will prevent navigation from happening.

    
    

    
  
And there are several other "example.com" and "mozilla.org" URLs used in this test.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 541205 [details] [diff] [review]
disable the test

This is bug 595292.

        Attachment #541205 -
        Flags: review-

    
    

    
      
  

    
    

    
  
(In reply to comment #88)
> I also see in the log:
> 
> TEST-INFO |
> chrome://mochitests/content/browser/browser/components/sessionstore/test/
> browser/browser_394759.js | Console message: [JavaScript Error: "ASSERTION:
> browser.js host is inconsistent. Content window has <window4.example.com>
> but cached host is <>.
> " {file: "chrome://browser/content/browser.js" line: 9533}]
> 
> Is that relevant?

nope

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
I hereby declare victory!
Assignee: nobody → ehsan
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [orange][not-ready-for-cedar] → [orange][fixed by bug 595292]
Target Milestone: --- → Firefox 7

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Hello.

Is there a way in which this issue can be verified?

    
    

    
  
Steps to verify [orange] bugs:
1) Look for the latest TinderboxPushlog Robot comments on the bug report
2) Ensure that none of the recent comments are for a branch that got the fix (e.g. trunk)
Status: RESOLVED → VERIFIED
Whiteboard: [orange][fixed by bug 595292] → [fixed by bug 595292]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: