1759506 - startup Crash in [@ mozilla::net::LoadInfo::SetReservedClientInfo] on Amazon

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

~200 crashes with Firefox 91.7.0esr, ~400 with 91.7.0 (must be an unofficial build), no crashes with Firefox 91.6.*. Crashes on Windows and macOS, >40% in the first minute after launch. Comments mention it is frequently observed on amazon.com (>80% of users with en-US locale).

Eden, please investigate the issue.

Crash report: https://crash-stats.mozilla.org/report/index/38099100-6802-4daf-9e7f-766680220314

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(!isSome())

Top 10 frames of crashing thread:

0 xul.dll mozilla::net::LoadInfo::SetReservedClientInfo netwerk/base/LoadInfo.cpp:1771
1 xul.dll mozilla::dom::`anonymous namespace'::ClientChannelHelperParent::CreateClientForPrincipal dom/clients/manager/ClientChannelHelper.cpp:247
2 xul.dll mozilla::dom::`anonymous namespace'::ClientChannelHelperParent::CreateClient dom/clients/manager/ClientChannelHelper.cpp:213
3 xul.dll mozilla::dom::`anonymous namespace'::ClientChannelHelper::AsyncOnChannelRedirect dom/clients/manager/ClientChannelHelper.cpp:143
4 xul.dll mozilla::net::nsAsyncRedirectVerifyHelper::DelegateOnChannelRedirect netwerk/base/nsAsyncRedirectVerifyHelper.cpp:153
5 xul.dll mozilla::net::nsAsyncRedirectVerifyHelper::Run netwerk/base/nsAsyncRedirectVerifyHelper.cpp:259
6 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:805
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1152
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:85
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:324

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Also a spike in 98.0

Comment 2

[Eden Chuang[:edenchuang]]

Unfortunately, I can not reproduce it locally.

The assertion comes from https://searchfox.org/mozilla-central/rev/d4d7611ee4dd0003b492b865bc5988a4e6afc985/mfbt/Maybe.h#837 which complains that LoadInfo::mReservedClientInfo is not Nothing() while emplacing it.

Basically, we expect that when calling ClientChannelHelper::AsyncOnChannelRedirect, the passed new channel's reseervedClient/reservedClientInfo should be null/Nothing(). But it seems that the case violates our assumption. That means the newLoadInfo's SetReservedClientInfo/GiveReservedSource is called before calling ClientChannelHelper::AsyncOnChannelRedirect.

Comment 3

[Kershaw Chang [:kershaw]]

Basically, we expect that when calling ClientChannelHelper::AsyncOnChannelRedirect, the passed new channel's reseervedClient/reservedClientInfo should be null/Nothing(). But it seems that the case violates our assumption. That means the newLoadInfo's SetReservedClientInfo/GiveReservedSource is called before calling ClientChannelHelper::AsyncOnChannelRedirect.

Is this an uncovered error when a client info is set for a new load info?
Can we just replace the already existed client info?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

The fact that this crash clearly showed up also in 91.7.0esr feels like a pretty solid clue to go off:
https://hg.mozilla.org/releases/mozilla-esr91/pushloghtml?fromchange=FIREFOX_91_6_0esr_RELEASE&tochange=FIREFOX_91_7_0esr_RELEASE

Not seeing anything super-obvious in that range, but maybe bug 1744352?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Or maybe bug 1711870?

Comment 6

[Andrew McCreight [:mccr8]]

Well, the immediate cause is certainly bug 1754305. My guess would be that this showed up as something else before that.

Comment 7

[Andrew McCreight [:mccr8]]

(bug 1754305 is in the range that Ryan posted in comment 4.)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

If this was previously a diagnostic assert, that means we were effectively just ignoring this situation on Release in the past, right? Would it be a viable wallpaper solution to basically do the same more explicitly now (i.e. early return or something) to match the previous behavior? We could potentially ifdef it to builds without MOZ_DIAGNOSTIC_ASSERT_ENABLED set so it doesn't change anything in Nightly and early Beta builds.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

That at least seems like it'd be a low-risk upliftable option to address this spike in the short-term while we investigate how we're getting into this state in the first place.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Bug 1759506 - Avoid crashing if mReservedClientInfo already has something, r=#necko

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)

If this was previously a diagnostic assert, that means we were effectively just ignoring this situation on Release in the past, right? Would it be a viable wallpaper solution to basically do the same more explicitly now (i.e. early return or something) to match the previous behavior?

Before the assert, we would overwritten the value in the Maybe<> without calling the destructor. In this case, it looks like it wouldn't cause anything worse than a leak. So, probably not a crash, like I was guessing before.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6178b7ab160
Avoid crashing if mReservedClientInfo already has something, r=edenchuang

Comment 13

[Kershaw Chang [:kershaw]]

Comment on attachment 9268121 [details]
Bug 1759506 - Avoid crashing if mReservedClientInfo already has something, r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Could crash.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The logic of this patch is straightforward.
• String changes made/needed: N/A

Comment 14

[Cristian Tuns]

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

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9268121 [details]
Bug 1759506 - Avoid crashing if mReservedClientInfo already has something, r=#necko

Approved for 99.0b5. Thanks.

Comment 16

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6e50819e0356

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9268121 [details]
Bug 1759506 - Avoid crashing if mReservedClientInfo already has something, r=#necko

We're going to want this on ESR too.

Comment 18

[Peter Magyari (Desktop QA)]

Hi,

I'm attempting to verify the fix but unfortunately I'm unable able to reproduce the crash on my end.
Are there any more detailed steps through which I could try to reproduce it?

Thank you!

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

I hadn't been able to reproduce the issue and based on the fix there is nothing to verify here.

Comment 20

[Brindusa Tot, DTE]

Based on previous comments, I'm removing the qe-verify+ flag

Comment 21

[Ryan VanderMeulen [:RyanVM]]

New report from Beta showing the diagnostic assert being hit now as expected:
https://crash-stats.mozilla.org/report/index/c25f55a3-89bd-41cc-94e2-dbd3f0220320

And as expected, no reports from 99.0b6 so far since diagnostic asserts are tied to early beta or earlier.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9268121 [details]
Bug 1759506 - Avoid crashing if mReservedClientInfo already has something, r=#necko

Wallpapers us back to where we were before the regressing bug landed. Approved for 91.8esr.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/f34283a3bb66

Comment 24

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9268121 [details]
Bug 1759506 - Avoid crashing if mReservedClientInfo already has something, r=#necko

P1/S2 crash. Fix works on Beta, approved for 98.0.2, thanks.

Comment 25

[Pascal Chevrel (relman team) -> :pascalc]

https://hg.mozilla.org/releases/mozilla-release/rev/43b6c4a1f577

Comment 26

[Eden Chuang[:edenchuang]]

(In reply to Kershaw Chang [:kershaw] from comment #3)

Basically, we expect that when calling ClientChannelHelper::AsyncOnChannelRedirect, the passed new channel's reseervedClient/reservedClientInfo should be null/Nothing(). But it seems that the case violates our assumption. That means the newLoadInfo's SetReservedClientInfo/GiveReservedSource is called before calling ClientChannelHelper::AsyncOnChannelRedirect.

Is this an uncovered error when a client info is set for a new load info?

I think it depends on why the ReservedClientInfo had already been set.

Can we just replace the already existed client info?

According to the context, I think we can replace the existing client Info since it is a cross-origin redirect for document loading.
According to the cross-origin redirection for ClientInfo, the previous ClientInfo should not be valid anymore, so it should be okay to replace the existing one.
But we probably should also clean up the LoadInfo::mReservedClient.

Comment 27

[Kershaw Chang [:kershaw]]

Let's close this bug and investigate further in bug 1761208.