Closed Bug 899286 Opened 11 years ago Closed 11 years ago

Intermittent browser_tabsettings.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1827 and more

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26
Tracking Flags:
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: RyanVM, Assigned: mikedeboer)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

Patch v3: clear timers on findbar close and destroy
2.88 KB, patch
dao
: review+
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
More findbar.xml flakiness since the recent rewrite. Mike, can you please look into these failures?

https://tbpl.mozilla.org/php/getParsedLog.php?id=25827031&tree=Mozilla-Central

Ubuntu VM 12.04 x64 mozilla-central opt test mochitest-browser-chrome on 2013-07-27 20:17:47 PDT for push 015135250e06
slave: tst-linux64-ec2-135

20:54:09     INFO -  TEST-START | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js
20:54:09     INFO -  TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Should have an add-ons manager window
20:54:09     INFO -  TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Should be displaying the correct UI
20:54:09     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | must wait for load
20:54:10     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Running test 1
20:54:10     INFO -  TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Options should be inline type
20:54:10     INFO -  TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Element should not be null, when checking visibility
20:54:10     INFO -  TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Preferences button should be visible
20:54:10  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1827
20:54:10     INFO -  Stack trace:
20:54:10     INFO -      JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1136
20:54:10     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
20:54:10     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Console message: [JavaScript Error: "TypeError: this.close is not a function" {file: "chrome://global/content/bindings/findbar.xml" line: 1827}]
20:54:10     INFO -  TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | New tab should have loaded the options URL
20:54:10     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Test 1 took 296ms
20:54:10     INFO -  TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Should have an add-ons manager window to close
20:54:10     INFO -  TEST-PASS | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | Should be closing window with correct URI
20:54:10     INFO -  INFO TEST-END | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_tabsettings.js | finished in 877ms
Flags: needinfo?(mdeboer)
I certainly will!
Flags: needinfo?(mdeboer)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
When working on a Try push - https://tbpl.mozilla.org/?tree=Try&rev=321ae7095374 - I noticed that mochitest-bc was perma-oranging on the findbar. This is 'browser/base/content/test/browser_zbug569342.js', a completely different test than the one above, but because of its predictability I was able to reproduce it locally on my Ubuntu box.

The cause is that there are many timers running inside the findbar of 500ms and more. When the findbar is destroyed, the binding methods are not available anymore that long after, when the timers finish.
Relevant try run: https://tbpl.mozilla.org/?tree=Try&rev=85f0cb385a11

This resolves most, if not all, findbar intermittent oranges. Timers were running and callback were evaluated well after the findbar was destroyed, causing syntax errors. The usual timeout length is 500ms and interval length is 5000ms(!).
Attachment #786827 - Flags: review?(ttaubert)
Comment on attachment 786827 [details] [diff] [review]
Patch v1: clear timers on findbar close and destroy

Review of attachment 786827 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think think I'm familiar enough with the findbar to review it but I'll give you some feedback anyway :)

::: toolkit/content/widgets/findbar.xml
@@ +430,5 @@
>                                   this._observer);
> +
> +          // Clear all timers that might still be running.
> +          if (this._flashFindBarTimeout)
> +            clearInterval(this._flashFindBarTimeout);

Shouldn't we also clear all timers when clear() is called?

@@ +436,5 @@
> +            clearTimeout(this._highlightTimeout);
> +          if (this._findResetTimeout)
> +            clearTimeout(this._findResetTimeout);
> +          if (this._findResetTimeout)
> +            clearTimeout(this._findResetTimeout);

You're checking _findResetTimeout twice.

@@ +1202,5 @@
>            }
> +          if (this._flashFindBarTimeout) {
> +            clearInterval(this._flashFindBarTimeout);
> +            this._flashFindBarTimeout = null;
> +          }

The whole _flash() method seems a little weird to me. Can't we just set an attribute and make it flash via a CSS animation? That would be one timer to less to care about.

@@ +1765,5 @@
>            this.open(aMode);
>  
>            if (this._flashFindBar) {
>              this._flashFindBarTimeout =
> +              setInterval(function() { this._flash(); }.bind(this), 500);

setInterval(() => this._flash(), 500);
Attachment #786827 - Flags: review?(ttaubert)
Updated patch with Tim's comments addressed. Thanks Tim!
Attachment #786827 - Attachment is obsolete: true
Attachment #787414 - Flags: review?(dao)
Blocks: 257061
Comment on attachment 787414 [details] [diff] [review]
Patch v2: clear timers on findbar clear, close and destroy

>+      <method name="_clearTimers">

s/clear/cancel/ ?

>+          if (this._flashFindBarTimeout) {
>+            clearInterval(this._flashFindBarTimeout);
>+            this._flashFindBarTimeout = null;
>+          }
>+          if (this._quickFindTimeout) {
>+            clearTimeout(this._quickFindTimeout);
>+            this._quickFindTimeout = null;
>+          }
>+          if (this._highlightTimeout)
>+            clearTimeout(this._highlightTimeout);
>+          if (this._findResetTimeout)
>+            clearTimeout(this._findResetTimeout);

Why are you setting only some properties to null?

>       <method name="clear">
>         <body><![CDATA[
>           this.browser.fastFind.collapseSelection();
>           this._findField.reset();
>           this.toggleHighlight(false);
>           this._updateStatusUI();
>           this._enableFindButtons(false);
>           this._setFoundLink(null);
>+          this._clearTimers();
>           this._foundEditable = null;
>           this._currentWindow = null;
>         ]]></body>
>       </method>

Why should timers be cancelled here?
(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 787414 [details] [diff] [review]
> Patch v2: clear timers on findbar clear, close and destroy
> 
> >+      <method name="_clearTimers">
> 
> s/clear/cancel/ ?

Sure, I'll rename that!

> Why are you setting only some properties to null?

Because relevant parts of the findbar code rely on this for these properties. I can set them all to null, is that preferred?

> Why should timers be cancelled here?

Tim asked about this in comment 4, I put it there because I think it wouldn't hurt. But now that I thought about it some more I deem this unnecessary. I'll remove it in the next iteration.
Flags: needinfo?(dao)
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Because relevant parts of the findbar code rely on this for these
> properties. I can set them all to null, is that preferred?

I think that would be cleaner.
Flags: needinfo?(dao)

        Attachment #787414 -
        Attachment is obsolete: true

        Attachment #787414 -
        Flags: review?(dao)

        Attachment #795526 -
        Flags: review?(dao)

    
    

    
  
Comment on attachment 795526 [details] [diff] [review]
Patch v3: clear timers on findbar close and destroy

>+      <method name="_cancelTimers">
>+        <body><![CDATA[
>+          if (this._flashFindBarTimeout) {
>+            clearInterval(this._flashFindBarTimeout);
>+            this._flashFindBarTimeout = null;
>+          }
>+          if (this._quickFindTimeout) {
>+            clearTimeout(this._quickFindTimeout);
>+            this._quickFindTimeout = null;
>+          }
>+          if (this._highlightTimeout) {
>+            clearTimeout(this._highlightTimeout);
>+            this._highlightTimeout = null;
>+          }
>+          if (this._findResetTimeout) {
>+            clearTimeout(this._findResetTimeout);
>+            this._findResetTimeout = null;
>+          }
>+        ]]></body>
>+        </method>

</method> is indented too far.

        Attachment #795526 -
        Flags: review?(dao) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6722ec7dacf3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

    
    

    
  
If I understand correctly, this affects Fx25, but not 24, correct? If so, please request Aurora uplift on this patch.

          status-firefox24:
          --- → unaffected

          status-firefox25:
          --- → affected

          status-firefox26:
          --- → fixed

    
    

    
  
Comment on attachment 795526 [details] [diff] [review]
Patch v3: clear timers on findbar close and destroy

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 776708
User impact if declined: mochitest-bc intermittent failures, no known user impact
Testing completed (on m-c, etc.): 2 days on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

        Attachment #795526 -
        Flags: approval-mozilla-aurora?

        Attachment #795526 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
      
  
Depends on: 926897

    
    

    
  
(In reply to Carsten Book [:Tomcat] from comment #19)
> seems its back
> 


filed bug 926897 for that

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: