1427012 - Crash in PR_Read | mozilla::layout::PRFileDescStream::read

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-8b7b1d86-fedd-401d-b6ee-1337a0171224.
=============================================================

Top 10 frames of crashing thread:

0 nss3.dll PR_Read nsprpub/pr/src/io/priometh.c:109
1 xul.dll mozilla::layout::PRFileDescStream::read layout/printing/DrawEventRecorder.h:92
2 xul.dll mozilla::layout::PrintTranslator::TranslateRecording layout/printing/PrintTranslator.cpp:31
3 xul.dll mozilla::layout::RemotePrintJobParent::PrintPage layout/printing/ipc/RemotePrintJobParent.cpp:130
4 xul.dll mozilla::layout::RemotePrintJobParent::RecvProcessPage layout/printing/ipc/RemotePrintJobParent.cpp:88
5 xul.dll mozilla::layout::PRemotePrintJobParent::OnMessageReceived ipc/ipdl/PRemotePrintJobParent.cpp:245
6 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:3290
7 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2119
8 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2049
9 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1895

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

this printing related crash signature is newly showing up (with a low volume) in firefox 58 on windows & macos.

Comment 1

[Mike Taylor [:miketaylr]]

Jim, can you take a look?

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

If I'm reading the crash report correctly, the only way this should happen is if the content process sends an incorrect filepath to the parent in the IPC message -- this whole mechanism was changed in bug 87f5eb24ef4b (landed for 59) to no longer pass paths.

Comment 3

[Bob Owen (:bobowen)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #2)
> If I'm reading the crash report correctly, the only way this should happen
> is if the content process sends an incorrect filepath to the parent in the
> IPC message -- this whole mechanism was changed in bug 87f5eb24ef4b (landed
> for 59) to no longer pass paths.

That or the file fails to open for some reason.
I think we should set mGood to !!mFd in PRFileDescStream::Open.

I also think we should:
* Check the return value of PR_Write in PRFileDescStream::Flush and write and set mGood accordingly.
* Check the return value of PR_Seek in PRFileDescStream::Seek and again set mGood accordingly.
* Add a check for mGood at the start of PRFileDescStream::write and read (probably Flush and possibly Seek as well).
* Add a destructor that calls Close() or possibly PR_Close directly if we don't want the flush.
* Remove the MOZ_ASSERT(aSize <= kBufferSize) in PRFileDescStream::write, because it can never happen.

I believe most if not all of these could be uplifted.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

These all seem like good improvements, I'll implement them.

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1427012 - make the stream class used in printing more resilient to runtime errors, instead of crashing;

Review commit: https://reviewboard.mozilla.org/r/211026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211026/

Comment 6

[Bob Owen (:bobowen)]

Comment on attachment 8940746 [details]
Bug 1427012 - make the stream class used in printing more resilient to runtime errors, instead of crashing;

https://reviewboard.mozilla.org/r/211026/#review217190

Also the destructor, otherwise I suspect there might be some printing error code paths that don't close the file.

::: commit-message-ca379:1
(Diff revision 1)
> +Bug 1427012 - make the stream class used in printing more resillient to runtime errors, instead of crashing; r?bobowen

nit: resilient

::: layout/printing/DrawEventRecorder.h:58
(Diff revision 1)
>    void Flush() {
>      // See comment in Close().
>      if (IsOpen() && mBufferPos > 0) {
> +      PRInt32 length =
> -      PR_Write(mFd, static_cast<const void*>(mBuffer.get()), mBufferPos);
> +        PR_Write(mFd, static_cast<const void*>(mBuffer.get()), mBufferPos);
> +      mGood = length != -1;

Should this be length != mBufferPos?

::: layout/printing/DrawEventRecorder.h:81
(Diff revision 1)
>      if (IsOpen()) {
>        // If we're writing more data than could ever fit in our buffer, flush the
>        // buffer and write directly.
>        if (aSize > kBufferSize) {
>          Flush();
>          PR_Write(mFd, static_cast<const void*>(aData), aSize);

Shouldn't we check this as well?

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8940746 [details]
Bug 1427012 - make the stream class used in printing more resilient to runtime errors, instead of crashing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211026/diff/1-2/

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8940746 [details]
Bug 1427012 - make the stream class used in printing more resilient to runtime errors, instead of crashing;

https://reviewboard.mozilla.org/r/211026/#review217212

::: layout/printing/DrawEventRecorder.h:62
(Diff revision 2)
>    void Flush() {
>      // See comment in Close().
>      if (IsOpen() && mBufferPos > 0) {
> +      PRInt32 length =
> -      PR_Write(mFd, static_cast<const void*>(mBuffer.get()), mBufferPos);
> +        PR_Write(mFd, static_cast<const void*>(mBuffer.get()), mBufferPos);
> +      mGood = length >= 0 && (size_t)length == mBufferPos;

c++ style casts please (and below).

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8940746 [details]
Bug 1427012 - make the stream class used in printing more resilient to runtime errors, instead of crashing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211026/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b813845b721
make the stream class used in printing more resilient to runtime errors, instead of crashing; r=bobowen

Comment 11

[Raul Gurzau (:RaulG)]

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

Comment 12

[Julien Cristau [:jcristau]]

Low volume crash, not sure this is worth uplifting this late.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

What sayeth you to that, Alex? :)

Comment 14

[Alex Gaynor [:Alex_Gaynor]]

I'd be hard pressed to argue it's not very low volume :-) We don't have clear reproduction instructions, so I'm ok with waiting for 59 for this.

Comment 15

[jscher2000]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #14)
> I'd be hard pressed to argue it's not very low volume :-) We don't have
> clear reproduction instructions, so I'm ok with waiting for 59 for this.

I get bitten by this bug (or at least this signature) when printing after waking Windows 7 x64 (Fx58 x64) from hibernation.

https://crash-stats.mozilla.com/report/index/c12022b3-bcc5-4246-be9b-a00890180212
https://crash-stats.mozilla.com/report/index/fb910834-0f18-4117-b573-114960180207
https://crash-stats.mozilla.com/report/index/4e1b516a-a207-4cc0-8310-5795e0180201
https://crash-stats.mozilla.com/report/index/adc37a0d-5c25-417f-a82d-bad751180131
https://crash-stats.mozilla.com/report/index/b6645d77-df9b-4fc8-a084-1cee70180124

It's hard to test because I only notice it when I hibernate the system on a home wi-fi network and wake the system on a work wired network (I seldom need to print at home). I will try to find a more convenient way to re-create the issue and leave Nightly open more often to make sure it's fixed.

In the same situation (after waking from hibernation at the office), Firefox 57, instead of crashing, would display the meaningless/generic "An error occurred while printing" alert. The resolution was to restart Firefox 57. I suspect the underlying issue is the same, and might be a different bug -- more graceful failure is only a Band-aid...