1592172 - Don't reuse tip results or try to update the overflow state of their nonexistent URLs

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Drew Willcoxon :adw]

+++ This bug was initially created as a clone of Bug #1591327 +++

This error happens repeatedly as I type to trigger my tip provider:

tipButton is undefined UrlbarView.jsm:976
    _updateIndices resource:///modules/UrlbarView.jsm:976
    _updateResults resource:///modules/UrlbarView.jsm:739
    onQueryResults resource:///modules/UrlbarView.jsm:440
    notify resource:///modules/UrlbarController.jsm:602
    receiveResults resource:///modules/UrlbarController.jsm:193
    notifyResults resource:///modules/UrlbarProvidersManager.jsm:401
    add resource:///modules/UrlbarProvidersManager.jsm:407
    add self-hosted:870
    onSearchResult resource:///modules/UrlbarProviderUnifiedComplete.jsm:138
    notify resource://gre/modules/UnifiedComplete.jsm:2773
    notifyResult resource://gre/modules/UnifiedComplete.jsm:2788
    _addMatch resource://gre/modules/UnifiedComplete.jsm:2155
    _addSearchEngineMatch resource://gre/modules/UnifiedComplete.jsm:1860
    _matchCurrentSearchEngine resource://gre/modules/UnifiedComplete.jsm:1777
    InterpretGeneratorResume self-hosted:1149
    AsyncFunctionNext self-hosted:688

Comment 1

[Drew Willcoxon :adw]

I think basically we have some oversights in the view for handling tips, places where we need to take them into account but aren't. For example, when I was debugging this earlier, it looked like we might be reusing tip result DOM for other results or vice versa. I'm hoping the JS errors are what's causing the UnifiedComplete results to not show up, but if not, there's probably more to this.

Comment 2

[Drew Willcoxon :adw]

The remaining problems are fixed by updating _rowCanUpdateToResult so that it doesn't allow tips to be reused and _setRowVisibility so that it doesn't try to update tips' nonexistent URLs.

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1592172 - Quantumbar: Recreate result DOM as necessary when reusing rows, and don't try to update the overflow state of nonexistent tip URLs

Comment 4

[Drew Willcoxon :adw]

Resummarizing based on comment 2 and the patch.

Comment 5

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7512e5aee096a7bd369d0155202733d1859a03ce

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4ccc4398453
Quantumbar: Recreate result DOM as necessary when reusing rows, and don't try to update the overflow state of nonexistent tip URLs r=mak

Comment 7

[Andrei Ciure[:aciure]]

Backed out changeset d4ccc4398453 (bug 1592172) for causing browser_updateRows.js to permafail

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=d4ccc43984530b4f687407786d6f1358e6b389bd&selectedJob=274522527

backout: https://hg.mozilla.org/integration/autoland/rev/1dd79924a920934999b6f5e7d50b4c42dff6d5ce

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27be25a45501
Quantumbar: Recreate result DOM as necessary when reusing rows, and don't try to update the overflow state of nonexistent tip URLs r=mak

Comment 9

[Drew Willcoxon :adw]

My test was bitrotted by bug 1589923. Should be fixed now.

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/27be25a45501

Comment 11

[Drew Willcoxon :adw]

Beta try push in preparation for an uplift request: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae3b974e70d50fc707abc3998840d4fde545d54a

Comment 12

[Drew Willcoxon :adw]

Looks like this is causing a timeout in browser_ext_urlbar.js on beta. I'll investigate.

Comment 13

[Drew Willcoxon :adw]

Ah, we'll want to uplift bug 1582339 first. That's why the test times out.

Comment 14

[Drew Willcoxon :adw]

Another beta try push with this and bug 1582339: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38c156dab477773c35c656fe134f94fe1ddc58ee

Comment 15

[Drew Willcoxon :adw]

Another beta try push since the last one didn't land properly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8afb1a19d56d5fd98cc3b8c68fad1e3c810af2cc

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 9104827 [details]
Bug 1592172 - Quantumbar: Recreate result DOM as necessary when reusing rows, and don't try to update the overflow state of nonexistent tip URLs

Beta/Release Uplift Approval Request

• User impact if declined: We'd like the option of running quantumbar experiments on 71 that require this patch (bug 1564506, bug 1568594). Otherwise we'd need to wait until 72.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This isn't a trivial patch, and it touches code that shows results in the urlbar. However, I marked this as low risk because (1) it only fixes a bug and doesn't add new features; (2) the fix is relevant only when tip results are present, and tip results would only be shown by experiment extensions and are not shown in normal Firefox; and (3) this patch has a test, and the urlbar is already well tested, and there are no relevant test failures on a try push (on beta): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae3b974e70d50fc707abc3998840d4fde545d54a
• String changes made/needed:

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9104827 [details]
Bug 1592172 - Quantumbar: Recreate result DOM as necessary when reusing rows, and don't try to update the overflow state of nonexistent tip URLs

Low risk, bug 1582339 is uplifted now, uplift approved for 71 beta 9, thanks.

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/271f87528b86

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.