1746679 - PDF viewer UI wrong theme with privacy.resistFingerprinting

Status: NEW

(Firefox :: PDF Viewer, defect)

Description

[voruti]

Attachment: comparison.jpg

Steps to reproduce:

preparation:

• fresh Windows-Sandbox on Windows 10
• open Edge, download and install the latest Firefox (currently version 95)
• enable dark theme in "about:addons"
• open a PDF file: it uses the dark theme (like expected)

bug:

• change "privacy.resistFingerprinting" to "true" in "about:config"
• switch back to the PDF file: the UI uses the white theme (which seems wrong; see attached screenshot)

Actual results:

The PDF viewer UI is using a white theme, despite everything else is set to dark mode.
When changing "privacy.resistFingerprinting" to "false", the PDF viewer UI is using the dark theme - like expected.

Bug discovered at https://www.reddit.com/r/FirefoxCSS/comments/rbvlcz/comment/hoyonet

Expected results:

I expect the PDF viewer UI to be in dark mode even with "privacy.resistFingerprinting" set to "true".

Comment 1

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

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

Comment 2

[Kenan]

confirming with Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:97.0) Gecko/20100101 Firefox/97.0 Build: 20211217212339

Comment 3

[Peter Magyari (Desktop QA)]

I was able to reproduce this issue also on MacOS 11.6

I'm setting the flags where the issue is reproducible.

Voroti, could you use mozregression in order to find the Bug that introduced this?
The issue does not occur on ESR 91.

You can download the mozregression-tool from here:
https://mozilla.github.io/mozregression/

Thank you!

Comment 4

[voruti]

First time using the mozregression-tool, here is what it gave me:
https://phabricator.services.mozilla.com/D128611

Comment 5

[Peter Magyari (Desktop QA)]

Thanks!

Comment 6

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

The severity field is not set for this bug.
:bdahl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Fatih Kilic]

We could do something like this (or this) in here but I'm not sure about the performance cost of this. I wish that we could use a specific principal for pdfjs and just compare it

Comment 9

[Tom Ritter [:tjr]]

Why do we need it? Why isn't this sufficient? Or are you saying we wouldn't use either of those specifically, but we'd need to add an odd exception for pdf.js of some type? Do you know what that exception would look like?

Comment 10

[Fatih Kilic]

Purely for performance reasons. When we were working on https://phabricator.services.mozilla.com/D212177, smaug said the following about IsSoftwareRenderingOptionExposed implementation

This looks a bit slow. There is nsCOMPtr, there is copy to nsAutoCString and then string comparison.
Have you checked how much overhead this adds to getContextAttributes() calls?
I don't know how hot the relevant canvas code is though.
One could store a principal for about:fingerprintingprotection somewhere and then do a principal->Equals check. That would be just a virtual call and some pointer comparisons.

Then we created a principal during nsContentUtils::Init and used that for about:fingerprintingprotection page, so that we could later just compare principals. I think we use it in two places so far, in Autoplay checks and IsSoftwareRenderingOptionExposed function. SImilar to smaug's comment, I also don't know how hot Gecko_MediaFeatures_PrefersColorScheme function is, so that's why I suggested it.

Also I thought we must not be the only one that checks for pdfjs in some way to provide exceptions/provide functionality to it.

Comment 11

[Tom Ritter [:tjr]]

I'm confused. It sounds like you're saying we can't add a check there because it would be costly performance wise.

But Gecko_MediaFeatures_PrefersColorScheme should already be checking for RFP and its' exceptions. Shouldn't it go into aDocument->PrefferedColorScheme() and then ShouldRFP(CSSPrefersColorScheme and then if it's a chrome or resource principal be exempted?

Comment 12

[Fatih Kilic]

It should but the problem is, Document::RecomputeResistFingerprinting() thinks the URL is file:///...pdf or http(s)://...pdf and doesn't exempt it.

Comment 13

[Tom Ritter [:tjr]]

(In reply to Fatih Kilic from comment #12)

It should but the problem is, Document::RecomputeResistFingerprinting() thinks the URL is file:///...pdf or http(s)://...pdf and doesn't exempt it.

This doesn't jive... RecomputeRFP / Document::ShouldRFP doesn't rely on URL - it should rely on the principal, right? If the principal is file:// or http(s):// then that makes sense. But then this function doesn't make sense. And that function is used in lots of important places....

Comment 14

[Fatih Kilic]

It looks like the issue is here we check for cookiejar should resistfingerprinting before we check for scheme. At least that's what the logs are showing me.

Comment 15

[Tom Ritter [:tjr]]

(In reply to Fatih Kilic from comment #14)

It looks like the issue is here we check for cookiejar should resistfingerprinting before we check for scheme. At least that's what the logs are showing me.

Okay that makes sense at a low level, Cookie Jar will override any later checks, and that's what we want. But at a higher level... why is CookieJarSettings coming into play here? That would imply (I think) that the pdf is being loaded as a Subdocument and it's inheriting the CJS from the parent document. But pdf.js documents don't look like subdocuments visually in the browser. Are they still considered subdocuments?

Comment 16

[Fatih Kilic]

I honestly don't know. This is the log I got

[Child 83425: Main Thread]: E/nsResistFingerprinting Inside RecomputeResistFingerprinting fallback case.
[Child 83425: Main Thread]: E/nsResistFingerprinting Called nsContentUtils::ShouldResistFingerprinting(nsIChannel* aChannel) with NULL channel
[Child 83425: Main Thread]: E/nsResistFingerprinting Finished RecomputeResistFingerprinting with result 1
[Child 83425: Main Thread]: E/nsResistFingerprinting Inside RecomputeResistFingerprinting fallback case.
[Child 83425: Main Thread]: E/nsResistFingerprinting Called nsContentUtils::ShouldResistFingerprinting(nsIChannel* aChannel) with NULL channel
[Child 83425: Main Thread]: E/nsResistFingerprinting Finished RecomputeResistFingerprinting with result 1
[Child 83425: Main Thread]: E/nsResistFingerprinting Inside RecomputeResistFingerprinting fallback case.
[Child 83425: Main Thread]: E/nsResistFingerprinting Called nsContentUtils::ShouldResistFingerprinting(nsIChannel* aChannel) with NULL channel
[Child 83425: Main Thread]: E/nsResistFingerprinting Finished RecomputeResistFingerprinting with result 1
[Child 83425: Main Thread]: E/nsResistFingerprinting Inside RecomputeResistFingerprinting with URI file:///.../sample.pdf channel
[Child 83425: Main Thread]: E/nsResistFingerprinting Inside ShouldResistFingerprinting(nsIChannel*) CookieJarSettingsSaysShouldResistFingerprinting said true
[Child 83425: Main Thread]: E/nsResistFingerprinting Finished RecomputeResistFingerprinting with result 1
[Child 83425: Main Thread]: E/nsResistFingerprinting Inside RecomputeResistFingerprinting with URI file:///.../sample.pdf channel
[Child 83425: Main Thread]: E/nsResistFingerprinting Inside ShouldResistFingerprinting(nsIChannel*) CookieJarSettingsSaysShouldResistFingerprinting said true
[Child 83425: Main Thread]: E/nsResistFingerprinting Finished RecomputeResistFingerprinting with result 1

RecomputeResistFingerprinting uses the fallback if/else branch for some documents but I don't know what they are. They might be pdfjs, and if they are we just do a coarse check and return without any calls to SchemeSaysShouldNotResistFingerprinting

Comment 17

[Tom Ritter [:tjr]]

ni to myself to debug this

Comment 18

[Tom Ritter [:tjr]]

So a pdf.js document seems:

• To be TYPE_DOCUMENT
• To have a null loading principal (and supposedly this is the only exception to the rule that Document types must have a loading principal)
• A Trigger Principal of System (unverified, but told this)
• A NodePrincipal() and a document->GetPrincipal() of the Null Principal

Comment 19

[Tom Ritter [:tjr]]

At this point, I'm kind of convinced that IsPDFJS is just incorrect. Code Coverage shows it's true a very few number of times, 75 being the greatest, but in others 5 times, 2 times.