Closed Bug 1898437 Opened 2 months ago Closed 2 months ago

Could not record event: TypeError: can't access property "providerName" on re-submitting a search link

Categories

(Firefox :: Address Bar, defect, P1)

Firefox 128
Desktop
All
defect

Tracking

()

VERIFIED FIXED
128 Branch
Tracking Status
firefox-esr115 --- unaffected
firefox126 --- unaffected
firefox127 --- unaffected
firefox128 --- verified
firefox129 --- verified

People

(Reporter: cbaica, Assigned: klubana)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [sng][search-regression])

Attachments

(1 file)

Found in

  • Fx 128.0a1

Affected versions

  • Fx 128.0a1

Affected platforms

  • all

Steps to reproduce

  1. Launch Firefox.
  2. Perform a search using the address bar and wait for the SERP to load.
  3. Click the address bar again and re-submit the search link.
    3.1.(if needed) Click the address bar, hit escape to have the link displayed instead of the persistent search term.

Expected result

  • SERP is reached again without any displayed errors in the console.

Actual result

  • The following error is displayed in the browser console.
Could not record event:  TypeError: can't access property "providerName", details.result is undefined

Regression range

  • Looking for a regression ASAP.

Additional notes

  • This occurs for all the search engines.
Could not record event:  TypeError: can't access property "providerName", details.result is undefined
    #notifyEngagement resource:///modules/UrlbarProvidersManager.sys.mjs:454
    notifyEngagementChange resource:///modules/UrlbarProvidersManager.sys.mjs:420
    #internalRecord resource:///modules/UrlbarController.sys.mjs:983
    record resource:///modules/UrlbarController.sys.mjs:843
    handleNavigation resource:///modules/UrlbarInput.sys.mjs:773
    handleCommand resource:///modules/UrlbarInput.sys.mjs:624
    handleKeyNavigation resource:///modules/UrlbarController.sys.mjs:345
    _on_keydown resource:///modules/UrlbarInput.sys.mjs:4091
    maybeDeferEvent resource:///modules/UrlbarEventBufferer.sys.mjs:159
    _on_keydown resource:///modules/UrlbarInput.sys.mjs:4090
    handleEvent resource:///modules/UrlbarInput.sys.mjs:588

I think this may be a regression from bug 1857236. Specifically this line. It looks like in most other cases, we account for details.result potentially being undefined.

However, in looking at the possible effects of this, I'm also slightly confused by the architecture.

Looking up the stack, there's TelemetryEvent.record which is documented as being for recording telemetry, so I was then expecting the onEngagement functions to be reporting event telemetry, but that doesn't appear to be the case - it seems to be for tidying up/performing actions on the providers themselves.

It looks like some of the reason may be due to letting record() figure out about possible re-entrancy, or when we're not actually finishing the session. Though it seems a bit dangerous if telemetry was to throw, then we wouldn't handle the onEngagement calls.

This might be something worth considering in a different bug though, I just thought I'd raise it here as I found it confusing when I looked at this.

Flags: needinfo?(mak)
Flags: needinfo?(klubana)

The architecture is a historical incident, it was supposed to register telemetry, but then it grew up being a events-handler kind of object, indeed we have a bug on file to rename TelemetryEvent object in bug 1827766. Short, the TelemetryEvent name is misleading and a red herring.
onEngagement means engaging with a result, the telemetry event is named similarly because it represents the same happening, but that's where the relation ends.

That said, notifyEngagementChange should not interrupt telemetry recording.
We should unify the code paths in https://searchfox.org/mozilla-central/rev/a44891c52387ca4bd7c35b50f0d335f3980ef36a/browser/components/urlbar/UrlbarController.sys.mjs#955-983 inverting the if and converging to a single final call to notifyEngagementChange. We should probably try/Catch it too.

Finally, it looks like we didn't consider the "missing result" case. The problem is, who do we notify? onEngagement is usually sent to the provider who generated the picked result.
We can start using this._resultForCurrentValue here (in handleNavigation), though there will still be cases where the input just decides on its own what to do (no provider returned anything usable in time), then probably we should not notify any provider. notifyEngagementChange should just skip onEngagement if result is not defined.

Flags: needinfo?(mak)
Priority: -- → P1
Whiteboard: [sng][search-regression]

This also affects the user any time they select "Paste & Go" as seen in https://bugzilla.mozilla.org/show_bug.cgi?id=1886140

Set release status flags based on info from the regressing bug 1857236

Assignee: nobody → klubana
Status: NEW → ASSIGNED
Pushed by klubana@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3264bce58de
Fix TypeError on re-submitting a search link by ensuring details.result is defined. r=mak
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 128 Branch

Issue is reproducible on a 2024-05-23 Nightly build on Windows 10.
Verified as fixed on Firefox 128.0b1 and Firefox Nightly 129.0a1 on Windows 10, Ubuntu 22, macOS 14.

Status: RESOLVED → VERIFIED
Flags: needinfo?(klubana)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: