Closed Bug 1488420 Opened Last year Closed Last year

Update pdf.js to version 2.0.815

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

References

Details

+++ This bug was initially created as a clone of Bug #1485014 +++

Changes since last update:
#10000 Remove the unused `defaultColor` property on `ColorSpace` instances
#10008 Update translations/packages and fix duplicated function name in the Esprima fixtures 
#9990 Convert the `Catalog` class, in `src/core/obj.js`, to ES6 syntax
#10007 Convert the code in `src/core/colorspace.js to use ES6 classes
#10015 Adding chunkname to async loaded code
#10020 Ensure that the built `PdfJsDefaultPreferences.jsm` file won't be affected/touched during tree-wide ESLint rule changes in `mozilla-central` (PR 9571 follow-up)
#10022 Implement text rendering modes in SVG backend
#10010 Attempt to find truncated endstream commands, in the fallback code-path, in `Parser.makeStream` (issue 10004)
#10019 Add general support for re-dispatching events, on `EventBus` instances, to the DOM
#9986 Attempt to combine separate beginText/endText sequences in `getTextContent` (issue 9984)
#10032 Replace broken link for `pr8808.pdf.link`
#10031 Add a new parameter to `JpegImage.getData` to indicate the source of the image data (issue 9513)
#10033 [api-minor] Implement a permissions API
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd24042d3bfa
Update pdf.js to version 2.0.813. r=bdahl
Backed out for bc failures on browser_pdfjs_zoom.js

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=197382273&repo=mozilla-inbound&lineNumber=28477
Flags: needinfo?(ryanvm)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8cc54d235c9
Backed out changeset fd24042d3bfa for bc failures on browser_pdfjs_zoom.js. CLOSED TREE
Hey Jonas, bisection on Try is strongly pointing at PR #9986 as the culprit for these Win10 leaks:
https://github.com/mozilla/pdf.js/commit/c94df0fef341a5ef4288b290af72f2ff43c51046

Do you have any idea what might be going on?
Flags: needinfo?(ryanvm) → needinfo?(jonas.jenwald)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)
> Hey Jonas, bisection on Try is strongly pointing at PR #9986 as the culprit
> for these Win10 leaks:
> https://github.com/mozilla/pdf.js/commit/
> c94df0fef341a5ef4288b290af72f2ff43c51046
> 
> Do you have any idea what might be going on?

Not at all, since that particular commit only contains code that runs in a Worker thread. But looking at all of the Try results, I'd agree that that commit seems clearly responsible.
However, how this ends up causing window(s) to leak I don't understand, but then again I don't know much (or anything really) about the Mochitest infrastructure. 

What's unfortunate is that I cannot reproduce these failures locally, when running the PDF.js Mochitests using an artifact build on Windows 7, hence I'm not able do any sort of debugging locally.

The commit in question isn't of vital importance, since it only implemented a tiny improvement for text-selection and as such it doesn't matter too much if it's reverted (there's certainly worse text-selection bugs around).

Ryan, my apologies for the troubles this caused you!

Before I submit a PR to revert the commit, could I maybe trouble you to please help do *one* more Try push to confirm that reverting actually fixes the issue? In particular, can you please re-test the patch from comment #1, with line https://hg.mozilla.org/integration/mozilla-inbound/rev/fd24042d3bfa#l4.3072 changed to `if (false) {` instead since that should disable this text-selection improvement?
Flags: needinfo?(jonas.jenwald) → needinfo?(ryanvm)
Ryan, thanks a lot for helping out here!

Since the latest Try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=60f45e40436e62efa5f0dc25e7a1901d6a0ce579, was green I've submitted https://github.com/mozilla/pdf.js/pull/10041 to revert the offending commit.
Flags: needinfo?(ryanvm)
Hey Jonas, nice to see you spying on my Try pushes :D

As you noted, the |if (false)| workaround does "fix" the problem. That said, I'm not entirely convinced we want to go so far as to reverting the change yet. What do you think about me updating as far as #10019 for now while we continue to investigate? I had some IRC discussions about it today and there are some things we can look at still.
Flags: needinfo?(jonas.jenwald)
To be clear, whatever is going on here is likely to be hitting a bug in either Gecko or the test harness, not with pdf.js itself.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> That said, I'm not entirely convinced we want to go so far as to reverting the change yet.

While I still don't understand the Mochitest failure itself, having just looked again (in detail) at the offending commit there could actually be a bug in one edge-cases. Please note however that I cannot see how that would cause the failures discussed here, the only effect of it would be that the selectable text appears "misplaced".

> What do you think about me updating as far as #10019 for now while we continue to investigate?

Given the above, and that I don't have a good way of debugging this (nor that much time to spend on it), the best course of action would IMHO be to back it out upstream. (As previously mentioned, the commit was only a partial solution anyway.)
Flags: needinfo?(jonas.jenwald)
OK, feel free to drop me a line if you ever have a patch you want to try running through Try again some time.
Summary: Update pdf.js to version 2.0.813 → Update pdf.js to version 2.0.815
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb928e685a3
Update pdf.js to version 2.0.815. r=bdahl
https://hg.mozilla.org/mozilla-central/rev/2eb928e685a3
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Blocks: 1489996
You need to log in before you can comment on or make changes to this bug.