1667032 - Crash in [@ OOM | large | NS_ABORT_OOM | nsTArray_base<T>::EnsureCapacity<T> | nsTArray_Impl<T>::SetLength<T> | nsPrinterWin::DefaultSettings]

Status: RESOLVED FIXED

(Core :: Printing: Setup, defect, P1)

Description

[Julien Cristau [:jcristau]]

Crash report: https://crash-stats.mozilla.org/report/index/018bc5a5-8939-4c79-bc26-fc2e20200924

Top 10 frames of crashing thread:

0 xul.dll NS_ABORT_OOM xpcom/base/nsDebugImpl.cpp:620
1 xul.dll nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_RelocateUsingMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator> xpcom/ds/nsTArray-inl.h:154
2 xul.dll nsTArray_Impl<unsigned char, nsTArrayInfallibleAllocator>::SetLength<nsTArrayInfallibleAllocator> xpcom/ds/nsTArray.h:2198
3 xul.dll nsPrinterWin::DefaultSettings const widget/windows/nsPrinterWin.cpp:77
4 xul.dll static std::_C__Invoker_functor::_Call<`lambda at /builds/worker/checkouts/gecko/widget/PrintBackgroundTask.h:54:17'> 
5 xul.dll mozilla::SpawnPrintBackgroundTask<nsPrinterBase, mozilla::PrintSettingsInitializer>::<unnamed-tag>::operator const widget/PrintBackgroundTask.h:53
6 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/widget/PrintBackgroundTask.h:48:11'>::Run xpcom/threads/nsThreadUtils.h:577
7 xul.dll nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:299
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234
9 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:332

These printing related OOM crashes look new in 82.

Comment 1

[Julien Cristau [:jcristau]]

Seems related to bug 1664530?

Comment 2

[Jonathan Kew [:jfkthame]]

Ugh... we're trying to allocate 4GB, which isn't going to end well.

The code in WithDefaultDevMode was already buggy, actually, but bug 1664530 added a new code path that reaches it, and looks like we hit the brokenness that way.

The trouble is that

  if (bytesNeeded < sizeof(DEVMODEW)) {
    return false;
  }

won't actually take the early return if bytesNeeded (a signed LONG value) is -1, indicating that DocumentPropertiesW failed! This is because the sizeof expression is unsigned, so -1 gets promoted to 4,294,967,295, which looks plenty big enough for a DEVMODEW.

(I thought we usually got compiler warnings for signed-vs-unsigned comparisons?)

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1667032 - Fix signed-vs-unsigned comparison in nsPrinterWin::CopyDefaultDevmodeW(). r=bobowen

Comment 4

[Jonathan Watt [:jwatt]]

Marking P1 since this seems to be spiking on Beta as Beta users are getting updated from b81 to b82.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ecd9c203a7c
Fix signed-vs-unsigned comparison in nsPrinterWin::CopyDefaultDevmodeW(). r=emilio

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 9177591 [details]
Bug 1667032 - Fix signed-vs-unsigned comparison in nsPrinterWin::CopyDefaultDevmodeW(). r=bobowen

Beta/Release Uplift Approval Request

• User impact if declined: Possible spurious-OOM crash for Windows users if a printer driver call fails.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: No known STR, rare crash seen in the wild.
• List of other uplifts needed: Bug 1664981
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minimal risk, just corrects the test for the Windows API failure.
  (Note that the patch here may be dependent on bug 1664981 being uplifted first, in order to apply cleanly.)
• String changes made/needed:

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1ecd9c203a7c

Comment 8

[Julien Cristau [:jcristau]]

Since we know this happens in the wild, do we need to keep the assert here about DocumentPropertiesW failing?

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9177591 [details]
Bug 1667032 - Fix signed-vs-unsigned comparison in nsPrinterWin::CopyDefaultDevmodeW(). r=bobowen

approved for 82.0b4

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Julien Cristau [:jcristau] from comment #8)

Since we know this happens in the wild, do we need to keep the assert here about DocumentPropertiesW failing?

Bob, WDYT? Is there any value in trying to track down a root cause here (which would probably require finding someone who encounters the DIAGNOSTIC_ASSERT on Nightly, and investigating their configuration), or should we just silence the error and move on?

Comment 11

[Bob Owen (:bobowen)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)

(In reply to Julien Cristau [:jcristau] from comment #8)

Since we know this happens in the wild, do we need to keep the assert here about DocumentPropertiesW failing?

Bob, WDYT? Is there any value in trying to track down a root cause here (which would probably require finding someone who encounters the DIAGNOSTIC_ASSERT on Nightly, and investigating their configuration), or should we just silence the error and move on?

I suspect there may be multiple reasons why this might fail, given it'll be dependent on the driver as well I imagine.
We just used to just fail for this sort of thing, so maybe we should convert this type of thing to telemetry, so at least we have some sight of the size of the problem.

Also, the change in bug 1664981, might help with preventing some failures here.

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c25b28d3bf3a