1430416 - The findbar (Ctrl + F) occasionally stops working in previously discarded tabs

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Sebastian Schrader]

I'm using the new tabs.discard API in Firefox 58 (b15) and noticed that the findbar (Ctrl + F) stops working in some of the previously discarded tabs, after they are reactivated. When pressing Ctrl + F or accessing the menu option in the Edit menu, nothing happens and the browser console shows the following two errors in this order:

Empty string passed to getElementById().
findbar.xml:277:14

TypeError: this.browser is null
findbar.xml:598:11

The issue is difficult to forcefully reproduce, because not in every discarded tab the findbar is broken. But it happens frequently enough during typical browser sessions.
I couldn't find any way to restore the functionality without closing the tab. Navigating to different pages did not help.

I'm using Richard Neomy's Auto Tab Discard extension, which he developed specifically for the new API, but I hardly believe, that his implementation can break core browser functionality, such as findbar, which is AFAIK not accessible to WebExtensions.

https://addons.mozilla.org/en-US/firefox/addon/auto-tab-discard/

Comment 1

[Ciprian Muresan [:cmuresan], Ecosystem QA]

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID 	20180116220110

I have managed to reproduce this issue on the latest Nightly 59.0a1 build on Windows 10 x64, Mac 10.12 and Arch Linux. I've opened multiple tabs, discarded them, switched to each tab, tried to open the Find toolbar and for me most of them did not open the toolbar.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: hidetabs.xpi

100% str on osx.

- Load extension in about:debugging.
- load another tab (mozilla.org)
- search for some text in that tab (cmd+F) that exists so something is highlighted, preferably a recurring word.
- switch back to about:debugging
- open browser action panel, scroll down to "discard all" and click it (panel does not close)
- switch back to mozilla.org, note it reloads
- cmd+G to "search again"

expected:

cmd+G finds the next occurance

failure:

errors in console and find does not work.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1430416 unset the findbar when discarding a browser tab,

Review commit: https://reviewboard.mozilla.org/r/215436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215436/

Comment 4

[Shane Caraveo (:mixedpuppy)]

We should probably uplift this since any use of the discard api will break find when the browser is restored.

Comment 5

[:Gijs (he/him)]

Mike's a better reviewer here as he knows more about find bar code.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8945209 [details]
Bug 1430416 unset the findbar when discarding a browser tab,

https://reviewboard.mozilla.org/r/215436/#review221608

r=me with comment addressed. Thanks for fixing this!

::: browser/base/content/tabbrowser.xml:2572
(Diff revision 1)
>              this._tabListeners.delete(tab);
>              this._tabFilters.delete(tab);
>  
> +            // Reset the findbar and remove it if it is attached to the tab.
> +            if (tab._findBar) {
> +              tab._findBar.close();

Please pass in `true` here, so that we ensure that the findbar is not animated out of view.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8945209 [details]
Bug 1430416 unset the findbar when discarding a browser tab,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215436/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8786e17ec63b
unset the findbar when discarding a browser tab, r=mikedeboer

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/8786e17ec63b

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Andrei, can your team verify this next week some time? 
Shane, I'd like to uplift the fix to 59 beta if you think it is not too risky.

Comment 11

[Petruta Horea [:phorea]]

I've reproduced the issue using Firefox 59 beta 5 with both comment 0 and comment 2, across platforms.

Using latest Nightly 60.0a1, Ctrl + F works after discarding tabs and there are no errors in browser console, however with Ctrl + G the find bar is not displayed again and the first occurrence from the page will be highlighted, not the next one. 
Ctrl + F shows the find bar and the next occurrence.

Shane, is this expected? Thank you!

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #11)
> I've reproduced the issue using Firefox 59 beta 5 with both comment 0 and
> comment 2, across platforms.
> 
> Using latest Nightly 60.0a1, Ctrl + F works after discarding tabs and there
> are no errors in browser console, however with Ctrl + G the find bar is not
> displayed again and the first occurrence from the page will be highlighted,
> not the next one. 
> Ctrl + F shows the find bar and the next occurrence.
> 
> Shane, is this expected? Thank you!

I just did a quick test without discard.  If you start a find (ctrl-f), hide the find bar, then do find-again (ctrl-g), the bar is not shown again.  If you then use ctrl-f again, the find bar is shown.  

I'm not concerned that the find is started again after discard (rather than continuing) or that ctrl-g does not re-show the find bar, so long as ctrl-f does show the find bar.  I just tested on osx, ctrl-g does show the find bar again.  Perhaps that is somehow platform dependent.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8945209 [details]
Bug 1430416 unset the findbar when discarding a browser tab,

Approval Request Comment
[Feature/Bug causing the regression]: webextension discard api
[User impact if declined]: find fails on a tab that is discarded then restored
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes (str in comments)
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low to moderate
[Why is the change risky/not risky?]: requires a webextension to discard a tab before the code path is used.  I don't see how that code path would fail, but if it did, a tab would be left in a state where it is not discarded but some items may be disabled on it.
[String changes made/needed]: none

Comment 14

[Petruta Horea [:phorea]]

Thanks! Marking 60 as verified based on above comments.

Comment 15

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8945209 [details]
Bug 1430416 unset the findbar when discarding a browser tab,

Fix for a regression with find, verified in nightly. Let's uplift for 59 beta 6.

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fad47dbabc01

Comment 17

[Petruta Horea [:phorea]]

Verified as fixed using Firefox 59 beta 7 under Win 10 64-bit and Mac OS X 10.13.