Closed Bug 1524888 Opened 10 months ago Closed 9 months ago

Certain characters in pdfs do not display correctly

Categories

(Firefox :: PDF Viewer, defect)

65 Branch
Unspecified
All
defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 - verified
firefox67 + verified

People

(Reporter: c.tozlowski, Assigned: bdahl)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

  1. Go to this pdf file: https://1password.com/files/1Password%20for%20Teams%20White%20Paper.pdf

  2. The first occurence of this issue appears on page 10.

Actual results:

The character is displayed incorrectly. See the attached screenshot.

Expected results:

The characters "18000" should be displayed.

Component: Untriaged → PDF Viewer

[Tracking Requested - why for this release]: characters are broken in built-in pdf viewer

I can reproduce the issue on Nightly 67.0a1 Windows10

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5097db630d1fb51f7f3fcd5523a924fa823e77ce&tochange=caa65bf34838ab7861dcfed3ae59f9f6ed89fc0b

Regressed by:
caa65bf34838 Ryan VanderMeulen — Bug 1489996 - Update pdf.js to version 2.0.843. r=bdahl

Blocks: 1489996
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Unspecified → All

:RyanVM,
The bunch of patch causes the regression. Can you please look into this?

Flags: needinfo?(ryanvm)

Alice, I'm not an active developer of pdf.js - I just import the upstream changes. Redirecting to bdahl who's a better point of contact.

Flags: needinfo?(ryanvm) → needinfo?(bdahl)

I've submitted https://github.com/mozilla/pdf.js/pull/10539 upstream, and while it does not fix the underlying bug it will at least provide a better fallback behaviour for fonts that are rejected by the sanitizer.

Assignee: nobody → bdahl

Brendan, do you think this is low risk enough for beta uplift?

Oh, wait, there's no patch, just an upstream fix. never mind!

Flags: needinfo?(bdahl)

(In reply to Jonas Jenwald [:Snuffleupagus] from comment #5)

I've submitted https://github.com/mozilla/pdf.js/pull/10539 upstream, and
while it does not fix the underlying bug it will at least provide a better
fallback behaviour for fonts that are rejected by the sanitizer.

Bug 1527714 was just pushed with the changes from this upstream commit included. Would be great if someone could verify it with a Nightly build after it's merged to mozilla-central.

This is a straight backport of the upstream commit run through the usual pdf.js updating process. It applied cleanly to the base revision being shipped in 66.

Attachment #9043707 - Flags: review?(bdahl)

I can confirm that the pdf linked in comment #0 is now displayed with no weird characters in today's nightly.

Attachment #9043707 - Flags: review?(bdahl) → review+

Comment on attachment 9043707 [details] [diff] [review]
backport the upstream commit to Beta

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1489996

User impact if declined

Certain characters in PDFs will not show correctly.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

See comment #1

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Adds a new fallback code path, shouldn't affect the default way PDFs are rendered.

String changes made/needed

Attachment #9043707 - Flags: approval-mozilla-beta?
Comment on attachment 9043707 [details] [diff] [review]
backport the upstream commit to Beta

Fix for a bad regression in PDF font display, verified in nightly.
Let's uplift for beta 10.
Attachment #9043707 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: NEW → RESOLVED
Closed: 9 months ago
Flags: qe-verify+
Resolution: --- → FIXED

This should still be fixed "properly", what's been implemented thus far is simply a (generally useful) work-around.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Let's file a follow-up bug for the proper fix then. This bug has already had a patch land across two different releases and it'll become increasingly difficult to keep track of things if we start adding more onto it.

Status: REOPENED → RESOLVED
Closed: 9 months ago9 months ago
Resolution: --- → FIXED
Whiteboard: [qa-triaged]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:66.0) Gecko/20100101 Firefox/66.0
Build ID: 20190221160854

Verified as fixed on the latest Beta build (66b10).

Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Whiteboard: [qa-triaged]
Whiteboard: [qa-triaged]
You need to log in before you can comment on or make changes to this bug.