1647881 - Extension results aren't returned because XPCOM URIs cannot be sent to extensions

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Harry Twyford]

Bug 1628079 added fixupInfo from URIFixup to UrlbarQueryContext. This allows us to cache the fixupInfo for a given search string, allowing us to one day reduce the number of calls to URIFixup. This is already paying off in the patch for bug 1645521, which eliminates a call to URIFixup previously made in UnifiedComplete.

The issue is that fixupInfo contains XPCOM URIs. UrlbarProviderExtension serializes the queryContext and sends it to registered extensions, but XPCOM objects can't be sent to extensions. This means UrlbarProviderExtension throws when it tries to communicate with its extensions and no results are returned.

This hasn't become an issue because as of this filing, queryContext.fixupInfo is only initialized in UrlbarProviderSearchSuggestions. We don't have any tests that call both UrlbarProviderExtensions and UrlbarProviderSearchSuggestions.

This bug blocks bug 1645521 because the new UrlbarProviderHeuristicFallback initializes queryContext.fixupInfo before UrlbarProviderExtensions sends the queryContext to its extension, i.e. the queryContext contains XPCOM URIs by the time it is sent to an extension.

Comment 1

[Harry Twyford]

Consumers of queryContext.fixupInfo probably don't need all the information URIFixup provides. Right now, UrlbarProviderSearchSuggestions just checks keywordAsSent and fixedURI as booleans. My WIP patch for bug 1645521 uses fixedURI.scheme, fixedURI.displaySpec, and fixedURI.pathQueryRef.

Instead of storing the entire fixupInfo object in the queryContext, we could store a custom object with only the properties we need. For example, we modify this to be

let info = Services.uriFixup.getFixupURIInfo(
    this.searchString.trim(),
    flags
);
this._fixupInfo = {
    uriScheme: info.fixedURI.scheme,
    uriDisplaySpec: info.fixedURI.displaySpec,
    uriPathQueryRef: info.fixedURI.pathQueryRef,
    keywordAsSent: info.keywordAsSent,
};

Another approach could be to recast fixedURI to a URL in UrlbarQueryContext's copy of fixupInfo. Or we could just simply delete fixupInfo from the queryContext sent to extensions.

Marco, do you have any suggestions here? All the proposed solutions above have their drawbacks; do you prefer one?

Comment 2

[Marco Bonardo [:mak]]

I think it's fine to pick only things we care about! It's a good point that we should avoid storing XPCOM objects in the queryContext.
I'd not go that far as defining one property for each url piece, it seems simpler to just store a couple properties like context.fixup.url (URL object) and context.fixup.isSearch... unless we need something that is in nsIURI but not in URL, but even then it would be better to ask netwerk to add chrome-only APIs to URL, if possible.

Comment 3

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

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

Comment 4

[Harry Twyford]

Attachment: Bug 1647881 - Store only a subset of fixupInfo in the queryContext. r?mak!

Comment 5

[Harry Twyford]

This is done, but will land in a stack with bug 1645521, which is blocked by bug 1648468. Bumping the iteration since it won't land until bug 1648468 is done.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Is there a user-facing impact from this issue? Do we need to fix this in Beta79 too?

Comment 7

[Harry Twyford]

No, we're not running any experiments that use extension results right now/in 79, so it's okay if this doesn't get uplifted.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b306b83c0d1
Store only a subset of fixupInfo in the queryContext. r=mak

Comment 9

[Dorel Luca [:dluca]]

Backed out 5 changesets (bug 1647881, bug 1645521, bug 1645324) for BC failures in urlbar/tests/browser/browser_autocomplete_a11y_label.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309084323&repo=autoland&lineNumber=8517

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=65f90856987542a1bcdcdc582274f5bd16e2f5fa

Backout:
https://hg.mozilla.org/integration/autoland/rev/d71dd7660462b2aa8ec528d6df3b5a4591d45c39

Comment 10

[Harry Twyford]

See bug 1645521 comment 6.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aceb6f9acf3b
Store only a subset of fixupInfo in the queryContext. r=mak

Comment 12

[Narcis Beleuzu [:NarcisB]]

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

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out: https://hg.mozilla.org/integration/autoland/rev/863e463684183a334f15b465d932372e62d24a4f

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/851d9df46723
Store only a subset of fixupInfo in the queryContext. r=mak

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/851d9df46723