1606901 - Crash in [@ mozilla::net::PDocumentChannelParent::SendRedirectToRealChannel]

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-fc264654-c85a-44d6-b13e-789fb0200103.

Seen while looking at nightly crashes: https://bit.ly/2SRxCU2. Fairly low volume Windows crash which started in 20191205215330

All crashes have MOZ_RELEASE_ASSERT((aVar).get_PParentToChildStreamParent()) (NULL actor value passed to non-nullable param)

Possible regression range based on build id: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6989fcd6bab30c909411fbec04a4f29a78024ddd&tochange=ed92ceb9cb68d134de79f7e71aa70882e5a91f31

There are two IPC related landings in that changeset.

Top 10 frames of crashing thread:

0 xul.dll static mozilla::ipc::IPDLParamTraits<mozilla::ipc::IPCRemoteStreamType>::Write ipc/ipdl/InputStreamParams.cpp:898
1 xul.dll static mozilla::ipc::IPDLParamTraits<mozilla::ipc::IPCRemoteStreamParams>::Write ipc/ipdl/InputStreamParams.cpp:1096
2 xul.dll static mozilla::ipc::IPDLParamTraits<mozilla::ipc::InputStreamParams>::Write ipc/ipdl/InputStreamParams.cpp:2030
3 xul.dll static mozilla::ipc::IPDLParamTraits<mozilla::ipc::IPCStream>::Write ipc/ipdl/IPCStream.cpp:45
4 xul.dll static mozilla::ipc::IPDLParamTraits<mozilla::Maybe<mozilla::ipc::IPCStream> >::Write ipc/glue/IPDLParamTraits.h:250
5 xul.dll static mozilla::ipc::IPDLParamTraits<nsIInputStream*>::Write ipc/glue/IPCStreamUtils.cpp:537
6 xul.dll static mozilla::ipc::IPDLParamTraits<mozilla::dom::ReplacementChannelConfigInit>::Write ipc/ipdl/DOMTypes.cpp:2856
7 xul.dll static mozilla::ipc::IPDLParamTraits<mozilla::Maybe<mozilla::dom::ReplacementChannelConfigInit> >::Write ipc/glue/IPDLParamTraits.h:250
8 xul.dll static mozilla::ipc::IPDLParamTraits<mozilla::net::RedirectToRealChannelArgs>::Write ipc/ipdl/NeckoChannelParams.cpp:4942
9 xul.dll mozilla::net::PDocumentChannelParent::SendRedirectToRealChannel ipc/ipdl/PDocumentChannelParent.cpp:136

Comment 1

[Marcia Knous [:marcia]]

Adding a ni on jed to perhaps identify what might have regressed or if this assert is anything to be concerned about.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Looks like there are two problems here:

1. A missing null check, probably here, in SerializeInputStreamAsPipeInternal. There are several error cases in the function it calls that return nullptr.
2. Not checking for forbidden nulls when constructing the IPDL-generated type, only when serializing it.

Comment 3

[Jim Mathies [:jimm]]

do we know what we want to do with this? volume looks questionably high on beta.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Looks like part 1 of comment 2 is a trivial fix, so let's take this now.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This is showing up as one of the top crashes on Beta73 so far.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Jed are you taking this one?

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I don't know this code. :baku?

(I'm not sure where this bug even belongs. Maybe Necko, because DocumentChannel seems to be involved?)

Comment 8

[Andrew Overholt [:overholt]]

Jean-Yves or Matt might have thoughts here. Core: Networking is the component we've been using for DocumentChannel (bug 1556493).

Comment 9

[Honza Bambas (:mayhemer)]

This needs an immediate attention.

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1606901 - Test that channel is still opened before continuing. r?mattwoodrow

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7aee28f987c3
Test that channel is still opened before continuing. r=mattwoodrow

Comment 12

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7aee28f987c3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 9121447 [details]
Bug 1606901 - Test that channel is still opened before continuing. r?mattwoodrow

Beta/Release Uplift Approval Request

• User impact if declined: parent will crash if for whatever reason a child process got killed
• 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: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We just check if the child is still alive before calling a method.

Been in Nightly for a few days, crashes have stopped

• String changes made/needed: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121447 [details]
Bug 1606901 - Test that channel is still opened before continuing. r?mattwoodrow

Fixes an Fx73 topcrash. Approved for 73.0b8.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/484e2cc7166ccafa0f8c4afe0d42a484a1eff217

Comment 17

[[:philipp]]

these crashes seem to be continuing in 73.0b8 unfortunately - do you rather want to reopen this bug or should i get a new one on file?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Let's reopen.

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Poked at this a bit. Like what :jld said, this appears to be being caused, in part, by a lack of error checking when serializing an nsIInputStream. Specifically, it appears that the call to IPCStreamSource::Create is not checked for errors (https://searchfox.org/mozilla-central/rev/cfd1cc461f1efe0d66c2fdc17c024a203d5a2fd8/ipc/glue/InputStreamUtils.cpp#118). This lack of checking causes us to end up with an invalid IPCStream object to serialize, due to nullptr being present when it isn't allowed, which causes the crash in this bug.

This method will return nullptr if the stream which is being sent is not marked as non-blocking (or some other less-likely scenarios), so I'm guessing that our uploadStream is marked as blocking when we're trying to serialize it.

We could add checking here, and fail the input stream serialization, but I believe that would lead to a different crash, as the code is using nsIInputStream directly as a type, and the serializer for that type assumes serialization is infallible: https://searchfox.org/mozilla-central/rev/cfd1cc461f1efe0d66c2fdc17c024a203d5a2fd8/ipc/glue/IPCStreamUtils.cpp#535

I'm not entirely certain I understand why uploadStream is being sent down on the new child channel at all, but we should probably not try to change that in an uplifted patch. The easiest solution right now would probably be to switch to using the fallible IPCStream serialization methods, add the null check in the stream serialization codepath, and not attach any upload stream if it fails.

Comment 20

[Chris Peterson [:cpeterson]]

Due to the ongoing crashes in 73.0b8, Relman has asked us to disable DocumentChannel in 73.0b9. That is bug 1610888.

Comment 21

[Matt Woodrow (:mattwoodrow)]

(In reply to :Nika Layzell (ni? for response) from comment #19)

I'm not entirely certain I understand why uploadStream is being sent down on the new child channel at all, but we should probably not try to change that in an uplifted patch. The easiest solution right now would probably be to switch to using the fallible IPCStream serialization methods, add the null check in the stream serialization codepath, and not attach any upload stream if it fails.

We need the upload stream on the final channel in the content process, because we use it when adding to session history.

In the process switch case, this means that we need to serialize it from the parent. The same-process case could instead get a copy from the DocumentChannelChild, but we haven't done this as we wanted to have the two code paths be as similar as possible (but could revisit that decision).

We also might want to change the way we add to session history for loads (and do it from the parent), which would fix this for all cases.

Comment 22

[Nika Layzell [:nika] (ni? for response)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)

We also might want to change the way we add to session history for loads (and do it from the parent), which would fix this for all cases.

Yeah, doing that change would likely be part of the async session history changes we have planned.

I'm surprised we somehow have a blocking upload stream in the parent process, as theoretically we should've received it from the content process, meaning (I'm hoping), that it's guaranteed to be async. Is it possible for a channel to gain an upload stream due to a redirect or something like that?

Comment 23

[Matt Woodrow (:mattwoodrow)]

(In reply to :Nika Layzell (ni? for response) from comment #22)

I'm surprised we somehow have a blocking upload stream in the parent process, as theoretically we should've received it from the content process, meaning (I'm hoping), that it's guaranteed to be async. Is it possible for a channel to gain an upload stream due to a redirect or something like that?

I haven't been able to find any ways to get a blocking input stream! There's code like HttpBaseChannel::EnsureUploadStreamIsCloneable which can mutate it, but still seems like that always gets a non-blocking upload stream.

Comment 24

[Julien Cristau [:jcristau]]

No crashes in 73.0b9 and up, marking as disabled for that release.

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1606901 - P2. Abort should we failed at creating IPCStreamSource. r?mattwoodrow

Still no understanding on why this could actually fail.
IPCStreamSource::Create could only fail under two circumstances prior the first fix in bug 1606901.
Either the ipc channel was closed and aManager->SendPParentToChildStreamConstructor(source) would have returned nullptr.
Or IPCStreamSourceParent::Create(aInputStream) returned null.

The original fix prevented the first condition from occurring. The second can only occur if IPCStreamSourceParent::Initialize failed, which happens should the stream be blocking.

It is still unknow on how the uploadStream sets here (used for POST) could be blocking.

For now, we will just prevent the issue from occurring.

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1606901 - P1. Don't assume that nsIAsyncInputStream are always non-blocking. r?baku

From the documentation:
" While this interface is almost exclusively used with non-blocking streams, it is not necessary that nsIInputStream::isNonBlocking return true"

So we can't assume that a nsIAsyncInputStream is always non-blocking. Handle the case where it is.

Depends on D61367

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57f2c7a63fcf
P1. Don't assume that nsIAsyncInputStream are always non-blocking. r=baku

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121447 [details]
Bug 1606901 - Test that channel is still opened before continuing. r?mattwoodrow

Removing the Beta approval on this to get it off the needs-uplift radar.

Comment 29

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/57f2c7a63fcf

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09db01447338
P2. Abort should we failed at creating IPCStreamSource. r=mattwoodrow

Comment 31

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/09db01447338

Comment 32

[Firefox Bug Husbandry Bot]

Please specify a root cause for this bug. See :tmaity for more information.

Comment 33

[Nhi Nguyen (:nhi)]

NI for RCA