1883496 - Move search header interception into Necko

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Roger Yang [:royang]]

Currently we use URL interceptor to decide if we should intercept and add additional headers depending on the URL. However, this resulted in some behaviours that we do not want in corner cases.

1. Toolbar search term is cleared too early,
2. Reload behaviour is modified.

To avoid these problems, move interception behaviour into GeckoEngineSession so we add the additional headers when calling loadUrl.

Comment 1

[Roger Yang [:royang]]

The idea here is when we create the GeckoEngineSession (or after we create it we call a new API), we give GeckoEngineSession a map(or a pair initially) of {}->Boolean and a Map<String, String>. If {}->Boolean return true, then we add the header specified in the map.

This way, we only apply the header when we call loadUrl. then we don't have to intercept anymore

Comment 2

[Brian Grinstead [:bgrins]]

+Valentin for awareness. Another idea would be to bake the tier 1 header specifically directly into Necko (gated behind a pref, or something). But it sounds like this is highlighting an underlying issue in the existing interception which would affect other consumers of the API, so makes sense to fix this regardless.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

It would be much better - both for performance and maintainability - to add these headers in Necko instead of relying on channel interception to do it.

Comment 4

[Sunil Mayya]

To implement in this Necko, I think we need the following steps:

• Implement IsDeviceRamAboveThreshold in Gecko AppShell
• Implement a wrapper in nsAndroidNetworkLinkService that wraps IsDeviceRamAboveThreshold in Gecko AppShell
• Implement a helper function IsGoogleDomain() to match google domain and path described here
• Add the header X-Search-Subdivision in asyncOpen if IsDeviceRamAboveThreshold is true.

Comment 5

[Sunil Mayya]

Attachment: Bug 1883496 - add X-Search-Subdivision assignment for android. r=#necko

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1883496 - Part 2: Remove UrlRequestInterceptor in Fenix

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f6444926c9c
add X-Search-Subdivision assignment for android. r=necko-reviewers,android-reviewers,geckoview-reviewers,valentin,gl,owlish,Roger
https://hg.mozilla.org/integration/autoland/rev/01f2eb842653
Part 2: Remove UrlRequestInterceptor in Fenix r=android-reviewers,Roger

Comment 8

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/2f6444926c9c
https://hg.mozilla.org/mozilla-central/rev/01f2eb842653

Comment 9

[Florin Bilt]

(In reply to Pulsebot from comment #7)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f6444926c9c
add X-Search-Subdivision assignment for android.
r=necko-reviewers,android-reviewers,geckoview-reviewers,valentin,gl,owlish,
Roger
https://hg.mozilla.org/integration/autoland/rev/01f2eb842653
Part 2: Remove UrlRequestInterceptor in Fenix r=android-reviewers,Roger

Perfherder has detected a browsertime performance change from push 01f2eb8426531aba2c35399292dc11f539b3ccff.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
24%	google-search-restaurants FirstVisualChange	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	246.88 -> 186.88	Before/After
23%	google-search-restaurants PerceptualSpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	249.62 -> 193.38	Before/After
18%	google-search-restaurants SpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	266.14 -> 219.04	Before/After
17%	google-search-restaurants SpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	265.54 -> 221.26	Before/After
13%	google-search-restaurants ContentfulSpeedIndex	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	258.11 -> 224.61	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 467

For more information on performance sheriffing please see our FAQ.

Comment 10

[Markus Stange [:mstange]]

This looks like the reverse of bug 1876370, so it looks like this patch has indeed fixed the regression from bug 1866133. Thank you!

(Unfortunately I can't find a continuous graph that shows both the regression and the improvement, it looks like we lost the old graphs in the monorepo migration.)