1680587 - Focusing on the global search bar from the chat tab fails with "Uncaught TypeError: can't access property "hasAttribute", window.gBrowser.selectedBrowser" when no chat accounts configured.

Status: NEW

(Chat Core :: General, defect)

Description

[Lasana Murray]

STR:

1. Optional: Create a new profile with a mail account and no chat accounts (if you don't want to remove your existing chat accounts).
2. Click the chat button to go to the chat.
3. Enter a search term in the global search bar that should give a result.

Expected Result:
User should see a dropdown menu of matching results, no errors thrown.

Actual Result:
The dropdown still appears but the error is thrown. This hinders testing.

Comment 1

[Lasana Murray]

This happens because the toolkit's autocomplete-input has a "focus" event handler that expects window.gBrowser.selectedBrowser to be set once there is a window.gBrowser.

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

window.gBrowser is <tabmail> and has a getter for selectedBrowser which eventually calls getBrowser() on the current tab's tabinfo object or returns null:
https://searchfox.org/comm-central/rev/4a07865f0fd09191dbc094acc58f47a4b563606a/mail/base/content/tabmail.js#1543

In this case, we rely on chatTabType.getBrowser, unfortunately that also selectively returns null if its desired DOM elements are not found:
https://searchfox.org/comm-central/rev/4a07865f0fd09191dbc094acc58f47a4b563606a/mail/components/im/content/chat-messenger.js#419

This is simillar to the bug 1663904.

I think it's a good idea to avoid returning null/undefined as much as possible. Can lead to hard to track down bugs.

Comment 2

[Lasana Murray]

I'm wondering what the consequences of returning tabmail instead of null from selectedBrower might be? Does not seem to be used much directly in comm-central at least.

https://searchfox.org/comm-central/search?q=selectedBrowser&case=true&path=mail

Comment 3

[Magnus Melin [:mkmelin]]

Sorry I don't have much insight. Would have to try and see what blows up, which could be a lot.

Comment 4

[Lasana Murray]

The property is used a lot in mozilla-central with little to no checks if its null. I see in tabbrowser.js the selectedBrowser value is initialized with a dynamically created browser element so that's probably why.

https://searchfox.org/mozilla-central/rev/6bb59b783b193f06d6744c5ccaac69a992e9ee7b/browser/base/content/tabbrowser.js#370

(I'm assuming tabmail.js is our version of a tabbrowser.js)

Comment 5

[Lasana Murray]

Attachment: bug1680587.patch

Instead of returning null, selectedBrowser will return a throw away <browser> that we initialize in the connectedCallback(). This is sort of inline with what I'm seeing in tabbrowser.js.

Comment 6

[Lasana Murray]

Requesting a review again Alessandro because of the work you did in converting tabmail.

Comment 7

[Lasana Murray]

Try push in progress, hopefully this does not break anything, may still need to keep an eye out in the wild regardless though.

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

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9191330 [details] [diff] [review]
bug1680587.patch

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

Sorry, I'm not super comfortable with this section and I haven't properly wrapped my head around all the gist of tabmail and browser, so I think would be better to bounce this review to Magnus.

::: mail/base/content/tabmail.js
@@ +491,5 @@
>  
> +      /**
> +       * Returned by selectedBrowser if the current tab provides none.
> +       */
> +      this._defaultSelectedBrowser = this._createDefaultSelectedBrowser();

Do we need to define this in the connectedCallback method?
I don't think it's necessary until we call the selectedBrowser() getter, but I might be wrong.

@@ +627,5 @@
> +     * tab types provide none of their own. Some toolkit functions expect this
> +     * property to always be set.
> +     */
> +    _createDefaultSelectedBrowser() {
> +      let browser = document.createXULElement("browser");

I'm not sure this is correct, it seems weird we're creating a fake browser, but I might be wrong.
Also it would probably be worth adding this condition into the getBrowserForSelectedTab() method instead of creating an helper method.

Comment 9

[Alessandro Castellani [:aleca]]

Also, I'm not sure if it's related, but I'm getting this in the terminal when typing a gloda search:

TypeError: can't access property 0, val.identities is null
-- Exception object --
*
-- Stack Trace --
complete/matches<@resource:///modules/GlodaAutoComplete.jsm:236:9
complete@resource:///modules/GlodaAutoComplete.jsm:235:8
startSearch@resource:///modules/GlodaAutoComplete.jsm:559:25

Comment 10

[Lasana Murray]

(In reply to Alessandro Castellani (:aleca) from comment #9)

Also, I'm not sure if it's related, but I'm getting this in the terminal when typing a gloda search:

TypeError: can't access property 0, val.identities is null
-- Exception object --
*
-- Stack Trace --
complete/matches<@resource:///modules/GlodaAutoComplete.jsm:236:9
complete@resource:///modules/GlodaAutoComplete.jsm:235:8
startSearch@resource:///modules/GlodaAutoComplete.jsm:559:25

With this patch applied? Which one of the gloda search bars (mail/chat/results tab)? I can't reproduce.

Comment 11

[alta88]

Creating a fake browser in this layer is unfortunately an incorrect way to bandage this problem. It should be done holistically as requested in Bug 1649035, meaning each nonconforming tabType must implement a getBrowser() that returns a <browser> when onTabOpened() is called. For chat and glodaFacet it means having <browser> in the tab panel or xhtml defined up front rather than later. For calendar it means wrapping the UI in a <browser>, like chromeTab type does for saved files eg.

It's disappointing the big picture solution, that every Tb tab has a <browser> like every Fx tab, isn't being mentored properly here.

Comment 12

[Magnus Melin [:mkmelin]]

I don't understand the initial Step To Reproduce. You want to search, but don't have an account and nothing to search? How would it match anything? Because if you have a log (=old conversation) or conversation showing, you'll have a browser ready, and there is no problem.

Comment 13

[Lasana Murray]

Thanks for sharing bug 1680587 alta88. My first inclination was the tab types should always return browser elements but I don't know much about that api or tabmail yet so I figured there must be a reason they were implemented the way they are.

This bug will have to be fixed via 1680587 then. I'm not quite comfortable with this stuff yet. I'll disable the relevant test in bug 1679113.

Comment 14

[Lasana Murray]

Magnus, it's not 100% clear to me how the global search bars are supposed to work but at the very least they should not throw errors when focused. The global search is supposed to search both mail and chat I believe so regardless of a chat account being setup, the user should still be able to use it.

Note: They still appear to work with the error being thrown but as I mentioned before it looks like most of the MC code expects browser.selectedBrowser to always be set so returning null might cause further problems down the road, maybe.

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to Lasana Murray from comment #14)

Magnus, it's not 100% clear to me how the global search bars are supposed to work but at the very least they should not throw errors when focused. The global search is supposed to search both mail and chat I believe so regardless of a chat account being setup, the user should still be able to use it.

Sure, but if they do not have a chat account set up, searching from the chat tab seems quite of an edge case, even if ideally it would work.