Closed Bug 1427012 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect, critical)

58 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: philipp, Assigned: Alex_Gaynor)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.
Jim, can you take a look?
Flags: needinfo?(jmathies)
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.
(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.
Flags: needinfo?(agaynor)
These all seem like good improvements, I'll implement them.
Assignee: nobody → agaynor
Flags: needinfo?(jmathies)
Flags: needinfo?(agaynor)
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?
Attachment #8940746 - Flags: review?(bobowencode)
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).
Attachment #8940746 - Flags: review?(bobowencode) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b813845b721
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Low volume crash, not sure this is worth uplifting this late.
What sayeth you to that, Alex? :)
Flags: needinfo?(agaynor)
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.
Flags: needinfo?(agaynor)
(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...
You need to log in before you can comment on or make changes to this bug.