The Node.js url.parse API (https://nodejs.org/api/url.html#urlparseurlstring-parsequerystring-slashesdenotehost)
is deprecated because it's prone to security issues (to the point that Node.js doesn't even publish CVEs for it anymore).
The official reccomendation is to instead use the global URL constructor, available both in Node.js and in browsers.
Node.js filesystem APIs accept URL objects as parameter, so this also avoids a few URL->filepath conversions.
This major version contains three breaking changes that impact us:
- The `product` option has been renamed to the more suitable `browser`.
- The `page.screenshot()` API returns a `Uint8Array` instead of a
`Buffer`, but since `pngjs` requires a `Buffer` object we need to do
the conversion using `Buffer.from()` before passing data to `pngjs`.
- The browser configuration should be set using a configuration file
instead of environment variables. Note that as a bonus this allows us
to remove the `cross-env` dependency since that was only used to set
the Puppeteer environment variable equally for all operating systems.
For more information about the changes between the old and new Puppeteer
versions refer to https://github.com/puppeteer/puppeteer/releases.
In PR #18356 the new tab page logic was disabled to prevent Firefox
from logging failed network connections to Contile, the Mozilla Tiles
service that is used for the new tab page [1]. However, recently this
log reappeared locally and on the bots:
```
console.warn: TopSitesFeed: Failed to fetch data from Contile server:
NetworkError when attempting to fetch resource.
```
It looks like Contile communication is also triggered from other places
in Firefox such as the URL bar [2], so this commit fixes the issue by
disabling network connections to Contile [3] altogether regardless of
their origin within Firefox. Note that we don't revert the change from
PR #18356 because as noted in [4] it can't hurt to keep that disabled
too to avoid overhead for a feature we don't use in the tests.
[1] https://github.com/mozilla-services/contile
[2] 196ef8360e/browser/components/urlbar/UrlbarProviderTopSites.sys.mjs (L38)
[3] 196ef8360e/browser/components/newtab/lib/TopSitesFeed.sys.mjs (L111)
[4] https://github.com/mozilla/pdf.js/pull/18356#issuecomment-2200354730
If uncaught exceptions occur in the tests (which happened in #17962 and
can be triggered manually by throwing an error in `integration-boot.js`)
the teardown logic of the tests doesn't get to run and thus spawned
browser processes are not closed properly. Given that `test.mjs` is the
only process that has a reference to them they will become orphaned and
keep running if `test.mjs` exits without explicitly closing them.
This commit fixes the issue by always closing the browsers if uncaught
exceptions occur, and we make sure to log them for debugging purposes.
It should help to have such a garbage in the logs:
```
console.warn: TopSitesFeed: Failed to fetch data from Contile server: NetworkError when attempting to fetch resource.
JavaScript error: , line 0: TypeError: NetworkError when attempting to fetch resource.
```
This commit fixes the following log line that currently shows up for
every type of tests that involve a browser:
`System addon update list error SyntaxError: XMLHttpRequest.open:
'http://%(server)s/dummy-system-addons.xml' is not a valid URL.`
If Firefox is in testing mode, the system addons update URL is
configured to a dummy URL so that it can't actually update (see for
example the same value in Marionette at
https://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/geckoinstance.py#109),
but this doesn't stop Firefox from trying to update, and when it does
it logs this line because the URL is obviously invalid.
Hence this patch which disables system addon updates altogether so
Firefox doesn't attempt to use the dummy URL anymore. The browser
updates are all managed by Puppeteer, and regular updates have already
been disabled too (see
6937a76f0a/packages/browsers/src/browser-data/firefox.ts (L302-L303)).
When seleciting on a touch screen device, whenever the finger moves to a
blank area (so over `div.textLayer` directly rather than on a `<span>`),
the selection jumps to include all the text between the beginning of the
.textLayer and the selection side that is not being moved.
The existing selection flickering fix when using the mouse cannot be
trivially re-used on mobile, because when modifying a selection on
a touchscreen device Firefox will not emit any pointer event (and
Chrome will emit them inconsistently). Instead, we have to listen to the
'selectionchange' event.
The fix is different in Firefox and Chrome:
- on Firefox, we have to make sure that, when modifying the selection,
hovering on blank areas will hover on the .endOfContent element
rather than on the .textLayer element. This is done by adjusting the
z-indexes so that .endOfContent is above .textLayer.
- on Chrome, hovering on blank areas needs to trigger hovering on an
element that is either immediately after (or immediately before,
depending on which side of the selection the user is moving) the
currently selected text. This is done by moving the .endOfContent
element around between the correct `<span>`s in the text layer.
The new anti-flickering code is also used when selecting using a mouse:
the improvement in Firefox is only observable on multi-page selection,
while in Chrome it also affects selection within a single page.
After this commit, the `z-index`es inside .textLayer are as follows:
- .endOfContent has `z-index: 0`
- everything else has `z-index: 1`
- except for .markedContent, which have `z-index: 0`
and their contents have `z-index: 1`.
`.textLayer` has an explicit `z-index: 0` to introduce a new stacking context,
so that its contents are not drawn on top of `.annotationLayer`.
In Node.js 14.14.0 the `fs.rmSync` function was added that removes files
and directories. The `recursive` option is used to remove directories
and their contents, making it a drop-in replacement for the `rimraf`
dependency we use.
Given that PDF.js now requires Node.js 18+ we can be sure that this
option is available, so we can safely remove `rimraf` and reduce the
number of project dependencies in the test folder.
This commit also gets rid of the indirection via the `removeDirSync`
test helper function by simply calling `fs.rmSync` directly.
Co-authored-by: Wojciech Maj <kontakt@wojtekmaj.pl>
This commit removes the final callbacks in this code by switching to a
promises-based interface, overall simplifying the code. Moreover, we
document why we write to files on disk and modernize the code using e.g.
template strings.
The original `test.py` code, see
c2376e5cea/test/test.py,
did not have any timeout logic for TTX, but it got introduced when
`test.py` was ported from Python to JavaScript as `test.js` in
c2376e5cea (diff-a561630bb56b82342bc66697aee2ad96efddcbc9d150665abd6fb7ecb7c0ab2f).
However, I don't think we've ever actually seen TTX timing out.
Moreover, back then we used a very old version of TTX and ran the font
tests on the bots (where a hanging process would block other jobs and
would require a manual action to fix), so this code was most likely
only included defensively.
Fortunately, nowadays it should not be necessary anymore because we use
the most recent version of TTX (which either returns the result or
errors out, but isn't known to hang on inputs) and we run the font tests
on GitHub Actions which doesn't block other jobs anymore and also
automatically times the job out for us in the unlikely event that a hang
would ever occur.
In short, we can safely remove this logic to simplify the code and to get
rid of a callback.
In implementing caret browsing mode in pdf.js, I didn't notice that selectstart isn't always triggered.
So this patch removes the use of selectstart and rely only on selectionchange.
In order to simplify the selection management, the selection code is moved in the AnnotationUIManager:
- it simplifies the code;
- it allows to have only one listener for selectionchange instead of having one by visible page
for selectstart.
I had to add a delay in the integration tests for highlighting (there's a comment with an explanation),
it isn't really nice, but it's the only way I found and in real life there always is a delay between
press and release.
The `if` statement is no longer necessary because the Node.js versions
that didn't provide `dns.setDefaultResultOrder` are no longer supported,
but looking into this a bit more it turns out that the entire workaround
is no longer necessary because the issue got fixed in Firefox 105 in bug
1769994. Indeed, Firefox now starts nicely with the workaround removed.
Reverts 60ed3cd297.
The test helper code largely predates the introduction of modern
JavaScript features and should be refactored to improve readability.
In particular callbacks make the code harder to understand and maintain.
This commit:
- replaces the callback argument with returning a promise;
- replaces the recursive function calls with a simple loop;
- uses `const`/`let` instead of `var`;
- uses arrow functions for shorter code;
- uses template strings for shorter string formatting code.
The test helper code largely predates the introduction of modern
JavaScript features and should be refactored to improve readability.
In particular callbacks and recursive function calls make the code
harder to understand and maintain.
This commit:
- replaces the callback argument with returning a promise;
- replaces the recursive function calls with a simple loop;
- uses `const`/`let` instead of `var`;
- uses template strings for shorter string formatting code;
- improves the error messages to have more details.
The goal is to be able to get these outlines to fill the shape corresponding
to a text selection in order to highlight some text contents.
The outlines will be used either to show selected/hovered highlights.
This commit prepares for running the font tests on GitHub Actions where
we can't spin up headful browsers because there are no display
capabilities on the workers. This will also be useful for porting other
test targets to GitHub Actions at a later time, as well as running the
tests locally in headless mode.
This commit prepares for the introduction of extra options in later
commits by changing the function signatures of the `startBrowser(s)`
functions to take parameter objects instead of plain parameters. This
makes the call sites explicitly state which parameters they pass,
improving overall readability as well.
The current logic is more complicated than it needs to be because it's
passing a callback function to `startBrowsers` instead of a string.
This commit simplifies the logic by passing the base URL as a string to
`startBrowsers` and having it do further augmentation internally,
thereby removing all indirection of the function calls to `makeTestUrl`
and the inner function it returned.
The previous change that set the timeout had effect because we have seen
quite a few protocol timeouts now correctly being raised in the context
of the active test, however we have also still seen a handful of cases
where this wasn't the case and the one second difference turned out to
be too low (likely because the operation was started slightly after one
second into the test run). We therefore tweak the value to be 75% of the
Jasmine timeout. This should be enough to catch operations that happen
later on in the test run, and if a single operation takes that long any
hope for success is already gone anyway.
The default protocol timeout is 180 seconds according to the
documentation at https://pptr.dev/api/puppeteer.browserconnectoptions,
but the Jasmine timeout we configure in the individual boot files is 30
seconds. The consequence of this is that if a protocol (CDP) error
occurs after 30 seconds Jasmine will fail the test, but the actual
protocol error from Puppeteer is raised much later in the context of
another test, which causes unrelated failures or tracebacks.
This commit fixes the problem by configuring Puppeteer to always use a
lower protocol timeout than the Jasmine timeout so that protocol errors
are always raised in the context of the test that actually triggered it.
Occasionally some test-suites may fail to start on the bots, however that's not correctly reflected in the botio-output posted to GitHub which makes it easy to accidentally overlook this situation.
Looking at the raw logs when that happens they always seem to contain a line such as `Run NaN tests` which means that we should be able to easily make this situation a *failure* as intended.
