Closed Bug 1430416 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P2)

58 Branch
x86_64
All
defect

Tracking

(firefox58 wontfix, firefox59+ verified, firefox60 verified)

VERIFIED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 + verified
firefox60 --- verified

People

(Reporter: sebastian.schrader, Assigned: mixedpuppy)

Details

Attachments

(2 files)

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/
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → WebExtensions: Untriaged
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Toolkit
Hardware: Unspecified → x86_64
Assignee: nobody → mixedpuppy
Priority: -- → P2
Attached file 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.
We should probably uplift this since any use of the discard api will break find when the browser is restored.
Attachment #8945209 - Flags: review?(gijskruitbosch+bugs) → review?(mdeboer)
Mike's a better reviewer here as he knows more about find bar code.
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.
Attachment #8945209 - Flags: review?(mdeboer) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8786e17ec63b
unset the findbar when discarding a browser tab, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/8786e17ec63b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(andrei.vaida)
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!
Flags: needinfo?(andrei.vaida)
(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.
Flags: needinfo?(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
Attachment #8945209 - Flags: approval-mozilla-beta?
Thanks! Marking 60 as verified based on above comments.
Status: RESOLVED → VERIFIED
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.
Attachment #8945209 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed using Firefox 59 beta 7 under Win 10 64-bit and Mac OS X 10.13.
Flags: qe-verify+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.