Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add logging for print_backend failures #29884

Merged
merged 1 commit into from Oct 5, 2021
Merged

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jun 24, 2021

Description of Change

Refs:

Chromium recently added the ability to distinguish between no printer(s) and a query failure. I don't think a failure to enumerate printers should result in a user-facing error, but we should at least log this nuance instead of dropping it on the ground.

Checklist

Release Notes

Notes: none

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 24, 2021
@codebytere codebytere added target/14-x-y semver/patch backwards-compatible bug fixes labels Jun 24, 2021
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 25, 2021
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We propagate user facing generic error on failure cases of obtaining a valid printer

if (printer_name.empty() || !IsDeviceNameValid(printer_name)) {
if (print_callback)
std::move(print_callback).Run(false, "no valid printers available");
return;
}

If we want to distinguish the failures further, shouldn't they be added to final user facing error message ?

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general

shell/browser/api/electron_api_web_contents.cc Outdated Show resolved Hide resolved
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like linux arm DCHECK crashes are related.

@codebytere
Copy link
Member Author

codebytere commented Sep 8, 2021

@deepak1556 yeah i'm seeing:

[88155:0908/214127.957025:ERROR:print_backend_cups.cc(218)] CUPS: Error getting default printer: Bad file descriptor

is this something you think we should try to fix ourselves or just not run this specific test there given it was already not succeeding?

Ah wait i missed this was a crash and not just a failure - hmmmm i'll look into it though it's slightly baffling given that this isn't really a behavioral change in terms of Chromium functions being called.

@deepak1556
Copy link
Member

Looks like the DCHECK fails once the print backend mojom has a failure (not sure why the scoped_refptr referencing the backend would fail at this point), might want to try emulating the mojom failure locally and see if it repros.

@codebytere codebytere force-pushed the printer-error-logs branch 4 times, most recently from 00634f8 to 4f66136 Compare October 4, 2021 21:16
@codebytere codebytere merged commit d2508a6 into main Oct 5, 2021
@codebytere codebytere deleted the printer-error-logs branch October 5, 2021 07:16
@release-clerk
Copy link

release-clerk bot commented Oct 5, 2021

No Release Notes

@trop
Copy link
Contributor

trop bot commented Oct 5, 2021

I have automatically backported this PR to "15-x-y", please check out #31286

@trop trop bot added the in-flight/15-x-y label Oct 5, 2021
@trop
Copy link
Contributor

trop bot commented Oct 5, 2021

I have automatically backported this PR to "14-x-y", please check out #31287

@trop trop bot removed the target/15-x-y label Oct 5, 2021
@trop
Copy link
Contributor

trop bot commented Oct 5, 2021

I have automatically backported this PR to "16-x-y", please check out #31288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants