1006714 - 'Find' fails to search whole PDF with pdf.js when opened in a new tab

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect)

Description

[bugcon]

Re-report of bug 941177 with more accurate bug behavior description.

Steps to reproduce:
1. Open a PDF in a new tab (example PDF links below)
2. Search for a term near the bottom of the PDF

Actual results:
For Link 1 where pdf.js is client side, term is not found
For Link 2 where pdf.js is server side, term _is_ found _only_ when using pdf.js's built-in search (magnifying glass icon in toolbar at top of page), however, using Firefox's own find bar (e.g via find-as-you-type) does _not_ find the term.

Expected results:
Term should be found regardless of how far down the docuement it is.

Link 1: http://www.hupfeld-software.de/files/Dynasys-Handbuch.pdf
Link 2: http://mozilla.github.io/pdf.js/web/viewer.html

Comment 1

[Yury Delendik (:yury)]

note: that this is broken only for "open in new tab" only; it works fine for "open in new window", regular hyperlink click opening or typing address in the address bar.

Comment 2

[Yury Delendik (:yury)]

None of the events are called when PDF viewer is opened in new tab.

http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#718

Looks like it's a regression:

Last good nightly: 2013-07-07
First bad nightly: 2013-07-08

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f8a08d0a1b2a&tochange=17a47dcef75d

Suspect is
f29cb83f66bd	Adam Dane [:hobophobe] — Bug 537013 - Make the find bar exist on a per-tab basis. r=dao

Are we getting wrong gFindBar at http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#919 ?

Comment 3

[Adam]

Attachment: Naive patch

Yes, it will need to use |tabbrowser.getFindBar(someTab)| to get the correct findbar: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#158

gFindBar only points to the active tab's findbar now.

Same looks to be true for lines 322, 323, and 440 of PdfStreamConverter.jsm as well.

I've attached a simple/untested patch. Basically adds a function |getFindBar| to lookup the tabbrowser, tab, and finally use that to get the findbar. I'm not very familiar with the PDF.jsm code, so it may be better to e.g., cache the findbar if it will be gotten frequently, etc.

Comment 4

[Jonas Jenwald [:Snuffleupagus]]

Unfortunately the patch will not work as is, since it prevents the PDF Viewer from loading with the following error in the console: "browser.getTabBrowser is not a function (PdfStreamConverter.jsm:66)".

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8418444 [details] [diff] [review]
Naive patch

>+  var browser = getChromeWindow(domWindow);

getChromeWindow returns the chrome window, not a browser.

Comment 6

[Adam]

Thanks for the feedback!

As PDF.js is a github project I submitted a pull request there: https://github.com/mozilla/pdf.js/pull/4753

Comment 7

[Yury Delendik (:yury)]

We might want to try to uplift this patch (without the test though) to FF31.

Comment 8

[Petruta Horea [:phorea], Desktop QA]

Set to New as per above comments.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Adam, could you fill the uplift request? Thanks

Comment 10

[Adam]

Attachment: Patch for uplift

[Approval Request Comment]
Bug caused by (feature/regressing bug #): per-tab findbar, bug 537013
User impact if declined: Finding in PDFs displayed via PDF.js will not always work.
Testing completed (on m-c, etc.): Two weeks on m-c with one minor issue (bug 1008450, intermittent test failure caused in combination with findbar code that's new to m-c, but that's been fixed on m-c and wouldn't affect Aurora).
Risk to taking this patch (and alternatives if risky): Low risk, as this only changes how the extension locates the findbar (score-or-so of lines changed in one file that is only used by the extension).
String or IDL/UUID changes made by this patch: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46f6b49cc290

Fixed on trunk by bug 1007627.

Comment 12

[Yury Delendik (:yury)]

bug 1008450 might need to uplifted as well

Comment 13

[Adam]

Bug 1008450 should not need uplift because it was caused by a combination of this bug fix and another code change (bug 257061) that's new to mozilla-central/not on Aurora.

Comment 14

[Paul Silaghi, QA [:pauly]]

Reproduced in nightly 2014-05-06.
Verified fixe 32.0a1 (2014-05-25), Win 7 x64

Comment 15

[Bogdan Maris, Desktop QA]

Reproduced in Nightly (2014-05-06), verified as fixed on Windows 7 64bit, Mac OS X 10.9.4 and Ubuntu 13.04 64bit using Firefox 31 beta 6.