1355887 - Crash in mozilla::net::nsAsyncRedirectVerifyHelper::OnRedirectVerifyCallback

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-1bb1ecf5-022c-48e0-ab45-383d92170412.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170411030208: http://bit.ly/2oz8XEn

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=731639fccc709a4dd95fed7e9dda88efb2227906&tochange=f914d40a48009c5acd1093e9939cc0ec035696dd

Comment 1

[Dragana Damjanovic [:dragana]]

I looked at regression range, but I have not seen a obvious candidate for the crash.

Can you take a look you were looking into HttpChannelParent-HttpChannelChild recently?

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Not sure if this crash has any relationship with my recent HttpChannel modification based on the regression range and crash stack. There are similar reports from Firefox Aurora channel (54.0a2) as well.

Put my first hand investigation here for notes:

Two different crash stacks on socorro:

1. https://crash-stats.mozilla.com/report/index/be26b44c-e2a8-4fad-b39b-76ac82170408
>mozilla::net::nsAsyncRedirectVerifyHelper::OnRedirectVerifyCallback
>mozilla::net::nsAsyncRedirectVerifyHelper::DelegateOnChannelRedirect
>mozilla::net::nsIOService::AsyncOnChannelRedirect
>mozilla::net::nsAsyncRedirectVerifyHelper::Run

The OnRedirectVerifyCallback is always from the failure handling after sink->AsyncOnChannelRedirect().
https://hg.mozilla.org/mozilla-central/annotate/f914d40a4800/netwerk/base/nsAsyncRedirectVerifyHelper.cpp#l171

The API contract we have here is that the implementer of nsIChannelEventSink.asyncOnChannelRedirect() should never call OnRedirectVerifyCallback() if returning fail cause. Didn't find any suspicious code after a quick check on latest m-c.

2. https://crash-stats.mozilla.com/report/index/c61fbcc3-adbf-46d9-96ec-0ec782170412
>mozilla::net::nsAsyncRedirectVerifyHelper::OnRedirectVerifyCallback
> // some JS-to-native call stack
>mozilla::dom::SubtleCrypto::Verify

This might indicate there is another call path directly calls OnRedirectVerifyCallback without increasing mExpectedCallbacks first, since we only do ++mExpectedCallbacks in nsAsyncRedirectVerifyHelper::DelegateOnChannelRedirect. However there is no JS call stack in our crash report so I could not get further information.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

My plate is quite full so I'll not have time to go deeper on this bug recently. Will need someone to take care of this if we want to do something quick.

Comment 4

[Jason Duell]

Valentin, give this a go.  I guess Honza would be next in line if you can't figure it out.

5-6 crashes per day, so not a drop-everything-else bug.

Comment 5

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #2)
> This might indicate there is another call path directly calls
> OnRedirectVerifyCallback without increasing mExpectedCallbacks first, since
> we only do ++mExpectedCallbacks in
> nsAsyncRedirectVerifyHelper::DelegateOnChannelRedirect. However there is no
> JS call stack in our crash report so I could not get further information.

I've looked at this and I think sc's hunch is correct.
There is a fair number of addons using this API, and 3 of the ones I looked at called onRedirectVerifyCallback even if asyncOnChannelRedirect threw an error. [1]

In this case, when mExpectedCallbacks is decremented by and addon implemented callback, it could eventually get to have a negative value.
Since this is not exactly a critical issue, and XPCOM addons are going away in 57, I'm not sure how much time we ought to spend on fixing this. We have code to deal with this [2], so maybe we could change the DIAGNOSTIC_ASSERT to a MOZ_ASSERT and wait for the complication to go away.

[1] https://dxr.mozilla.org/addons/search?q=onRedirectVerifyCallback&redirect=true
[2] https://hg.mozilla.org/releases/mozilla-aurora/annotate/f09f78db26a5/netwerk/base/nsAsyncRedirectVerifyHelper.cpp#l113

Comment 6

[Jason Duell]

Low crash rate, and the rare bug that will fix itself in a few months.