1407186 - Crash in imgRequest::OnStartRequest

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-45f5ec68-a228-40f1-9ed9-cd49e0171010.
=============================================================

There are 5 crashes in nightly 58 with buildid 20171009220104. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1391693.

[1] https://hg.mozilla.org/mozilla-central/rev?node=4ff6aa3fae0bc95db7fd69dc7537781c57f55e20

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Do you have access to the URLs where these crashes are occurring?  Can you email them to me?  I'd like to try to reproduce.

Comment 2

[Calixte Denizet (:calixte)]

Done.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I'm having trouble reproducing this locally.  Lets see if the fix in bug 1407225 helps here.  They both seem to relate to redirects.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

If this persists after bug 1407225, then I'd like to see if my further changes in bug 1204254 help.  I believe the crash here is due to a duplicate OnStartRequest being fired and my Cancel/Abort fixes in bug 1204254 may eliminate that.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Looking at this a bit more, we could also try checking for a failed mStatus here:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#3006

It seems likely these are channels that are getting canceled in the middle of a the new internal redirect to the InterceptedHttpChannel class.  This may just be an existing race that is not normally triggered in nsHttpChannel on other redirects.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Oh, I actually think I see the problem now.

I think we are double-invoking the listener here:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedHttpChannel.cpp#465

And here:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#3033

When the nsHttpChannel redirects the InterceptedHttpChannel::AsyncOpen() is probably returning an error.  And we are invoking the listener callback from both classes.

I should change InterceptedHttpChannel::AsyncOpen() to either not return an error in this case or to not invoke the listener itself.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Yea, this is what nsHttpChannel::AsyncOpen() does:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#6141

It returns NS_OK and invokes the listener with the error itself via AsyncAbort().  InterceptedHttpChannel should do the same.

I'll write a patch for this in the morning.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that we reset the interception if ChannelIntercepted() returns an error. r=asuth

I think this test shows the problem.  Our previous interception code did a ResetInterception() in this case, but in InterceptedHttpChannel we are returning an error.  We're also mistakenly double-firing listener callbacks.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix InterceptedHttpChannel::AsyncOpen() error handling. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc5b40bc69fa989f7fc3adbf079429d37067a45a

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8917377 [details] [diff] [review]
P2 Test that we reset the interception if ChannelIntercepted() returns an error. r=asuth

So, we had a couple problems with InterceptedHttpChannel::AsyncOpen() error handling.  This patch fixes them by:

1. Don't both return an error value and invoke listener callbacks.  This results in both the caller and the InterceptedHttpChannel invoking the listener callbacks which can cause nullptr derefs like this crash.
2. If the initial interception of a channel fails, we need to ResetInterception() to match previous behavior.  We try to be nice and fallback to network for internal problems.
3. If this is a pre-synthesized channel we fire Cancel() and fire listener callbacks with an error code.  We can't fallback to network after the service worker has already seen the FetchEvent.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8917404 [details] [diff] [review]
P1 Fix InterceptedHttpChannel::AsyncOpen() error handling. r=asuth

This adds a test to verify we ResetInterception() if something unexpected happens during nsIInterceptedChannel::ChannelIntercepted().  Without the P1 patch we fail this test in non-e10s mode.  With the P1 patch we pass this test in non-e10s mode.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8917404 [details] [diff] [review]
P1 Fix InterceptedHttpChannel::AsyncOpen() error handling. r=asuth

Review of attachment 8917404 [details] [diff] [review]:
-----------------------------------------------------------------

The MakeScopeExit closed over the rv makes for refreshingly concise code!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3b5e007cd1
P1 Fix InterceptedHttpChannel::AsyncOpen() error handling. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a282a37fcb1
P2 Test that we reset the interception if ChannelIntercepted() returns an error. r=asuth

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f3b5e007cd1
https://hg.mozilla.org/mozilla-central/rev/1a282a37fcb1

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

So far zero crash reports in the first buildid with the fix.  So looking pretty good.

By the way, the quick crash bug was super helpful in getting this fixed.  Thanks!