951142 - Intermittent browser_browserDrop.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1168 | Test timed out

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=32052526&tree=Fx-Team

Rev5 MacOSX Mountain Lion 10.8 fx-team opt test mochitest-browser-chrome on 2013-12-16 15:40:05 PST for push c12ebde00d15
slave: talos-mtnlion-r5-038

16:08:53     INFO -  TEST-START | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js
16:08:53  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1168
16:08:53     INFO -  Stack trace:
16:08:53     INFO -      JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1216
16:08:53     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
16:08:53     INFO -  JavaScript error: chrome://global/content/bindings/findbar.xml, line 1168: this.close is not a function
16:08:53  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1168
16:08:53     INFO -  Stack trace:
16:08:53     INFO -      JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1216
16:08:53     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
16:08:53     INFO -  JavaScript error: chrome://global/content/bindings/findbar.xml, line 1168: this.close is not a function
16:08:53  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1168
16:08:53     INFO -  Stack trace:
16:08:53     INFO -      JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1216
16:08:53     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
16:08:53     INFO -  JavaScript error: chrome://global/content/bindings/findbar.xml, line 1168: this.close is not a function
16:08:53  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1168
16:08:53     INFO -  Stack trace:
16:08:53     INFO -      JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1216
16:08:53     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
16:08:53     INFO -  JavaScript error: chrome://global/content/bindings/findbar.xml, line 1168: this.close is not a function
16:08:53  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1168
16:08:53     INFO -  Stack trace:
16:08:53     INFO -      JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1216
16:08:53     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
16:08:53     INFO -  JavaScript error: chrome://global/content/bindings/findbar.xml, line 1168: this.close is not a function
16:08:53  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1168
16:08:53     INFO -  Stack trace:
16:08:53     INFO -      JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1216
16:08:53     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
16:08:53     INFO -  JavaScript error: chrome://global/content/bindings/findbar.xml, line 1168: this.close is not a function
16:08:53     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | Console message: [JavaScript Error: "TypeError: this.close is not a function" {file: "chrome://global/content/bindings/findbar.xml" line: 1168}]
16:08:53     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | Console message: [JavaScript Error: "TypeError: this.close is not a function" {file: "chrome://global/content/bindings/findbar.xml" line: 1168}]
16:08:53     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | Console message: [JavaScript Error: "TypeError: this.close is not a function" {file: "chrome://global/content/bindings/findbar.xml" line: 1168}]
16:08:53     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | Console message: [JavaScript Error: "TypeError: this.close is not a function" {file: "chrome://global/content/bindings/findbar.xml" line: 1168}]
16:08:53     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | Console message: [JavaScript Error: "TypeError: this.close is not a function" {file: "chrome://global/content/bindings/findbar.xml" line: 1168}]
16:08:53     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | Console message: [JavaScript Error: "TypeError: this.close is not a function" {file: "chrome://global/content/bindings/findbar.xml" line: 1168}]
16:09:24     INFO -  libpng warning: zero length keyword
16:09:24     INFO -  libpng warning: Empty language field in iTXt chunk
16:09:29     INFO -  SCREENSHOT: <see log>
16:09:29  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | Test timed out
16:09:29     INFO -  TEST-INFO | MEMORY STAT vsize after test: 4552404992
16:09:29     INFO -  TEST-INFO | MEMORY STAT residentFast after test: 865214464
16:09:29     INFO -  TEST-INFO | MEMORY STAT heapAllocated after test: 190782816
16:09:29     INFO -  INFO TEST-END | chrome://mochitests/content/browser/toolkit/content/tests/browser/browser_browserDrop.js | finished in 30059ms

Comment 1

[Mike de Boer [:mikedeboer]]

In bug 899286 I fixed a similar issue where I could circumvent putting a conditional before the action. Bug 926897 is VERY similar.

I could do that here and prevent future issues like this (see http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#1172); `if (this.close) this.close();`

However, the issue seems to be that the findbar receives a keypress event even though it's not initialized and opened at all. The event handler phase is set to 'capturing'. This can be due to bug 537013, which moved the findbar into the tabbrowser, thus making it the first target handler during event capture phase for VK_ESCAPE key events originating from elements lower down the DOM tree.

IMO, the findbar ESC key event handler is set to be too greedy by using the capturing phase.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: check for a close method to be present on the binding before invoking it

Taking this as the first route, because I think that changing the event phase has too much potential impact on other toolkit consumers.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=689539f1587a

Comment 3

[:Gavin Sharp [email: [e-mail]]]

The underlying issue here is that someone is trying to call this.close when the binding has been torn down. The XBL handler should be removed during binding teardown, so I'm skeptical that that patch would fix anything. But there is another this.close() caller in handleEvent, I bet it's the one throwing this error. Shouldn't we remove the keypress and mouseup handlers added by the "browser" setter in this binding's destructor?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(and I assume bug 926897 is the same bug, just in a different test/different platform)

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #3)
> The underlying issue here is that someone is trying to call this.close when
> the binding has been torn down. The XBL handler should be removed during
> binding teardown, so I'm skeptical that that patch would fix anything. But
> there is another this.close() caller in handleEvent, I bet it's the one
> throwing this error.

I don't think so, as the error "uncaught exception - TypeError: this.close is not a function at chrome://global/content/bindings/findbar.xml:1168" definitively points to the call in the handler element.

> Shouldn't we remove the keypress and mouseup handlers
> added by the "browser" setter in this binding's destructor?

We should. Will that also prevent event handlers defined in <handler> elements to be disabled?

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> > Shouldn't we remove the keypress and mouseup handlers
> > added by the "browser" setter in this binding's destructor?
> 
> We should. Will that also prevent event handlers defined in <handler>
> elements to be disabled?

...but this is already done by setting the browser property to `null` at http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#379

So handleEvent will not be called anymore after the findbar destructor is invoked. I still think that somehow the capturing event handler is active and causes this orange.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Indeed, you are right!

This is mysterious, and I'd like to understand the underlying issue, but I won't have a chance to look into it further and I suppose it's not necessary to block on that.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8348777 [details] [diff] [review]
Patch 1: check for a close method to be present on the binding before invoking it

At the very least there should be a comment explaining why the null check is there (wallpaper over some unknown issue) and a followup bug filed, though.

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: check for a close method to be present on the binding before invoking it

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8357688 [details] [diff] [review]
Patch 1: check for a close method to be present on the binding before invoking it

Thanks!

Comment 11

[Mike de Boer [:mikedeboer]]

remote: https://hg.mozilla.org/integration/fx-team/rev/113e065a2803

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/113e065a2803

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This applies cleanly or with minor unbitrotting all the way down to esr24. Is there any reason we can't take this patch in the name of fewer intermittents?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

no

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8357688 [details] [diff] [review]
Patch 1: check for a close method to be present on the binding before invoking it

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: random oranges on TBPL
Testing completed: on m-c
Risk to taking this patch (and alternatives if risky): minimal, it just adds some extra protection against this.close being null
String or UUID changes made by this patch: none

Comment 17

[bhavana bajaj [:bajaj]]

Comment on attachment 8357688 [details] [diff] [review]
Patch 1: check for a close method to be present on the binding before invoking it

Approving on all branches to avoid TBPL failures.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/68c89b39ed6c
https://hg.mozilla.org/releases/mozilla-beta/rev/cdd4b0b788a3
https://hg.mozilla.org/releases/mozilla-esr24/rev/57de5d980482
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f37664c7e70d