1675976 - Some urlbar.picked.searchmode.* probes can record bogus data

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Harry Twyford]

The urlbar.picked.searchmode.* probes are incremented when a result is picked and the Urlbar is in search mode. That check is made here. Unfortunately, when a result that enters search mode is picked, the Urlbar is already in search mode preview when we arrive at that line. A bogus entry is recorded. This affects:
urlbar.picked.searchmode.tabtosearch
urlbar.picked.searchmode.tabtosearch_onboard
urlbar.picked.searchmode.keywordoffer
urlbar.picked.searchmode.topsites_urlbar

I manually tested the other probes and they are unaffected. This includes urlbar.picked.searchmode.shortcut, which I understand is the one Product is most interested in. To fix this problem, we need to add a check that search mode is confirmed when those probes are incremented. I already have a patch ready.

Since users must pick a result to trigger the keywordoffer and topsites_urlbar search mode entry points, they always record a bogus entry in their urlbar.picked.searchmode.* probe. Thus, we can find their true values by subtracting the summed value of their urlbar.searchmode.* equivalent (e.g. sum(urlbar.picked.searchmode.keywordoffer) - sum(urlbar.searchmode.keywordoffer)).

The true values of the tab-to-search probes are harder to discern. When the user tabs to the tab-to-search result and starts typing, we do not consider the tab-to-search result "picked" and so we don't record the bogus data. However if the user clicks the tab-to-search result or highlights it and presses Enter, the bogus data is recorded.

EDIT: The crossed out information above is inaccurate. See comment 5.

Comment 1

[Harry Twyford]

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

Comment 2

[Harry Twyford]

Attachment: Bug 1675976 - Stop recording bogus data for some urlbar.picked.searchmode.* probes. r?mak!

Comment 3

[Marco Bonardo [:mak]]

(In reply to Harry Twyford [:harry] from comment #0)

Since users must pick a result to trigger the keywordoffer and topsites_urlbar search mode entry points, they always record a bogus entry in their urlbar.picked.searchmode.* probe. Thus, we can find their true values by subtracting the summed value of their urlbar.searchmode.* equivalent (e.g. sum(urlbar.picked.searchmode.keywordoffer) - sum(urlbar.searchmode.keywordoffer)).

Can you better explain this? If .picked.searchMode. is only incremented when we are in confirmed search mode, and those results don't appear in search mode usually (I can't think of a searchMode showing shortcuts or top sites, as well as tab-to-search).

Comment 4

[Teon Brooks [:teon]]

so IIUC keywordoffer and topsites_urlbar are incremented on the searchmode preview but the others are not?

I think I'm not following what's happening with tabtosearch and tabtosearch_onboard.

Comment 5

[Harry Twyford]

Sorry everyone, comment 0 is wrong. The same issue exists with all four affected probes, not just tabtosearch and tabtosearch_onboard.

(In reply to Marco Bonardo [:mak] from comment #3)

Can you better explain this? If .picked.searchMode. is only incremented when we are in confirmed search mode, and those results don't appear in search mode usually (I can't think of a searchMode showing shortcuts or top sites, as well as tab-to-search).

The issue is that currently urlbar.picked.searchMode.* is incremented when in confirmed search mode or in search mode preview. The attached patch makes it so we only record search mode telemetry when we're in confirmed search mode.

(In reply to Teon Brooks [:teon] from comment #4)

so IIUC keywordoffer and topsites_urlbar are incremented on the searchmode preview but the others are not?

I think I'm not following what's happening with tabtosearch and tabtosearch_onboard.

The four probes listed in comment 0 enter search mode after the user interacts with a result. For keywordoffer, it's (in most cases) the engines that appear when you type @. For topsites_urlbar, it's any search shortcut Top Site. For the tabtosearch probes, to a tab-to-search result. There are two ways to enter search mode using one of these results:

1. Clicking it/selecting it and pressing Enter
2. Selecting it with the keyboard and then just starting to type your query.

When the user does (1), we hit the code path that records our urlbar.picked telemetry, because we consider that result to be picked. Picking one of these results enters search mode. Unfortunately, the telemetry recording event happens after we enter search mode, so we think that it was a result picked within search mode. urlbar.picked.searchmode.* is incremented even though those results were picked outside of search mode.

With (2), the result isn't picked, so we never record in urlbar.picked.*.

The other probes are unaffected because they don't require an interaction with a result to enter search mode.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Harry Twyford [:harry] from comment #5)

There are two ways to enter search mode using one of these results:

1. Clicking it/selecting it and pressing Enter
2. Selecting it with the keyboard and then just starting to type your query.

When the user does (1), we hit the code path that records our urlbar.picked telemetry, because we consider that result to be picked. Picking one of these results enters search mode. Unfortunately, the telemetry recording event happens after we enter search mode, so we think that it was a result picked within search mode. urlbar.picked.searchmode.* is incremented even though those results were picked outside of search mode.

It sounds like a bug we should fix, to me. We can either annotate some info in the .searchMode object to not count it as picked, or, even better, we can just assume one can't "pick" one of those results in search mode, since they are not supposed to appear there.
I don't think we should rely on Data to filter those out.

Comment 7

[Harry Twyford]

The attached patch does fix the issue. Those results don't appear in confirmed search mode. By checking that we're in confirmed search mode, we'll never record those results in urlbar.picked.searchmode.*. Or do you mean changing the order that these events fire?

Comment 8

[Marco Bonardo [:mak]]

(In reply to Harry Twyford [:harry] from comment #7)

The attached patch does fix the issue. Those results don't appear in confirmed search mode.

You said:

Unfortunately, the telemetry recording event happens after we enter search mode, so we think that it was a result picked within search mode. urlbar.picked.searchmode.* is incremented even though those results were picked outside of search mode.

From how it's explained, it sounds like the patch is not going to fix this, because search mode is already confirmed when we record the telemetry.

I am saying that IF this happens also with the patch, it is a bug we should fix. Not changing the order of events, but just not marking those as picked in search mode.

To me it's unclear which problems are left after this patch, and why we couldn't fix them.

Comment 9

[Harry Twyford]

(In reply to Marco Bonardo [:mak] from comment #8)

(In reply to Harry Twyford [:harry] from comment #7)

The attached patch does fix the issue. Those results don't appear in confirmed search mode.

You said:

Unfortunately, the telemetry recording event happens after we enter search mode, so we think that it was a result picked within search mode. urlbar.picked.searchmode.* is incremented even though those results were picked outside of search mode.

Sorry, I should have been more clear: the telemetry recording event happens after we enter search mode preview. Clicking a result that enters search mode does the following, in this order:

1. The result is selected, which calls setValueFromResult.
2. setValueFromResult previews search mode.
3. We record *.picked.* telemetry
4. We confirm search mode.

So the issue is that we're in search more preview when we're recording urlbar.picked.searchmode.*. By only recording urlbar.picked.searchmode.* when search mode is confirmed, this issue is resolved.

I'll do some additional manual testing to make sure before landing.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b8e83a62b10
Stop recording bogus data for some urlbar.picked.searchmode.* probes. r=mak

Comment 11

[Marco Bonardo [:mak]]

(In reply to Harry Twyford [:harry] from comment #9)

So the issue is that we're in search more preview when we're recording urlbar.picked.searchmode.*. By only recording urlbar.picked.searchmode.* when search mode is confirmed, this issue is resolved.

Thank you for clarifying.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0b8e83a62b10

Comment 13

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

The patch landed in nightly and beta is affected.
:harry, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.