1663904 - Using Global Search bar logs error: TypeError: can't access property "hasAttribute", window.gBrowser.selectedBrowser is null

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Lasana Murray]

STR

• Enter a search term in the global search bar.
• Hit enter key.
• Open console:

Expected Result:
Search results should show with no errors in console.

Actual Result:
Search works but the following trace is observed:

Uncaught TypeError: can't access property "hasAttribute", window.gBrowser.selectedBrowser is null
    AutocompleteInput chrome://global/content/elements/autocomplete-input.js:70
    openTab chrome://messenger/content/glodaFacetTab.js:36
    openTab chrome://messenger/content/tabmail.js:959
    doSearch chrome://messenger/content/gloda-autocomplete-input.js:217
    MozGlodaAutocompleteInput chrome://messenger/content/gloda-autocomplete-input.js:60

I came across this while working on bug 1658227.

Comment 1

[Magnus Melin [:mkmelin]]

https://searchfox.org/mozilla-central/rev/b2716c233e9b4398fc5923cbe150e7f83c7c6c5b/toolkit/content/widgets/autocomplete-input.js#69-70

https://searchfox.org/comm-central/rev/1b97640106e37ba9a0a208f937b8396de9ac89d9/mail/base/content/msgMail3PaneWindow.js#649
Though it looks like selectedBrowser would return something at that point.

Comment 2

[Lasana Murray]

This does not appear to affect 78 though I have been unable so far to determine the difference between it and 82.

The error happens when the focus() method is called here https://searchfox.org/comm-central/source/mail/base/content/glodaFacetTab.js#36, it prematurely causes the "focus" event listener in autocomplete-input to be fired before the tab's browser property is properly initialized.
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/autocomplete-input.js#64

This does not seem to happen on prior versions as far as I can tell.

Comment 3

[Lasana Murray]

Looks like this error got introduced with this commit. It sets the global gBrowser property to tabmail causing the second part of the if condition in autocomplete-input to be evaluated (window.gBrowser would have been undefined previously).

https://hg.mozilla.org/comm-central/rev/08e8b872695145ef325df8fee38139517d3f1be2#l3.30

Comment 4

[Lasana Murray]

Attachment: 1663904.patch

I moved the call to focus() into the xulLoadHandler(). That way the browser is not null when autocomplete-input does its checks.

Comment 5

[Lasana Murray]

Try run in progress:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4426a11b8c6fcd6b732574b624c1e903cd3c782e

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9180233 [details] [diff] [review]
1663904.patch

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

::: mail/test/browser/global-search-bar/browser_global-search-bar.js
@@ +10,5 @@
> +  let errorThrown = false;
> +  let searchInput = window.document.querySelector("#searchInput");
> +
> +  window.addEventListener("error", e => {
> +    if (e.message.includes("window.gBrowser.selectedBrowser is null")) {

why restrict it to this specific error?

Comment 7

[Lasana Murray]

(In reply to Magnus Melin [:mkmelin] from comment #6)

Comment on attachment 9180233 [details] [diff] [review]
1663904.patch

Review of attachment 9180233 [details] [diff] [review]:

::: mail/test/browser/global-search-bar/browser_global-search-bar.js
@@ +10,5 @@

• let errorThrown = false;
• let searchInput = window.document.querySelector("#searchInput");
• window.addEventListener("error", e => {
• if (e.message.includes("window.gBrowser.selectedBrowser is null")) {

why restrict it to this specific error?

I figure it's better to be specific here in case any future bugs mislead us into thinking this bug has regressed.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9180233 [details] [diff] [review]
1663904.patch

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

The naming convention for mochitests are like browser_glodaSearchTab.js

::: mail/test/browser/global-search-bar/browser_global-search-bar.js
@@ +10,5 @@
> +  let errorThrown = false;
> +  let searchInput = window.document.querySelector("#searchInput");
> +
> +  window.addEventListener("error", e => {
> +    if (e.message.includes("window.gBrowser.selectedBrowser is null")) {

In general we test for effects, not that certain errors aren't thrown. And if you restrict checking to only this error, any other future errors would go unnoticed.
Maybe test that the focusing succeeded instead, (document.activeElement is the search input)?

@@ +19,5 @@
> +  searchInput.value = "Bugzilla";
> +  EventUtils.synthesizeMouseAtCenter(searchInput, {}, window);
> +  EventUtils.synthesizeKey("VK_RETURN", {}, window);
> +
> +  Assert.ok(!errorThrown, "using global search did no throw an error");

Unfortunately I don't think you can trust that the key press had time to cause any effects just yet. That is, the error listener may not yet have been invoked when you check this.

If you try ./mach test --verify it will also fail

Comment 9

[Lasana Murray]

Attachment: 1663904v2.patch

Changed the test to check the searchInput has focus. Added a cleanup function so --verify works.

Comment 10

[Lasana Murray]

Try run in progress:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eec0d44caf18b2619ce38e4d5cbb378f1e7c34ca

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9180469 [details] [diff] [review]
1663904v2.patch

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

Looks great, thx! r=mkmelin

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f56b81827087
Focus global search input after browser has been loaded. r=mkmelin DONTBUILD