1667768 - Don't issue a DIAGNOSTIC_ASSERT (i.e. crash, for dev-channel users) if OpenPrinter fails

Status: RESOLVED FIXED

(Core :: Printing: Setup, defect)

Description

[Jonathan Kew [:jfkthame]]

We probably should weaken the DIAGNOSTIC_ASSERT that we issue in a couple of places if OpenPrinterW() fails, so that we don't inflict crashes on developer-mode users who run into this.

An example of how to reliably trigger this at present is to remove a printer from the Windows list of devices while the Print UI is open, and then try to use that printer (which will already be present in the Destination list, possibly even already selected there).

If we just bail out instead of issuing the assertion, the print operation will fail with a "Printer not found" error, which is much friendlier and perfectly adequate to the situation.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1667768 - Don't DIAGNOSTIC_ASSERT if unable to open a Windows printer, just warn (debug only) and bail out. r=bobowen

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85a37b51bf2e
Don't DIAGNOSTIC_ASSERT if unable to open a Windows printer, just warn (debug only) and bail out. r=bobowen

Comment 3

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/85a37b51bf2e

Comment 4

[Jonathan Watt [:jwatt]]

Presumably we should uplift this to Beta?

Comment 5

[Jonathan Kew [:jfkthame]]

MOZ_DIAGNOSTIC_ASSERT doesn't fire on beta/release channels, does it?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

It fires on early beta.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 9178199 [details]
Bug 1667768 - Don't DIAGNOSTIC_ASSERT if unable to open a Windows printer, just warn (debug only) and bail out. r=bobowen

Beta/Release Uplift Approval Request

• User impact if declined: Possible DIAGNOSTIC_ASSERT crash for Windows users when a printer driver 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 to hit this, but a few occurrences have been seen in the wild.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just removes DIAGNOSTIC_ASSERT that isn't giving any actionable information.
• String changes made/needed:

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

It fires on early beta.

For MOZ_DEV_EDITION more specifically. Other than that MOZ_DEV_EDITION is set by browser/branding/aurora/configure.sh, I couldn't easily tell when MOZ_DEV_EDITION is set though.

Comment 9

[Julien Cristau [:jcristau]]

Diagnostic asserts fire on nightly and developer edition builds (which are beta-based, but with a different branding and some build differences, including that MOZ_DEV_EDITION define).

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9178199 [details]
Bug 1667768 - Don't DIAGNOSTIC_ASSERT if unable to open a Windows printer, just warn (debug only) and bail out. r=bobowen

stop crashes on devedition, approved for 82.0b6

Comment 11

[Julien Cristau [:jcristau]]

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