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.

Sign up for GitHub

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

Avoid an URI mismatch when looking for a filter id in the Firefox viewer (bug 1821408) #16144

Closed
wants to merge 1 commit into from
Closed

Avoid an URI mismatch when looking for a filter id in the Firefox viewer (bug 1821408) #16144

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/display/display_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,15 @@ class FilterFactory {

#_defs;

#baseURL = null;

#document;

#id = 0;

constructor({ ownerDocument = globalThis.document } = {}) {
this.#document = ownerDocument;
this.#baseURL = this.#document?.URL ? new URL(this.#document.URL) : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How "well" will this work for PDF documents opened with a data:-URL, since those can be arbitrary long?
As seen in e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1803050 those URLs can be really problematical, so should we perhaps use document.baseURI instead here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

baseURI is the problem here: in the builtin viewer, baseURI is resource://pdf.js/web which mismatches with the document URI.
So as far as I can tell we must keep the URL . The comparison is firstly made with scheme and path and in the data case the scheme is data and the path is application/data.... so we really need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if it becomes a problem (I mean someone complains about that) then we'll see what to do.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

baseURI is the problem here: in the builtin viewer, baseURI is resource://pdf.js/web which mismatches with the document URI.

Sorry, but why can't we just update the platform to handle this instead?

Looking at the Firefox code, we already have a bunch of special-cases in both JS and C++ code for that: https://searchfox.org/mozilla-central/search?q=resource%3A%2F%2Fpdf.js%2Fweb&path=
Also, there's even an existing C++ helper to check if something is the built-in PDF Viewer: https://searchfox.org/mozilla-central/rev/2f9a79d1c4ae7090cf50d93be86ba9dc2767a733/dom/base/nsContentUtils.cpp#6987-6995

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's what I did but the absolute url stuff did the job and I overlooked the data case:
https://phabricator.services.mozilla.com/D172223?id=691180

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this patch won't handle data:-URLs well, I really think that we should fix this on the Firefox side instead.

As mentioned we already have a bunch of "is built-in PDF Viewer"-checks in the Firefox C++ code, using the existing helper, so I don't understand why we can't add one more to address this bug as well; please see https://searchfox.org/mozilla-central/search?q=nsContentUtils%3A%3AIsPDFJS&path=&case=false&regexp=false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An other solution is to remove <base> from:

pdf.js/web/viewer-snippet-firefox-extension.html

Line 2 in 4a4c5d0

<base href="resource://pdf.js/web/">

which is used to compute the url of the filter.
There is a more general discussion on how resolve url in css:
w3c/csswg-drafts#3320

Copy link
Collaborator

Choose a reason for hiding this comment

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

An other solution is to remove <base> from:

That seems extremely risky, since any number of things in both the viewer and the platform code most likely depends on that being present; can we please not consider this here.

Still, my question here is what's so "special" about this case that we cannot be allowed to add one more special-case in the platform code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An other solution is to remove <base> from:

That seems extremely risky, since any number of things in both the viewer and the platform code most likely depends on that being present; can we please not consider this here.

I agree about the riskyness of removing <base>.

Still, my question here is what's so "special" about this case that we cannot be allowed to add one more special-case in the platform code?

Because ideally it should work without adding such an exception even if we're in a corner case.

@emilio ^^

}

get #cache() {
Expand Down Expand Up @@ -125,7 +128,16 @@ class FilterFactory {
// https://www.w3.org/TR/SVG11/filters.html#feComponentTransferElement

const id = `transfer_map_${this.#id++}`;
const url = `url(#${id})`;
let url;
if (this.#baseURL) {
this.#baseURL.hash = id;
// We use an absolute URL to avoid any issues in Firefox builtin viewer
// (see bug 1821408).
url = `url(${this.#baseURL})`;
} else {
url = `url(#${id})`;
}

this.#cache.set(maps, url);
this.#cache.set(key, url);

Expand Down