1591327 - Properly limit results to maxResults while taking result spans into account.

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Drew Willcoxon :adw]

This is a vague bug report, but I'm not sure what's going on yet. I'll clarify it and file other bugs as necessary as I investigate it.

I have a stub interventions extension to test the query categorization module I'm writing. All it does is register a provider that creates a tip result whose text depends on the matched category. In debugging this, I also tested with a bare-bones provider that creates a static tip, and the problems still happen.

When my provider returns the "active" behavior (so that the tip is interleaved in the normal results), the view flickers and doesn't display any results as I type. If the provider's behavior is "restricting", the tip appears by itself as expected, but when the provider later returns "inactive" (because the search string doesn't match a category), the normal heuristic result has the tip's button in it. And then when the tip is shown later, it sometimes has the "-- Search with ..." text in it.

There are a bunch of errors in the console as this is all happening.

Comment 1

[Drew Willcoxon :adw]

Returning interleaved results with types URL and SEARCH works correctly, so this does seem to be related to tips in particular.

Comment 2

[Drew Willcoxon :adw]

There's something wrong with the handled logic in the muxer. The muxer's sort is called many times for the same context as results are added. The handled set is recreated each time. So we end up "handling" some of the same results several times per context.

For normal results, that doesn't seem to be a problem. For tip results, their span is 3, so when sort does context.maxResults -= UrlbarUtils.getSpanForResult(result) - 1, it decreases the maxResults by 2, and since the tip result is handled more than once, maxResults ends up getting decreased too much, eventually to zero, which is why the view ends up empty. For normal results, UrlbarUtils.getSpanForResult(result) - 1 is just zero, so there's no problem there.

Not sure yet what exactly we should be doing here, whether the handled logic needs to be fixed, or maybe we shouldn't be decreasing maxResults at all. I thought that saving the handled set per context would fix it, but it doesn't.

Comment 3

[Marco Bonardo [:mak]]

why are we changing context.maxResults in the muxer? that looks wrong, it's just an information that the context is providing, modifying it doesn't seem to make any sense.

Comment 4

[Marco Bonardo [:mak]]

If the scope is so we won't fetch more results than expected, that was not intended to work like that, the muxer will just cut an adequate number of results, returning more results from providers is absolutely fine, up to maxResults. Indeed I'd consider it a bug, if the muxer decides to throw away one of these large resultSpan results, it would not have other results to pick from.

Comment 5

[Drew Willcoxon :adw]

Yes, decreasing context.maxResults definitely is wrong, but what I was less clear on is whether the handled logic is working incorrectly, too. On closer look, I think it's working fine. Note also that UrlbarProvidersManager is the piece that crops the results to maxResults. So the right fix is to move the result-span checking into UrlbarProvidersManager's cropping afaict.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #5)

Yes, decreasing context.maxResults definitely is wrong, but what I was less clear on is whether the handled logic is working incorrectly, too. On closer look, I think it's working fine. Note also that UrlbarProvidersManager is the piece that crops the results to maxResults.

ah right, the muxer sorts and then the PM crops. There's no strict reason about that split, both solutions would be fine. I suspect I put the cropping in the pm so that the muxer had less responsibilities :shrug:.

Comment 7

[Drew Willcoxon :adw]

The reason I'm hitting the maxResults problem but browser_urlbar_resultSpan.js doesn't is related to how many times the muxer's sort method is called. browser_urlbar_resultSpan.js adds one restricting provider, so it effectively disables UnifiedComplete, and it adds all its results synchronously. The provider's type is not immediate. Therefore, sort is called only once per search, when the _chunkTimer in the providers manager fires. Because sort is called only once, subtracting the tip's result span in the muxer is harmless.

The UnifiedComplete provider alone also doesn't have this problem, but for a different reason. The UnifiedComplete provider is immediate, so sort is called for each result in the search. But of course UnifiedComplete doesn't have tip results, so it's not affected.

But when you have a non-restricting extension provider, you have async results from the extension plus UnifiedComplete, so sort gets called multiple times per search.

Comment 8

[Drew Willcoxon :adw]

Attachment: Bug 1591327 - Quantumbar: Properly limit results to maxResults while taking result spans into account.

The muxer shouldn't decrease maxResults. The cropping in the providers manager should take result spans into account.

I updated the result-span test to also check a non-restricting provider, which triggers this bug.

Comment 9

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #0)

When my provider returns the "active" behavior (so that the tip is interleaved in the normal results), the view flickers and doesn't display any results as I type.

This patch kind of fixes this problem. Now the tip is shown, but it doesn't look right at first, and other results from UnifiedComplete that should be shown aren't. And there are still the JS errors in the view. I'll morph this bug to be about the maxResults problem and file a new one for investigating the remaining problems.

Comment 10

[Drew Willcoxon :adw]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf21b0dd7d7a
Quantumbar: Properly limit results to maxResults while taking result spans into account. r=mak

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cf21b0dd7d7a

Comment 13

[Drew Willcoxon :adw]

Attachment: Beta 71 patch

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 is a bug fix that properly limits the number of results shown in the quantumbar when special "tip" results are present. Tip results would only be shown by experiment extensions and are not shown in normal Firefox.
• String changes made/needed:

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9105073 [details] [diff] [review]
Beta 71 patch

Covered by tests, needed for 71 experiments and evaluated as low risk by the developer, uplift approved for 71 beta 6, thanks.

Comment 15

[Dorel Luca [:dluca]]

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