1823401 - Wrong values shown in PDF viewer

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect, P1)

Description

[filippoluchini]

Attachment: testcase.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/111.0

Steps to reproduce:

Unzip the attached file, that contains two valid PDF/A-3B files and a png.
Open CASO_A1.pdf in firefox using its PDF viewer. Then open CASO_B1.pdf in another tab of firefox using its PDF viewer.

Actual results:

CASO_A1.pdf shows correct data. CASO_B1.pdf shows uncorrect data (it seems to change values of bold numeric characters, eg exams results and dates), though values are right when inspecting the page (see inspect.png).
When opening CASO_B1.pdf first and CASO_A1.pdf then, CASO_B1.pdf shows correct data and CASO_A1.pdf shows uncorrect data.

Expected results:

Both CASO_A1.pdf and CASO_B1.pdf show correct data.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::PDF Viewer' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Calixte Denizet (:calixte)]

I can reproduce the issue on Windows but not on mac.
I ran mozregression and I found the following pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=17e3c9ff2cad9711445a6c39df85925c8f347336&tochange=afa247928245d02b1305a1850eb39f19b84a2f35

and this patch looked like a good potential culprit:
Bob Owen — Bug 1547286: Enable remote Canvas 2D in Windows Nightly builds. r=jrmuizel

hence I set gfx.canvas.remote: false and the bug disappeared.

:jrmuizel, would you have any idea on what could be wrong ?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

moving NI to Bob

Comment 4

[Bob Owen (:bobowen)]

That is very weird.
Seems like if B is opened first bold numbers are translated like this:
0123456789
0738924561

If A is opened first it's the other way around.

Comment 5

[Roberto Benedetti]

I tested those PDFs on a fresh Windows 11 install (VirtualBox) with Firefox 111 and bold numbers show up right.
On my Windows 10 laptop the issue can be reproduced.

Comment 6

[Bob Owen (:bobowen)]

OK my guess at the moment is that they are both using embedded fonts.
The key that we create to cache the font is the same, but for some reason the number glyphs are in different orders and so get translated.

Comment 7

[Calixte Denizet (:calixte)]

The two pdfs contains two different fonts which are the two subsets of an identical font.
Each font is extracted from the pdf and then loaded in using a FontFace object.
For those pdfs the loaded fonts have the same name: g_d0_f2 but their content are different.
How is the key in the cache computed ?

Comment 8

[Bob Owen (:bobowen)]

(In reply to Calixte Denizet (:calixte) from comment #7)

The two pdfs contains two different fonts which are the two subsets of an identical font.
Each font is extracted from the pdf and then loaded in using a FontFace object.
For those pdfs the loaded fonts have the same name: g_d0_f2 but their content are different.
How is the key in the cache computed ?

It's a hash of the head table bytes.

Comment 9

[Bob Owen (:bobowen)]

jfkthame: hashing the head tables for the font cache key isn't working for these PDFs, any ideas for an improvement to fix this?

Comment 10

[Jonathan Kew [:jfkthame]]

Presumably hashing the entire font would substantially reduce the risk of a collision, but that could be significantly more expensive.

Maybe include the URL of the document to which the font belongs as part of the hash? That should avoid cross-document confusion.

Comment 11

[Bob Owen (:bobowen)]

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

Presumably hashing the entire font would substantially reduce the risk of a collision, but that could be significantly more expensive.

Maybe include the URL of the document to which the font belongs as part of the hash? That should avoid cross-document confusion.

This isn't a hash collision the head table bytes and font data sizes are identical.
I'm not sure how easily we can get the URL.
Perhaps we could hash the entire font if is it under a certain size, to try and allow for these subsets.

Comment 12

[Jonathan Kew [:jfkthame]]

Yes - sorry, I didn't mean we're seeing a "hash collision" in the usual sense; rather, a collision between the two subset fonts because they have the same 'head' table and the same length. I haven't checked, but my guess is they even contain the exact same subset of glyphs -- but in a different order, because they get added to the subset in the order in which they occur. So it's expected that the head-table hashes are identical. If we could include the "owning" document's URL in the hash, that would distinguish them.

If that's not feasible, maybe including the 'cmap' table in the hash would help; that should cause a difference if the glyphs are reordered. But I suppose we can't guarantee 100% reliability with anything less than the full font data.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Based on comment #2, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

:bobowen, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit auto_nag documentation.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

What is the cache for?

Comment 15

[Bob Owen (:bobowen)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)

What is the cache for?

So we don't send the font data over multiple times.
I originally added it for printing when we didn't have UnscaledFont only ScaledFont.
I was just thinking actually, do we need this since lsalzman (I think) changed this to serialize the UnscaledFont?
Meaning could we just cache using the underlying pointer like we do for other objects?

Comment 16

[Bob Owen (:bobowen)]

I'm going to go with jfkthame's cmap plan (at least initially), it should fix issues like this one and is simple for uplifting.

Comment 17

[Bob Owen (:bobowen)]

Attachment: Bug 1823401: Hash cmap as well as head tables for SFNTData::GetUniqueKey. r=jfkthame!

Comment 18

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/jobs?repo=try&revision=fa10525c0e1f7bd8ca4d8c5a31160dd2336576a1

Comment 19

[Bob Owen (:bobowen)]

Attachment: Bug 1823401: Hash cmap as well as head tables for SFNTData::GetUniqueKey. r=jfkthame!

Original Revision: https://phabricator.services.mozilla.com/D173252

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f60561f5faf4
Hash cmap as well as head tables for SFNTData::GetUniqueKey. r=jfkthame

Comment 21

[Bob Owen (:bobowen)]

Comment on attachment 9324361 [details]
Bug 1823401: Hash cmap as well as head tables for SFNTData::GetUniqueKey. r=jfkthame!

Beta/Release Uplift Approval Request

• User impact if declined: If two PDF documents from the same process (domain, including file) have embedded subsets of fonts with the same name and exactly the same characters, but in a different order.
  The characters in the subsequently loaded document will get scrambled.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See attached testcase and description.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fairly simple change that adds the cmap tables from the fonts into the existing hashing for the font data key.
• String changes made/needed: None
• Is Android affected?: No

Comment 22

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/f60561f5faf4

Comment 23

[Timea Zsoldos [:tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 113.0a1 (2023.03.20) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Firefox 113.0a1 latest nightly (2023.03.23) on Windows 10, Ubuntu 22 and on macOS 12.
Tested with Windows 10 using Firefox 111.0.1, still affected.
Will have to verify on Fx 112.0b6 and on Fx 102.10.0esr official builds, once they're available.

Comment 24

[Dianna Smith [:diannaS]]

Comment on attachment 9324361 [details]
Bug 1823401: Hash cmap as well as head tables for SFNTData::GetUniqueKey. r=jfkthame!

Approved for 112.0b6

Comment 25

[Dianna Smith [:diannaS]]

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

Comment 26

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-03-20 Nightly build on Windows 10.
Verified as fixed on Firefox 102.0b6(build ID: 20230323181038) on Windows 10, Ubuntu 22, macOS 12.

Comment 27

[Timea Zsoldos [:tzsoldos], Desktop QA]

Based on comment 23 I changed the tracking flag for Firefox 111 to affected.

Comment 28

[Bob Owen (:bobowen)]

Pretty sure all these statuses got set to fixed by mistake early in the bug.
ESR102 is affected as far as I can tell from hg.mozilla.org

Comment 29

[Bob Owen (:bobowen)]

Comment on attachment 9324361 [details]
Bug 1823401: Hash cmap as well as head tables for SFNTData::GetUniqueKey. r=jfkthame!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Quite a serious bug that can result in the incorrect information to be displayed on PDFS.
• User impact if declined: If two PDF documents from the same process (domain, including file) have embedded subsets of fonts with the same name and exactly the same characters, but in a different order.
  The characters in the subsequently loaded document will get scrambled.
• Fix Landed on Version: 113
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fairly simply change that just adds the cmap font table bytes into an existing hash.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9324361 [details]
Bug 1823401: Hash cmap as well as head tables for SFNTData::GetUniqueKey. r=jfkthame!

Approved for 102.10esr.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/d4114770d3bd

Comment 32

[Gian-Carlo Pascutto [:gcp]]

I'm going to mark this as S1. There may be a workaround (with prefs), but it's not guaranteed that you immediately notice this bug exists, and if you don't, you could have something that can be loosely described as "dataloss". It's definitely a nasty bug.

Comment 33

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-03-20 Nightly build on Windows 10.
Verified as fixed on Firefox-esr 102.10.0(build ID: 20230403141754) on Windows 10, Ubuntu 22, macOS 12.