Fix `DOMFilterFactory.#createUrl` in MOZCENTRAL builds (18417 PR follow-up) #18430

Snuffleupagus · 2024-07-13T09:02:56Z

Somehow I managed to mess up the URL creation relevant to e.g. MOZCENTRAL builds, which is breaking the pending PDF.js update in mozilla-central; sorry about that!

timvandermeij

r=me, with passing tests.

timvandermeij · 2024-07-13T11:19:38Z

/botio test

moz-tools-bot · 2024-07-13T11:19:41Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/d237c8bc52a1012/output.txt

moz-tools-bot · 2024-07-13T11:19:41Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/feb921e4f5ea90c/output.txt

calixteman

Arrff... I missed that too.
Thank you.

calixteman · 2024-07-13T11:28:11Z

This test should have failed:

pdf.js/test/test_manifest.json

Lines 8777 to 8782 in 2d25437

    
           "id": "issue16114", 
        
           "file": "pdfs/issue16114.pdf", 
        
           "md5": "c04827ea33692e0f94a5e51716d9aa2e", 
        
           "rounds": 1, 
        
           "link": true, 
        
           "type": "eq"

.
@Snuffleupagus do you know why it didn't ?

moz-tools-bot · 2024-07-13T11:49:36Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/feb921e4f5ea90c/output.txt

Total script time: 29.91 mins

Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 11
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/feb921e4f5ea90c/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2024-07-13T11:52:10Z

do you know why it didn't ?

Yes, because of the pre-processor; see

pdf.js/src/display/display_utils.js

Lines 126 to 143 in 2d25437

    
           #createUrl(id) { 
        
             if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { 
        
               if (this.#baseUrl === undefined) { 
        
                 const url = this.#document.URL; 
        
                 if (url === this.#document.baseURI) { 
        
                   // No `<base>`-element present, hence a relative URL should work. 
        
                   this.#baseUrl = ""; 
        
                 } else if (isDataScheme(url)) { 
        
                   warn('#createUrl: ignore "data:"-URL for performance reasons.'); 
        
                   this.#baseUrl = ""; 
        
                 } else { 
        
                   this.#baseUrl = url.split("#", 1)[0]; 
        
                 } 
        
               } 
        
               return `url(${this.#baseUrl}#${id})`; 
        
             } 
        
             return `url(${id})`; 
        
           }

The idea was to ensure that we actually test the GENERIC code in the PDF.js test-suite, since the Firefox code-path is covered by tests in mozilla-central. Perhaps we should just use the #baseUrl feature regardless of build-target here?

moz-tools-bot · 2024-07-13T12:03:51Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/d237c8bc52a1012/output.txt

Total script time: 44.15 mins

Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 2

Image differences available at: http://54.193.163.58:8877/d237c8bc52a1012/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2024-07-13T12:51:08Z

@calixteman Would you prefer the following diff instead, where we'd always check if a baseUrl is needed?

diff --git a/src/display/display_utils.js b/src/display/display_utils.js
index 30964c9ab..9d4a41723 100644
--- a/src/display/display_utils.js
+++ b/src/display/display_utils.js
@@ -124,22 +124,19 @@ class DOMFilterFactory extends BaseFilterFactory {
   }
 
   #createUrl(id) {
-    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
-      if (this.#baseUrl === undefined) {
-        const url = this.#document.URL;
-        if (url === this.#document.baseURI) {
-          // No `<base>`-element present, hence a relative URL should work.
-          this.#baseUrl = "";
-        } else if (isDataScheme(url)) {
-          warn('#createUrl: ignore "data:"-URL for performance reasons.');
-          this.#baseUrl = "";
-        } else {
-          this.#baseUrl = url.split("#", 1)[0];
-        }
+    if (this.#baseUrl === undefined) {
+      const url = this.#document.URL;
+      if (url === this.#document.baseURI) {
+        // No `<base>`-element present, hence a relative URL should work.
+        this.#baseUrl = "";
+      } else if (isDataScheme(url)) {
+        warn('#createUrl: ignore "data:"-URL for performance reasons.');
+        this.#baseUrl = "";
+      } else {
+        this.#baseUrl = url.split("#", 1)[0];
       }
-      return `url(${this.#baseUrl}#${id})`;
     }
-    return `url(${id})`;
+    return `url(${this.#baseUrl}#${id})`;
   }
 
   addFilter(maps) {

calixteman · 2024-07-15T11:57:14Z

@calixteman Would you prefer the following diff instead, where we'd always check if a baseUrl is needed?

diff --git a/src/display/display_utils.js b/src/display/display_utils.js
index 30964c9ab..9d4a41723 100644
--- a/src/display/display_utils.js
+++ b/src/display/display_utils.js
@@ -124,22 +124,19 @@ class DOMFilterFactory extends BaseFilterFactory {
   }
 
   #createUrl(id) {
-    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
-      if (this.#baseUrl === undefined) {
-        const url = this.#document.URL;
-        if (url === this.#document.baseURI) {
-          // No `<base>`-element present, hence a relative URL should work.
-          this.#baseUrl = "";
-        } else if (isDataScheme(url)) {
-          warn('#createUrl: ignore "data:"-URL for performance reasons.');
-          this.#baseUrl = "";
-        } else {
-          this.#baseUrl = url.split("#", 1)[0];
-        }
+    if (this.#baseUrl === undefined) {
+      const url = this.#document.URL;
+      if (url === this.#document.baseURI) {
+        // No `<base>`-element present, hence a relative URL should work.
+        this.#baseUrl = "";
+      } else if (isDataScheme(url)) {
+        warn('#createUrl: ignore "data:"-URL for performance reasons.');
+        this.#baseUrl = "";
+      } else {
+        this.#baseUrl = url.split("#", 1)[0];
       }
-      return `url(${this.#baseUrl}#${id})`;
     }
-    return `url(${id})`;
+    return `url(${this.#baseUrl}#${id})`;
   }
 
   addFilter(maps) {

Let's do that: I think it's slightly simpler and it shouldn't add any specific penalty.

calixteman · 2024-07-15T11:58:00Z

@Snuffleupagus, could you merge this PR asap, and then I'll make a new release.

…ow-up) Somehow I managed to mess up the URL creation relevant to e.g. MOZCENTRAL builds, which is breaking the pending PDF.js update in mozilla-central; sorry about that! To avoid future issues, we'll now always check if absolute filter-URLs are necessary regardless of the build-target.

Snuffleupagus · 2024-07-15T12:18:08Z

/botio test

moz-tools-bot · 2024-07-15T12:18:11Z

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/a3981fdab0cd3a1/output.txt

moz-tools-bot · 2024-07-15T12:18:11Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/405f05fa597a2a8/output.txt

moz-tools-bot · 2024-07-15T12:48:07Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/405f05fa597a2a8/output.txt

Total script time: 29.93 mins

Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 10
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/405f05fa597a2a8/reftest-analyzer.html#web=eq.log

moz-tools-bot · 2024-07-15T13:02:33Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/a3981fdab0cd3a1/output.txt

Total script time: 44.35 mins

Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 3

Image differences available at: http://54.193.163.58:8877/a3981fdab0cd3a1/reftest-analyzer.html#web=eq.log

Snuffleupagus added core regression firefox-specific labels Jul 13, 2024

timvandermeij approved these changes Jul 13, 2024

View reviewed changes

calixteman approved these changes Jul 13, 2024

View reviewed changes

timvandermeij mentioned this pull request Jul 13, 2024

Disable network connections to Contile for the tests #18433

Merged

Snuffleupagus force-pushed the pr-18417-followup branch from 2f6551d to 4c45948 Compare July 15, 2024 12:16

Snuffleupagus merged commit f9e3b6b into mozilla:master Jul 15, 2024
9 checks passed

Snuffleupagus deleted the pr-18417-followup branch July 15, 2024 13:05

npanchal108 mentioned this pull request Aug 31, 2024

[Snyk] Upgrade pdfjs-dist from 4.0.269 to 4.5.136 npanchal108/ecommercebase#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `DOMFilterFactory.#createUrl` in MOZCENTRAL builds (18417 PR follow-up) #18430

Fix `DOMFilterFactory.#createUrl` in MOZCENTRAL builds (18417 PR follow-up) #18430

Snuffleupagus commented Jul 13, 2024

timvandermeij left a comment

timvandermeij commented Jul 13, 2024

moz-tools-bot commented Jul 13, 2024

moz-tools-bot commented Jul 13, 2024

calixteman left a comment

calixteman commented Jul 13, 2024

moz-tools-bot commented Jul 13, 2024

Snuffleupagus commented Jul 13, 2024

moz-tools-bot commented Jul 13, 2024

Snuffleupagus commented Jul 13, 2024

calixteman commented Jul 15, 2024

calixteman commented Jul 15, 2024

Snuffleupagus commented Jul 15, 2024

moz-tools-bot commented Jul 15, 2024

moz-tools-bot commented Jul 15, 2024

moz-tools-bot commented Jul 15, 2024

moz-tools-bot commented Jul 15, 2024

Fix DOMFilterFactory.#createUrl in MOZCENTRAL builds (18417 PR follow-up) #18430

Fix DOMFilterFactory.#createUrl in MOZCENTRAL builds (18417 PR follow-up) #18430

Conversation

Snuffleupagus commented Jul 13, 2024

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij commented Jul 13, 2024

moz-tools-bot commented Jul 13, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Jul 13, 2024

From: Bot.io (Linux m4)

Received

calixteman left a comment

Choose a reason for hiding this comment

calixteman commented Jul 13, 2024

moz-tools-bot commented Jul 13, 2024

From: Bot.io (Linux m4)

Failed

Snuffleupagus commented Jul 13, 2024

moz-tools-bot commented Jul 13, 2024

From: Bot.io (Windows)

Failed

Snuffleupagus commented Jul 13, 2024

calixteman commented Jul 15, 2024

calixteman commented Jul 15, 2024

Snuffleupagus commented Jul 15, 2024

moz-tools-bot commented Jul 15, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Jul 15, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Jul 15, 2024

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Jul 15, 2024

From: Bot.io (Windows)

Failed

Fix `DOMFilterFactory.#createUrl` in MOZCENTRAL builds (18417 PR follow-up) #18430

Fix `DOMFilterFactory.#createUrl` in MOZCENTRAL builds (18417 PR follow-up) #18430