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: