Closed Bug 1667032 Opened 2 years ago Closed 2 years ago

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

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox81 --- unaffected
firefox82 --- fixed
firefox83 --- fixed

People

(Reporter: jcristau, Assigned: jfkthame)

Details

(Keywords: crash, regression, Whiteboard: [print2020_v82][old-ui-])

Crash Data

Attachments

(1 file)

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.

Seems related to bug 1664530?

Flags: needinfo?(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?)

Flags: needinfo?(jfkthame)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED

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

Priority: -- → P1
Whiteboard: [print2020_v82][old-ui-]
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ecd9c203a7c
Fix signed-vs-unsigned comparison in nsPrinterWin::CopyDefaultDevmodeW(). r=emilio

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:
Attachment #9177591 - Flags: approval-mozilla-beta?
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

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

Flags: needinfo?(jfkthame)

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

approved for 82.0b4

Attachment #9177591 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(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?

Flags: needinfo?(jfkthame) → needinfo?(bobowencode)

(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.

Flags: needinfo?(bobowencode)
You need to log in before you can comment on or make changes to this bug.