1708869 - [http3 enabled] Wrong Site Identity Displayed in Address Bar Due to Possible Race Condition

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[SL]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

1. Open about:preferences in a tab.
2. Very quickly open another new tab, search for something, and hit enter.
3. Observe the site identity button.

Actual results:

The site identity button "inherits" the previously-focused tab's site identity buttons. For example, following the STR above, I'll get an incorrect Firefox site identity in the newly opened tab.

A look at the browser toolbox's console shows that when switching to the tab whose site identity is inherited from the previously-focused tab, a cert is null error is thrown in line 661 in the chrome://browser/content/browser-siteIdentity.js file.

An interesting observation I noticed was that this bug was reproducible in the first 6 beta releases from the 88 cycle (88.0b1 - 88.0b6) but was not reproducible in the remaining beta releases and the two release candidates (88.0b7 - 88.0b9, 88.0rc1, 88.0rc2). HOWEVER, this bug got reintroduced in 89.0b1 and is still reproducible in the current beta (89.0b6), which hints at the possibility that this bug was caused by code within the EARLY_BETA_OR_EARLIER define.

Expected results:

The site identity should correctly correspond to the currently focused tab, not "inheriting" it from the previously-focused tab.

Comment 1

[SL]

Attachment: Wrong Site Identity Displayed in a Tab for Google Search

Comment 2

[SL]

Attachment: Errors in Browser Toolbox Console After Repeatedly Switching Tabs Back-and-Forth

Comment 3

[SL]

Attachment: The Code Throwing the `cert Is null` Error

Comment 4

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

The Bugbug bot thinks this bug should belong to the 'Firefox::Address Bar' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 5

[Johann Hofmann [:johannh]]

Can you use mozregression to find a regressing bug for this, since this seems to have been introduced recently? https://mozilla.github.io/mozregression/

Thanks!

Comment 6

[SL]

I ran mozregression from 2021-02-01 to 2021-03-31 using the mozilla-beta repository and got the following regression range: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=557bab6160978a71e90c226c4750db67affe470e&tochange=ad9bae9bb10cdda034f34e5d6379c0fcfa698032 (which, admittedly, doesn't look very helpful in finding the exact regressing bug).

Note that it seems like this bug is only reproducible in Beta builds, but not Nightly builds (trying to reproduce this bug on the latest Nightly doesn't work either), which further hints that the possibility that this might be a Beta build-only error.

Shown below is the full mozregression results:

app_name: firefox
build_date: 2021-03-14 20:42:56.240000
build_file: C:\Users\SL\.mozilla\mozregression\persist\557bab616097-shippable--mozilla-beta--target.zip
build_type: integration
build_url: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/UlvIGEpET1KOk2Ac7MJeMQ/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
changeset: 557bab6160978a71e90c226c4750db67affe470e
pushlog_url: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=557bab6160978a71e90c226c4750db67affe470e&tochange=ad9bae9bb10cdda034f34e5d6379c0fcfa698032
repo_name: mozilla-beta
repo_url: https://hg.mozilla.org/releases/mozilla-beta
task_id: UlvIGEpET1KOk2Ac7MJeMQ

Comment 7

[:Gijs (he/him)]

early_beta_or_earlier is pretty much exclusively used for things behind prefs, so I dug through changes in the default prefs in the window you identified.

No relevant-looking prefs got changed in firefox.js wrt early beta or earlier, but in all.js we changed network.http.http3.enabled. Can you still reproduce on beta if you turn that off (or, conversely, can you reproduce on later betas if you turn it on there) ? You'd probably want to restart the browser to make sure the pref applies to all connections

If that's not it, can you try browser.cache.offline.enable (false on nightly + early beta, true elsewhere) ? And/or enabling/disabling webrender (seems terribly unlikely that it's that, but strictly speaking some prefs changed in that window and I guess it's some sort of race, so who knows)

Comment 8

[SL]

This is very interesting... I have no idea how HTTP/3 is related to this bug, but it somehow is. I can't seem to reproduce this bug with network.http.http3.enabled set to false in Firefox 89.0b7. I re-enabled HTTP/3 again to confirm whether this was the root cause of this bug, and the bug does get reproduced. Typically, I could reproduce the bug within 2-3 tries, but with the preference set to false I couldn't reproduce the bug after ~10 tries.

However, this doesn't explain why this bug can't be reproduced on Nightly (which has network.http.http3.enabled set to true), and I couldn't reproduce the issue using mozregression either, so I'm not sure what's going on here...

Comment 9

[:Gijs (he/him)]

(In reply to SL from comment #8)

However, this doesn't explain why this bug can't be reproduced on Nightly (which has network.http.http3.enabled set to true), and I couldn't reproduce the issue using mozregression either, so I'm not sure what's going on here...

I suspect there are other network/tls-related prefs that are nightly-only... Just to make this even more fun. Thanks for all your help tracking this down!

Valentin or Dana, can you investigate? Apparently we hit some race conditions with http3 enabled in the browser identity block, but not with it disabled - but only on beta, not on nightly...

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

What's the value of network.ssl_tokens_cache_enabled in the version that works vs. the version that doesn't work? Does flipping it help?

Comment 11

[SL]

network.ssl_tokens_cache_enabled is set to false in Beta (which can reproduce this bug) and is set to true in Nightly (which can't reproduce this bug). Setting network.ssl_tokens_cache_enabled to true does seem to prevent the bug from happening, but trying to reproduce this bug on Nightly with said pref set to false still doesn't work. Shown below is the complete table of results:

Firefox 89.0b8 (latest Beta)

	network.ssl_tokens_cache_enabled: true	network.ssl_tokens_cache_enabled: false
network.http.http3.enabled: true	NOT REPRODUCIBLE	REPRODUCIBLE
network.http.http3.enabled: false	NOT REPRODUCIBLE	NOT REPRODUCIBLE

Firefox 90.0a1 (latest Nightly)

	network.ssl_tokens_cache_enabled: true	network.ssl_tokens_cache_enabled: false
network.http.http3.enabled: true	NOT REPRODUCIBLE	NOT REPRODUCIBLE
network.http.http3.enabled: false	NOT REPRODUCIBLE	NOT REPRODUCIBLE

Comment 12

[SL]

Attachment: wrong site identity in nightly.png

I somehow managed to reproduce the bug on Nightly 90.0a1 by switching network.ssl_tokens_cache_enabled to false. I guess that means the table for Nightly should be as such (same as Beta):

Firefox 90.0a1 (latest Nightly)

	network.ssl_tokens_cache_enabled: true	network.ssl_tokens_cache_enabled: false
network.http.http3.enabled: true	NOT REPRODUCIBLE	REPRODUCIBLE
network.http.http3.enabled: false	NOT REPRODUCIBLE	NOT REPRODUCIBLE

Comment 13

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

So it seems when the pref is false we end up with the wrong cert info?
More specifically we end up with a null certInfo like in comment 2 and that is causing the wrong padlock to show up.

Comment 14

[Dragana Damjanovic [:dragana]]

this pref is really, really confusing. It is not disabling the cache, but it is only partially disabling it.

I will make a patch to fix this.

Comment 15

[Dragana Damjanovic [:dragana]]

Attachment: Fix rebuilding certs with HTTP3 and 0RTT

Comment 16

[Paul Zühlcke [:pbz]]

Comment 19

[Paul Zühlcke [:pbz]]

To clarify, this affects all channels where 0RTT and HTTP/3 is being rolled out, including release.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e2ff9600bac
Fix rebuilding certs with HTTP3 and 0RTT r=necko-reviewers,valentin

Comment 21

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/3e2ff9600bac

Comment 22

[Pascal Chevrel:pascalc (PTO until April 26)]

Dragana, this is marked as P1/S2, is that a candidate for a beta uplift or can it ride the 90 train? Thanks

Comment 23

[Dragana Damjanovic [:dragana]]

(In reply to Pascal Chevrel:pascalc from comment #22)

Dragana, this is marked as P1/S2, is that a candidate for a beta uplift or can it ride the 90 train? Thanks

Yes it is a candidate for a beta uplift. I will request an uplift.

Comment 24

[Dragana Damjanovic [:dragana]]

Comment on attachment 9220427 [details]
Fix rebuilding certs with HTTP3 and 0RTT

Beta/Release Uplift Approval Request

• User impact if declined: The site identity information in the address bar may be wrong or may be missing. Also we may show wrong information whether a site is secure or not.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The correct behavior was wrongly control by a separate pref. That pref is set to true on Nightly and the correct code path has been exercise on Nightly since January.
• String changes made/needed:

Comment 26

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9220427 [details]
Fix rebuilding certs with HTTP3 and 0RTT

P1/S2 with a dupe, approved for 89 beta 11, thanks.

Comment 27

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5716b4cfc4a6

Comment 28

[Hani Yacoub, Desktop QA]

Verified as fixed on Windows 10 x64, macOS 10.15 and on Ubuntu 20.04.