Closed Bug 1647881 Opened 1 year ago Closed 11 months ago

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

Categories

(Firefox :: Address Bar, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 80
Iteration:
80.1 - June 29 - July 12
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox77 --- unaffected
firefox78 --- unaffected
firefox79 --- wontfix
firefox80 --- fixed

People

(Reporter: harry, Assigned: harry)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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.

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?

Blocks: 1645521
Flags: needinfo?(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.

Flags: needinfo?(mak)
Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Iteration: --- → 79.2 - June 15 - June 28
Severity: -- → S2

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.

Iteration: 79.2 - June 15 - June 28 → 80.1 - June 29 - July 12

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

Flags: needinfo?(htwyford)

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.

Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b306b83c0d1
Store only a subset of fixupInfo in the queryContext. r=mak
Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aceb6f9acf3b
Store only a subset of fixupInfo in the queryContext. r=mak
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 80 → ---
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/851d9df46723
Store only a subset of fixupInfo in the queryContext. r=mak
Status: REOPENED → RESOLVED
Closed: 1 year ago11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80
You need to log in before you can comment on or make changes to this bug.