Closed Bug 1466314 Opened 2 years ago Closed 2 years ago

Crash in mozilla::InputStreamLengthHelper::GetAsyncLength

Categories

(Core :: Networking, defect, P2, critical)

Unspecified
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed

People

(Reporter: calixte, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [necko-triaged])

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is
report bp-3e243d6e-2996-43b5-b438-7d4630180601.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::InputStreamLengthHelper::GetAsyncLength xpcom/io/InputStreamLengthHelper.cpp:161
1 xul.dll mozilla::net::HttpChannelParent::DoAsyncOpen netwerk/protocol/http/HttpChannelParent.cpp:575
2 xul.dll mozilla::net::HttpChannelParent::Init netwerk/protocol/http/HttpChannelParent.cpp:132
3 xul.dll mozilla::net::NeckoParent::RecvPHttpChannelConstructor netwerk/ipc/NeckoParent.cpp:323
4 xul.dll mozilla::net::PNeckoParent::OnMessageReceived ipc/ipdl/PNeckoParent.cpp:942
5 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:3501
6 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2136
7 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2066
8 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1912
9 xul.dll mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1945

=============================================================

There is 1 crash in nightly 62 with buildid 20180601100102. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1464090.

[1] https://hg.mozilla.org/mozilla-central/rev?node=833f1e224c88
Flags: needinfo?(amarchesini)
It seems that we need to copy the stream if this happens.
Flags: needinfo?(amarchesini) → needinfo?(honzab.moz)
(In reply to Andrea Marchesini [:baku] from comment #1)
> It seems that we need to copy the stream if this happens.

As a quick fix only, please, but with a HUGE TODO to remove ASAP.

Then please file a bug to let the IPC file stream implement your new length interface (the sync one, probably).  Looking into DeserializeIPCStream, apparently this is only for fd-backed streams, that always have a known size.  Also let you know we don't support chunked (generated) content upload and we ALWAYS send content-length on requests with body.  So it's definitely possible to carry the stream length with it and expose the size, when we now have a way to do so.
Flags: needinfo?(honzab.moz)
Andrea, can you take this bug?
Flags: needinfo?(amarchesini)
Priority: -- → P2
Whiteboard: [necko-triaged]
absolutely. I already have a patch. I'm going to ask for a review asap.
Flags: needinfo?(amarchesini)
> Then please file a bug to let the IPC file stream implement your new length
> interface (the sync one, probably).  Looking into DeserializeIPCStream,
> apparently this is only for fd-backed streams, that always have a known
> size.

For Blob/File, the original fileStream stays on the parent side and it knows its length. If it's not related to Blob/File, but still nsFileStreams, we move filedescriptors: on the parent side, we use ::Available on the I/O thread.
We have several tests. I don't think this is the reason of this crash.

What I suspect it's happening here, is that we use a stream taken from cache API, or from IDB. Some how, this stream is a pipe, and it's sent to the parent process for a HTTP request. The parent side cannot retrieve the full size of that stream using InputStreamLengthHelper, and we crash.

I will audit the streams generated by IDB and Cache, both on parent side and on child side.
(In reply to Andrea Marchesini [:baku] from comment #5)
> > Then please file a bug to let the IPC file stream implement your new length
> > interface (the sync one, probably).  Looking into DeserializeIPCStream,
> > apparently this is only for fd-backed streams, that always have a known
> > size.
> 
> For Blob/File, the original fileStream stays on the parent side and it knows
> its length. If it's not related to Blob/File, but still nsFileStreams, we
> move filedescriptors: on the parent side, we use ::Available on the I/O
> thread.
> We have several tests. I don't think this is the reason of this crash.
> 
> What I suspect it's happening here, is that we use a stream taken from cache
> API, or from IDB. Some how, this stream is a pipe, and it's sent to the
> parent process for a HTTP request. The parent side cannot retrieve the full
> size of that stream using InputStreamLengthHelper, and we crash.
> 
> I will audit the streams generated by IDB and Cache, both on parent side and
> on child side.

Thank you.  I'm not against memory copying, but, if reasonably simple, I would put some effort to finding and fixing (=impl length) streams that may pass by here.
this patch is nice to have but unrelated. patch 2 is built on top of this.
Assignee: nobody → amarchesini
Attachment #8983388 - Flags: review?(bugs)
I did some audit and it seems that the StreamBlobImpl can receive a pipe stream from child process. The pipe stream doesn't know its size and InputStreamLengthHelper will assert when used. With this patch, I make any StreamBlobImpl able to wrap the underlying stream if implementing nsIAsyncInputStream and not nsIInputStreamLength.

The reason why such streams must be wrapped is because InputStreamLengthHelper cannot retrieve the length from them without copying them all in memory.
Attachment #8983393 - Flags: review?(bugs)
(In reply to Andrea Marchesini [:baku] from comment #8)
> Created attachment 8983393 [details] [diff] [review]
> part 2 - StreamBlobImpl uses InputStreamLengthWrapper
> 
> I did some audit and it seems that the StreamBlobImpl can receive a pipe
> stream from child process. The pipe stream doesn't know its size and
> InputStreamLengthHelper will assert when used. With this patch, I make any
> StreamBlobImpl able to wrap the underlying stream if implementing
> nsIAsyncInputStream and not nsIInputStreamLength.
> 
> The reason why such streams must be wrapped is because
> InputStreamLengthHelper cannot retrieve the length from them without copying
> them all in memory.

This patch seems broken?
Flags: needinfo?(amarchesini)
Attachment #8983393 - Attachment is obsolete: true
Attachment #8983393 - Flags: review?(bugs)
Flags: needinfo?(amarchesini)
Attachment #8983691 - Flags: review?(bugs)
Attachment #8983388 - Flags: review?(bugs) → review+
Comment on attachment 8983691 [details] [diff] [review]
part 2 - StreamBlobImpl uses InputStreamLengthWrapper

I guess we can do this, but why the relevant input stream doesn't implement the right interfaces?
Attachment #8983691 - Flags: review?(bugs) → review+
> I guess we can do this, but why the relevant input stream doesn't implement
> the right interfaces?

StreamBlobImpl is used in 2 different contexts:
. IDB for Wasm modules. This is not expose to content: it's just used internaly to serialize data to disk.
. Child-to-parent IPCBlob. We care about this only.

I can fix child-to-parent, but what about next usage of this StreamBlobImpl class? Probably it's better to fix this issue only once here.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53eda8b305e9
StreamBlobImpl CTOR must receive an already_AddRefed inputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f522a95458d
StreamBlobImpl should wrap the cloned stream with InputStreamLengthWrapper if needed, r=smaug
https://hg.mozilla.org/mozilla-central/rev/53eda8b305e9
https://hg.mozilla.org/mozilla-central/rev/3f522a95458d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.