Closed Bug 1592172 Opened 8 months ago Closed 8 months ago

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

Categories

(Firefox :: Address Bar, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 72
Iteration:
72.1 - Oct 21 - Nov 3
Tracking Status
firefox70 --- unaffected
firefox71 --- fixed
firefox72 --- fixed

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

+++ 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

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.

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.

Resummarizing based on comment 2 and the patch.

Points: 3 → 2
Summary: Investigate remaining failures when using tip results from extensions → Don't reuse tip results or try to update the overflow state of their nonexistent URLs
Attachment #9104827 - Attachment description: Bug 1592172 - Quantumbar: Don't reuse tip results or try to update the overflow state of their nonexistent URLs. → Bug 1592172 - Quantumbar: Recreate result DOM as necessary when reusing rows, and don't try to update the overflow state of nonexistent tip URLs
Pushed by dwillcoxon@mozilla.com:
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
Pushed by dwillcoxon@mozilla.com:
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

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

Flags: needinfo?(adw)
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
Flags: qe-verify-
Flags: in-testsuite+

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

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

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

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:
Attachment #9104827 - Flags: approval-mozilla-beta?

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.

Attachment #9104827 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Keywords: regression
You need to log in before you can comment on or make changes to this bug.