2051959 - [IPC Urlbar] Phase 6: prove the urlbar actor message path end-to-end (pref-on CI variant + wire-safety fixes)

Status: ASSIGNED

(Firefox :: Address Bar, task, P1)

Description

[Dão Gottwald [:dao]]

Phase 6 of IPC Urlbar (meta bug 2049112). Phases 0-5 build the parent/child controller split and carry queries, results, and telemetry across the Urlbar JSWindowActor pair; this phase proves the whole contract end-to-end and greens up the remaining message-path-only gaps.

The message path (browser.urlbar.ipc.chromeMessagePassing, off by default -- also the path a content-process <moz-urlbar> such as about:newtab will use) isn't exercised by any default CI run. This bug adds a dedicated pref-on CI variant that runs the chrome <moz-urlbar> mochitest-browser-chrome suite over the wire, and tracks the fixes needed to get it green. The default in-process chrome path is unaffected throughout.

Pref-on CI variant (the validation harness): a urlbar-ipc test variant forces browser.urlbar.ipc.chromeMessagePassing=true over the urlbar suite (scoped via MOZHARNESS_TEST_TAG), tier 3, with a mozinfo: urlbar_ipc per-test escape hatch. Running it locally surfaced the gaps below.

Gaps to green up:

1. Wire-safe result payloads. Payloads can carry values that aren't plain, structured-clone-safe data, so they don't survive the actor boundary:

   • ActionsResult class instances embedded in payloads by UrlbarProviderGlobalActions (consumed by its parent-side getViewUpdate). Structured-clone drops their prototype, so the parent-side dynamic getViewUpdate builds the row from broken action data (observed as a dynamic getViewUpdate returning an undefined l10n id).
   • Dynamic-suggestion payloads carrying a suggestionObject, which fails JsonSchemaValidator over the wire (Object has unexpected property 'suggestionObject').

   Options: demote ActionsResult to a typedef + plain objects, or carry the view-update data in the result (as Phase 4 pre-fetches getViewTemplate/getResultCommands) so live classes stay parent-side and never need serializing.

2. Provider hooks that need content-side state. Several providers' onEngagement/onSelection/pick hooks run in the parent but synchronously read the content-side input/view/selected element, which are null/absent on the message path:

   • UrlbarProviderGlobalActions.onEngagement -- details.element.dataset.action + controller.view.close()
   • UrlbarProviderAutofill.onEngagement -- controller.input._setValue() + controller.input.startQuery() (redo search after autofill dismissal)
   • UrlbarProviderActionsSearchMode -- details.element attributes + documentGlobal
   • UrlbarProviderCalculator -- controller.input.view.startTail150()
   • UrlbarProviderAiChat -- controller.input.inputField / sapLocation / context getters
   • UrlbarProviderQuickSuggestContextualOptIn -- details.element (feature-gated off today)

   Needs a general mechanism to route these hooks' content-side effects back to the child rather than reaching controller.input/controller.view/details.element directly parent-side.

3. Smaller items: a viewData deepEqual leak (message-path results carry a pre-fetched viewData property that breaks tests deep-comparing results), and two crashes (browser_autocomplete_enter_race, browser_searchModeSwitcher_basic).

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Route speculative connect over the actor message path. r?mbeier!

UrlbarChildController.speculativeConnect (called by the view on mousedown)
forwarded to this.#parent.speculativeConnect, which the message-path proxy
didn't implement, so it threw over the wire. Add a SpeculativeConnect C->P
message and the matching proxy method, serializing the result and context like
the other query messages.

The recording itself needs the chrome window, which the parent already resolves
from the actor (on both the direct and message paths), so the guard now keys on
browserWindow rather than input (the latter is null on the message path).
The heuristic "resultsadded" speculative connect still runs only on the direct
path, from receiveResults; only the view-initiated request crosses the wire.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Add a pref-on CI variant exercising the urlbar actor message-passing path. r?daleharvey,mbeier

The message path (browser.urlbar.ipc.chromeMessagePassing) is off by default,
so nothing in CI exercises the wire contract the chrome <moz-urlbar> instances
are meant to prove out. Add a urlbar-ipc test variant that forces the pref on
and runs the urlbar mochitest-browser-chrome suite over the wire, scoped via
MOZHARNESS_TEST_TAG: [urlbar] (the urlbar manifests already tag their tests)
so it doesn't run the whole browser-chrome suite. A mozinfo: urlbar_ipc flag
lets an individual test opt out (skip-if = urlbar_ipc) when it must stay on the
direct path.

Attached to mochitest-browser-chrome as urlbar-ipc (Windows/macOS, where the
plain M(bc) task runs) and webrender-sw+urlbar-ipc (Linux, where the suite runs
under software WebRender), on opt, autoland and mozilla-central, tier 3.

Comment 4

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Keep the message-path view-data cache off the result's enumerable shape. r?daleharvey,mbeier

The pre-fetched per-result view data (viewTemplate/resultCommands) is
attached to each reconstructed result on the message path so the view can read
it synchronously. Attaching it as a plain property left it enumerable, so it
leaked into result comparisons and serialization -- e.g. tests that deepEqual
a view result against an expected one saw a spurious viewData key. Define it
non-enumerable; the proxy still reads it by direct access.

Comment 5

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Make the message-path controller proxy a content module and let the child own controller construction. r?mbeier!

UrlbarParentControllerProxy is the content-side stand-in a
UrlbarChildController holds in place of the real parent controller, so for a
content-process <moz-urlbar> it has to run in the content global. Move it from
a .sys.mjs system module to a content/ .mjs (registered in jar.mn,
dropped from moz.build), and construct it on the content side: the actor's
getOrCreateController returns null on the message path and
UrlbarChildController builds the proxy itself, with the actor only handing out
an instanceId (createMessagePathInstance). Behavior-neutral on both paths in
chrome; the direct path is untouched.

Comment 6

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Use consistent direct-path / message-path terminology in comments. r?mbeier!

The comments named the two transports inconsistently ("direct" vs "in-process",
"message path" vs "message-passing path" vs "wire path"). Settle on "direct
path" (synchronous, real controller reference) and "message path" (actor
messages), since the distinction is the transport, not the process boundary --
the chrome pref-on variant runs the message path in-process. "in-process" /
"cross-process" now appear only where the process boundary itself is the point.

Comment 7

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Resolve the query when the actor is torn down mid-query. r?mbeier!

Making the proxy's startQuery a sendQuery round-trip means its promise
rejects with an AbortError if the actor is destroyed before the query
finishes -- e.g. the window or tab is closed mid-query. Callers store that
promise as lastQueryContextPromise and don't expect it to reject, so it
surfaced as "Actor 'Urlbar' destroyed before query 'StartQuery' was resolved"
uncaught rejections on the message path. Swallow that specific abort and resolve
with the started context (the query is moot once the actor is gone), mirroring
the direct path where startQuery doesn't reject on teardown. Other errors
still propagate.

Comment 8

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Bake the view's per-result data into UrlbarResult. r?mbeier!

The view's synchronous per-result data -- a dynamic result's view template and
a result's menu commands -- was fetched from the result's provider on demand,
which the message path can't do when the provider lives in another process. It
worked around this by pre-fetching the data with each QueryResults notification
and stashing it on a non-enumerable viewData expando that the view read back.

Compute it eagerly instead, when the result is finalized in
UrlbarProvidersManager, and store it on UrlbarResult as viewTemplate and
commands. Both are serialized by toWire()/fromWire(), so the view reads them
synchronously on both transports without a round-trip or the expando (and its
deepEqual/serialization hazard). getViewUpdate stays dynamic and round-trips.

Comment 9

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Tolerate a missing provider in getViewUpdate on the message path. r?mbeier!

getViewUpdate round-trips asynchronously on the message path, so the result's
provider can be gone by the time it runs parent-side (the query was superseded
and the row is being torn down). getProvider() then returned undefined and the
call threw an uncaught rejection. Return nothing in that case and have the view
skip the update, the same way it already bails when the row's result changed
while awaiting.

Comment 10

[Dão Gottwald [:dao]]

Attachment: WIP: Bug 2051959 - [IPC Urlbar] Phase 6: Fire an event when a dynamic result's view update lands. r?daleharvey,mbeier

A dynamic result's view update is applied asynchronously (the view awaits
getViewUpdate), and on the message path that's an actor round-trip, so the
update can land well after the row is shown. Tests that assert on the updated
DOM raced it and failed only on the wire.

Dispatch a DynamicResultUpdated event once the update is applied, add a
UrlbarTestUtils.promiseDynamicResultUpdated helper, and have the highlighting
subtests await it before asserting.

Comment 11

[Dão Gottwald [:dao]]

Attachment: WIP: Bug 2051959 - [IPC Urlbar] Phase 6: Make global-action results wire-safe. r?daleharvey,mbeier

Global-action results (the actions row) carry their per-action data -- key,
l10nId, l10nArgs, icon, dataset, engine -- in ActionsResult objects
held on the result payload. ActionsResult stored that data in private fields
exposed through getters, which a structured clone drops (it copies only own
enumerable data properties), so over the actor message path the actions rendered
blank; only providerName, a public field, survived.

Store the data in plain public fields instead, so the payload stays structured-
cloneable, and freeze the instance to keep it read-only as the getters did.
providerName can no longer be stamped on after construction (the object is
frozen), so each provider passes its own name in; the constructor now requires
providerName and key and throws without them.

The default in-process path is unaffected.

Comment 12

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Make the provider engagement hooks work on the message path. r?mbeier!

Provider onEngagement hooks run parent-side (in UrlbarProvidersManager, from
the parent controller). On the actor message path the parent controller has no
child, so controller.input/controller.view are undefined and details.element
is dropped on the wire, breaking the hooks that reach them (GlobalActions,
Autofill, ActionsSearchMode, Calculator, ContextualSearch).

Resolve the element reads from data that survives the wire: GlobalActions reads
the picked action key from details.pickedActionKey (resolved content-side by
the telemetry snapshot); QuickActions' pickAction and ActionsSearchMode
take the action key and input length from the result payload instead of the
picked element, and ActionsSearchMode recomputes the disabled state from the
action.

Route the input/view side effects to the content side: the parent-side
UrlbarChildControllerProxy exposes input/view stand-ins whose allowed
methods (search, _setValue, startQuery, close, startTail150) forward to
the real content objects as an InvokeContentAction message, so providers keep
calling controller.input.X()/controller.view.X() unchanged. ContextualSearch
passes the engine's name and aliases (all search() reads) as a plain object so
it stays structured-cloneable.

this.input is now a forwarding stand-in on the message path, so the
receiveResults first-result guard checks for the onFirstResult method rather
than mere truthiness (it only runs on the in-process input; the content side runs
onFirstResult itself).

The underscore-prefixed UrlbarInput._setValue is forwarded as-is here; a
follow-up renames it to setValue now that it's part of the actor-facing
surface.

The default in-process path is unaffected.

Comment 13

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Rename UrlbarInput._setValue to setValue. r?mbeier!

_setValue is now part of the controller's actor-facing surface: on the message
path a parent-side provider hook reaches it through controller.input, which
forwards to the real input as an InvokeContentAction message. The underscore
"internal, don't call from outside" convention no longer fits, so drop it.

Pure mechanical rename of the method on UrlbarInput/SmartbarInput and every
caller (UrlbarProviderAutofill, the test utils and tests), plus the message-
path InputProxy forwarder and its allowed-method set.

Comment 14

[Phabricator Automation]

Comment on attachment 9603959 [details]
WIP: Bug 2051959 - [IPC Urlbar] Phase 6: Fire an event when a dynamic result's view update lands. r?daleharvey,mbeier

Revision D310135 was moved to bug 2052555. Setting attachment 9603959 [details] to obsolete.

Comment 15

[Phabricator Automation]

Comment on attachment 9604203 [details]
WIP: Bug 2051959 - [IPC Urlbar] Phase 6: Make global-action results wire-safe. r?daleharvey,mbeier

Revision D310306 was moved to bug 2052554. Setting attachment 9604203 [details] to obsolete.

Comment 16

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Await deferred action effects in the secondary-action tests on the message path. r?mbeier!

Comment 17

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Skip payload re-validation when reconstructing a result over the actor boundary. r?mbeier!

UrlbarResult.fromWire() rebuilds the result through the validating
constructor, which re-runs the dynamic-result payload schema check. But a
result's payload is validated when its provider first creates it, parent-side;
internal fields added after that validation -- e.g. QuickSuggest's
suggestionObject, kept on the payload for dismissal -- are then rejected by the
re-validation on the receiving side, which broke dynamic suggestions on the
message path (JsonSchemaValidatorError on the cloned suggestionObject).

Reconstructed wire results carry already-validated data, so fromWire() skips
the schema validation.

Comment 18

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Search the selected search-mode engine on Enter instead of a stale heuristic. r?mbeier!

In an engine search mode, Enter navigated using the view's heuristic result.
On the message path a search-mode change re-queries asynchronously, so the
auto-selected heuristic can still carry the previously-cycled engine when Enter
fires, navigating to the wrong engine (surfacing as browser_searchModeSwitcher_basic
reaching the real Wikipedia engine and tripping the harness's non-local-connection
guard).

Make the pinned search-mode engine authoritative: when Enter would pick the
heuristic in an engine search mode, search the current engine directly. The
one-off branch's engine-search (query URL + search recording + form history) is
extracted into a shared helper the search-mode path reuses.

Comment 19

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Await the pending query in the actions-search-mode test before selecting on the message path. r?mbeier!

enter_action_search_mode typed a keyword then immediately Tab-selected the
action and pressed Enter, relying on the query producing results synchronously.
Over the Urlbar actor the query round-trips asynchronously, so Enter read the
view selection before the action result was rendered and the action never fired
(the test timed out waiting for about:preferences). Await promiseSearchComplete
after typing so the selection acts on the rendered results.

Comment 20

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Serialize testForceNewContent so the view regenerates rows over the message path. r?mbeier!

On the actor message path a result's testForceNewContent flag was dropped by
toWire(), so the view never regenerated the row's content when a later result
batch arrived and browser_removedSelectedElement saw the previously selected
element stay connected. Add it to the wire form.

The test's whole-result deepEqual only ever compared the view-assigned
expandos (rowIndex/viewTemplate/commands), which diverge over the wire by
design -- the view mutates its own reconstructed copy, not the test's object.
Compare the injected payload instead. Behavior-neutral on the direct path.

Comment 21

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Make result dismissal and feedback acknowledgment work over the message path. r?mbeier!

A provider's onEngagement runs parent-side on the message path, so a dismissal
acts on a result reconstructed from the wire rather than the context's own
instance. Three consequences, fixed here:

• removeResult() located the result via identity (indexOf), which fails for the
  wire copy. It now falls back to the result's rowIndex, which matches the
  results order. Bug 2052875 will replace this with a stable result id.

• The dismissal-acknowledgment l10n was set as an expando on the result, so it
  didn't cross the boundary and made the two copies diverge. Providers now pass
  it to removeResult() via an options object, which forwards it on the
  QUERY_RESULT_REMOVED notification to the view; nothing mutates the result.

• view.acknowledgeFeedback() reached the content view only in-process. Route it
  through the parent-side view stand-in (like close/setValue), and make its
  post-await staleness guard compare the row's own result rather than the wire
  copy (identity against the copy would always fail).

The direct path is behavior-neutral throughout (same instance, so rowIndex ==
indexOf and the options-supplied l10n equals the row result's own).

Because the effect round-trips asynchronously on the message path, the tests
await it -- onQueryResultRemoved via the new
UrlbarTestUtils.promiseControllerNotification, and the feedback attribute -- both
of which resolve synchronously in-process.

Comment 22

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Record a tab-close bounce over the message path. r?daleharvey,mbeier

On the message path the parent records the bounce asynchronously at trigger
time. For a tab-close trigger the tab is gone by the time the trigger message
is handled, so resolving its browser to query Interactions fails and the bounce
goes unrecorded; the other triggers keep the tab alive, so they were fine.

The child now hands the parent the live browser when it starts tracking, and
the parent holds it until the trigger. It's keyed by the stable browser id
rather than the browsing context id: the pick's navigation can replace the
browsing context between tracking and the trigger, so the context id no longer
matches.

Behavior-neutral on the direct path, which resolves the live browser object
synchronously and never takes the message-path trigger.

Comment 23

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Confirm tab-to-search onboarding search mode in the input. r?mbeier!

The onboarding result's search mode was confirmed by the provider's
onEngagement, which reached the input through the picked result's DOM element
(element.documentGlobal.gURLBar). That element doesn't cross the actor boundary,
so the confirmation never ran over the message path and the content-side
searchMode stayed null.

Confirm it in UrlbarInput's pick handling instead -- next to the record() call
in the DYNAMIC branch, gated on providesSearchMode, mirroring the
RESULT_TYPE.SEARCH branch that already confirms its search mode there. It runs
against the live input, so there's no round-trip and no dependence on the DOM
element crossing the boundary; the provider's onEngagement is dropped.

Behavior-neutral on the direct path: same confirmation call, same Glean events
(verified against the urlbar.searchmode telemetry).

Comment 24

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Green up the quickactions tests on the message path. r?daleharvey!

UrlbarProviderGlobalActions.onEngagement closed the view after a pick, but the
content-side pick handling (UrlbarInput.pickResult) already closes it. On the
message path that hook runs parent-side and its close round-trips back
asynchronously, arriving after a freshly-typed query has started and cancelling
it, so the view never opened. Dropping the redundant close is behavior-neutral
on the direct path: every global-action pick is where=current (they open no
URL), so pickResult always closes.

The rest is test-only. A provider engagement hook runs parent-side, so on the
message path a pick's side effect -- the action firing, a page opening, a tab
refocus -- lands asynchronously; tests that asserted it synchronously now await
it. Unchanged on the direct path, where it stays synchronous.

Comment 25

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Await the tip's deferred engagement effects in the search-tips and suppress-focus-border tests on the message path. r?mbeier!

The tip provider's onEngagement (disabling the tip once tipShownCount reaches
the cap) and onSearchSessionEnd (clearing the engagement's tip state) run
parent-side, so over the message path their effects land asynchronously after
the pick, and the test raced them.

Await the effects before asserting. UrlbarTestUtils.waitForPrefValue waits for
an UrlbarPrefs value (checking the current value first, then observing) and
folds the assertion in via its message, so it resolves immediately on the
default in-process path and only actually waits over the wire.

Test-only; the default path is unchanged.

Comment 26

[Dão Gottwald [:dao]]

Attachment: WIP: [IPC Urlbar] Phase 6 collapsed view: urlbar over the actor message path (not for landing)

Squashed, read-only view of the Phase 6 message-path commits, so a reviewer can
see the whole phase as a single diff. Excludes the pref-on CI variant. This
patch is not for landing; the official review happens on the separated commits
in the stack.

Comment 27

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Await the provider's deferred onSelection over the message path. r?mbeier!

A provider's onSelection hook runs parent-side, so over the message path it
lands asynchronously relative to the key navigation that drives the selection,
whereas the direct path runs it synchronously. The test asserted the selection
count immediately after synthesizing the navigation and raced it.

Await the hook instead: the provider resolves a promise once onSelection has
fired for every selected element, which happens synchronously on the default
in-process path.

Test-only; the default path is unchanged.

Comment 28

[Phabricator Automation]

Comment on attachment 9603509 [details]
Bug 2051959 - [IPC Urlbar] Phase 6: Make the message-path controller proxy a content module and let the child own controller construction. r?mbeier!

Revision D309881 was moved to bug 2054066. Setting attachment 9603509 [details] to obsolete.

Comment 29

[Phabricator Automation]

Comment on attachment 9603514 [details]
Bug 2051959 - [IPC Urlbar] Phase 6: Use consistent direct-path / message-path terminology in comments. r?mbeier!

Revision D309883 was moved to bug 2054066. Setting attachment 9603514 [details] to obsolete.

Comment 30

[Phabricator Automation]

Comment on attachment 9603883 [details]
Bug 2051959 - [IPC Urlbar] Phase 6: Bake the view's per-result data into UrlbarResult. r?mbeier!

Revision D310082 was moved to bug 2054070. Setting attachment 9603883 [details] to obsolete.

Comment 31

[Phabricator Automation]

Comment on attachment 9603508 [details]
Bug 2051959 - [IPC Urlbar] Phase 6: Keep the message-path view-data cache off the result's enumerable shape. r?daleharvey,mbeier

Revision D309880 was moved to bug 2054070. Setting attachment 9603508 [details] to obsolete.

Comment 32

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Add an onProviderEngagement notification so content can await a provider's engagement side effect. r?mbeier!

Provider onEngagement hooks run parent-side, so a provider's engagement side
effect (e.g. UrlbarProviderUnitConversion copying its result to the clipboard)
lands on the content side asynchronously over the actor message path, with no
signal content can wait on. Add a controller notification, onProviderEngagement,
fired once the picked result's provider has handled the engagement, so
content-side listeners can await that the side effect has run. It fires
synchronously on the direct path and after the actor round-trip on the message
path, riding the existing controller notification bus.

UrlbarTestUtils.promiseProviderEngagement wraps it for tests.
browser_unitConversion.js awaits it before reading the clipboard, and awaits
the dynamic result's view update (promiseDynamicResultUpdated) so the converted
output is present when the row is read. The notification is behavior-neutral on
the direct path.

Comment 33

[Dão Gottwald [:dao]]

Attachment: Bug 2051959 - [IPC Urlbar] Phase 6: Green up browser_caret_position.js on the message path. r?mbeier!

checkIfKeyStartsQuery detected a key-initiated query by reading a queryStarted
flag synchronously right after the key. The flag is set from the onQueryStarted
controller notification, which round-trips from the parent on the message path,
so it hadn't arrived yet and the query-expected assertions failed. For those
cases, await a promise the notification resolves; the no-query cases still read
synchronously. Test-only; direct path behavior-neutral.

Comment 34

[Phabricator Automation]

Comment on attachment 9603470 [details]
Bug 2051959 - [IPC Urlbar] Phase 6: Route speculative connect over the actor message path. r?mbeier!

Revision D309860 was moved to bug 2054110. Setting attachment 9603470 [details] to obsolete.