1722880 - Find in Page broken after opening a PDF

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect, P2)

Description

[Mathis Raguin]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

• Open a new Tab (A tab that has already been used to search will not trigger this bug)
• On this tab, open a PDF file
• Leave the PDF and go to an HTML page
• Try to search using CTRL + F

Actual results:

The search bar will appear but typing words won't trigger a search.
Pressing enter won't trigger a search either.
Matches results are not highlighted nor displayed on the search bar.

Expected results:

The search bar appears and search as usual

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::PDF Viewer' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Alice0775 White]

I can reproduce the issue in Nightly92.0a1 Windows10.

Comment 3

[Alice0775 White]

There are 2 regressions.
#1 Open PDF with file: Protocol
#2 Open PDF with https: Protocol

#1 Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=49228a69b071bc200360aa43845b42b996759479&tochange=0637dd270ef14763921d3099b6f6d5780fa702f6

#2 Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=69f8ba80bb419c8d4fe4b785adf07f035ffbca1b&tochange=bbf2aac9fc58ff26d969c05d027507b4b03d145d

Comment 4

[Bob Owen (:bobowen)]

Attachment: Remove Pdjfs TabFindInitialized listener when navigating away from pdf.

The issue here is caused by the TabFindInitialized listener added by PdfjsParent, which isn't removed when navigating away.
So, when you bring up the findbar for the next page, it hooks up its findbar listeners and the events go missing in action.

I first tried adding removing the listener to _removeEventListener and always calling it from didDestroy, but it seems didDestroy isn't called unless we create findbar on the PDF view and then navigate. I'm not sure why that is though.

So, instead I've also added a listener in PdfjsChild to send a message to the parent when the contentWindow unloads.

This appears to work, but the really bizarre bit is, if I try and debug this, when the PDFJS:Parent:removeEventListener message arrives in receiveMessage in the debugger, both this._boundToFindbar and this._boundToTab are already null, so it doesn't appear to remove any listeners in either scenario. But I think they must be being removed otherwise things would still be broken.

Gijs - any idea what's going on with the above?

Comment 5

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #4)

Created attachment 9234286 [details] [diff] [review]
Remove Pdjfs TabFindInitialized listener when navigating away from pdf.

The issue here is caused by the TabFindInitialized listener added by PdfjsParent, which isn't removed when navigating away.
So, when you bring up the findbar for the next page, it hooks up its findbar listeners and the events go missing in action.

OK, yeah, this is bad, the event listener should be removed when the PDF unloads. I'm also not 100% sure how this existing code works with PDFs in frames...

I first tried adding removing the listener to _removeEventListener and always calling it from didDestroy, but it seems didDestroy isn't called unless we create findbar on the PDF view and then navigate. I'm not sure why that is though.

I'm confused - unless we create the findbar and then navigate - what is the "unless" case? That is, what is the case where we add the listener, but didDestroy doesn't get called when we would ideally like to remove the listener? Is it a case where we load a PDF but don't ever open the findbar? Or something else?

From a very quick test, on vanilla nightly without your patch, if I set a browser debugger breakpoint in didDestroy in PdfJsParent, open https://www.irs.gov/pub/irs-pdf/f1040.pdf and then navigate to xkcd.com, the breakpoint fires. Are you seeing something else?

Broadly, I'd expect didDestroy in the parent to be called reliably if the browsing context goes away. If it doesn't, we should talk to Nika about that, because that's bad.

Comment 6

[Bob Owen (:bobowen)]

Moving this to PDF Viewer because that's where the problem seems to be.

(In reply to :Gijs (he/him) from comment #5)

(In reply to Bob Owen (:bobowen) from comment #4)

Created attachment 9234286 [details] [diff] [review]

...

I first tried adding removing the listener to _removeEventListener and always calling it from didDestroy, but it seems didDestroy isn't called unless we create findbar on the PDF view and then navigate. I'm not sure why that is though.

I'm confused - unless we create the findbar and then navigate - what is the "unless" case? That is, what is the case where we add the listener, but didDestroy doesn't get called when we would ideally like to remove the listener? Is it a case where we load a PDF but don't ever open the findbar? Or something else?

The unless is opening the findbar on the PDF, before navigating.

This is the scenario when loading the PDF from the filesystem, so it will be in the file content process.
If you create a new tab, load a PDF from file, navigate to example.org - breakpoint in didDestroy in PdfJsParent doesn't fire.
It does when I close the tab (it also crashes on resume, but that only happens when debugging and only with the breakpoint set - not sure if that is related).

If you create a new tab, load a PDF from file, open the findbar, navigate to example.org - breakpoint in didDestroy in PdfJsParent does fire.

In the scenario of PDF loading from http ...
If you create a new tab, load a PDF from web, navigate to example.org - breakpoint in didDestroy in PdfJsParent does fire (and browser crashes on resume).
In this case just removing the TabFindInitialized listener in _removeEventListener and always calling from didDestroy is enough to fix the findbar problem.

I added the listener to unload in PdfjsChild to send a message to remove the listener in the parent to get the PDF from filesystem case working, but it would be good if we could just rely on didDestroy.

The really weird thing is to quote from my previous comment - "This appears to work, but the really bizarre bit is, if I try and debug this, when the PDFJS:Parent:removeEventListener message arrives in receiveMessage in the debugger, both this._boundToFindbar and this._boundToTab are already null, so it doesn't appear to remove any listeners in either scenario. But I think they must be being removed otherwise things would still be broken."

Broadly, I'd expect didDestroy in the parent to be called reliably if the browsing context goes away. If it doesn't, we should talk to Nika about that, because that's bad.

nika - would you comment on the first scenario above, where didDestroy doesn't fire, on navigation, but fires when the tab is closed.
I notice that there seem to be other cases where it isn't always getting called.
Not sure if the crash is related at all.

Comment 7

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #6)

If you create a new tab, load a PDF from file, navigate to example.org - breakpoint in didDestroy in PdfJsParent doesn't fire.
It does when I close the tab (it also crashes on resume, but that only happens when debugging and only with the breakpoint set - not sure if that is related).

Maybe not related but still bad; I filed bug 1724083

Anyway, I think this is where it goes wrong, I expect the actor to be destroyed and didDestroy to be called if we process switch the tab (from file: to http (webisolated, in the fission case)).

I added the listener to unload in PdfjsChild to send a message to remove the listener in the parent to get the PDF from filesystem case working, but it would be good if we could just rely on didDestroy.

Yeah, I don't think we should need this. Adding unload listeners is bad for perf because it breaks bfcache; pagehide doesn't and would probably also work, but really we shouldn't need this workaround, I believe.

The really weird thing is to quote from my previous comment - "This appears to work, but the really bizarre bit is, if I try and debug this, when the PDFJS:Parent:removeEventListener message arrives in receiveMessage in the debugger, both this._boundToFindbar and this._boundToTab are already null, so it doesn't appear to remove any listeners in either scenario. But I think they must be being removed otherwise things would still be broken."

I would add logging to check the same thing, to see if it's a debugger thing. I don't really know exactly how it works, but I think the debugger doesn't fully pause the parent process (if it did, its own debugging code couldn't run) and so weird things can happen if other C++ code ends up triggering more JS code that runs while the debugger is "paused" on the JS code you're debugging.

Broadly, I'd expect didDestroy in the parent to be called reliably if the browsing context goes away. If it doesn't, we should talk to Nika about that, because that's bad.

nika - would you comment on the first scenario above, where didDestroy doesn't fire, on navigation, but fires when the tab is closed.
I notice that there seem to be other cases where it isn't always getting called.
Not sure if the crash is related at all.

I doubt the crash is related to the root cause of this bug, though it might be related to the JS-running-while-the-debugger-is-paused thing. Nika's still the best person to comment on the use of didDestroy, why it isn't firing, and/or alternatives we should be using, though. :-)

Comment 8

[:Gijs (he/him)]

Comment on attachment 9234286 [details] [diff] [review]
Remove Pdjfs TabFindInitialized listener when navigating away from pdf.

(dropping the feedback request in lieu of the needinfo bouncing we're doing on the bug :-)

Comment 9

[Bob Owen (:bobowen)]

(In reply to :Gijs (he/him) from comment #7)

(In reply to Bob Owen (:bobowen) from comment #6)

...

The really weird thing is to quote from my previous comment - "This appears to work, but the really bizarre bit is, if I try and debug this, when the PDFJS:Parent:removeEventListener message arrives in receiveMessage in the debugger, both this._boundToFindbar and this._boundToTab are already null, so it doesn't appear to remove any listeners in either scenario. But I think they must be being removed otherwise things would still be broken."

I would add logging to check the same thing, to see if it's a debugger thing. I don't really know exactly how it works, but I think the debugger doesn't fully pause the parent process (if it did, its own debugging code couldn't run) and so weird things can happen if other C++ code ends up triggering more JS code that runs while the debugger is "paused" on the JS code you're debugging.

Good call, when I added logging, _removeEventListener was being called twice, once from receiveMessage (from the child) and once from didDestroy???
The debugger was only catching the one in receiveMessage and the other had already happened.

So, I commented out the send of PDFJS:Parent:removeEventListener in PdfjsChild (the unload listener was still there) and sure enough didDestroy was still called and the findbar worked after the navigation.

However I then removed the unload listener in PdfjsChild and didDestroy stopped being called in PdfjsParent.
(Which in a weird way I was quite happy about because I could have sworn it wasn't working originally and indeed it wasn't.)
So, somehow having the unload listener in the child causes didDestroy to be called in the parent - does that give you any more of a clue?

Comment 10

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #9)

So, I commented out the send of PDFJS:Parent:removeEventListener in PdfjsChild (the unload listener was still there) and sure enough didDestroy was still called and the findbar worked after the navigation.

However I then removed the unload listener in PdfjsChild and didDestroy stopped being called in PdfjsParent.
(Which in a weird way I was quite happy about because I could have sworn it wasn't working originally and indeed it wasn't.)
So, somehow having the unload listener in the child causes didDestroy to be called in the parent - does that give you any more of a clue?

My immediate thought is that the presence/absence of the unload handler will affect whether the page gets bfcached, which then maybe impacts whether the actor goes away or not? But that is once more something where I'd have to ask nika...

Comment 11

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bob Owen (:bobowen) from comment #9)

However I then removed the unload listener in PdfjsChild and didDestroy stopped being called in PdfjsParent.
(Which in a weird way I was quite happy about because I could have sworn it wasn't working originally and indeed it wasn't.)
So, somehow having the unload listener in the child causes didDestroy to be called in the parent - does that give you any more of a clue?

As :Gijs suspected, this is because of the page now being able to enter the BFCache, so we keep it around after navigating away because the unload listener is gone. We're planning to eventually cache pages which have unload handlers by not running them if we put the document in the BFCache, as that is also chrome's plan, but haven't made a change like that yet.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

I think it might make sense for us to add new callbacks to JSWindowActors to track when the document enters/leaves the BFCache in addition to the normal lifecycle. Perhaps that would be useful here?

Comment 13

[Bob Owen (:bobowen)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #12)

I think it might make sense for us to add new callbacks to JSWindowActors to track when the document enters/leaves the BFCache in addition to the normal lifecycle. Perhaps that would be useful here?

It seems it would be good to have something that always got called when navigating away from the doc, so either actor destroyed or entering the BFCache (and maybe other things).

That way you would always have something reliable for this type of clean-up, without having to know all those conditions.
Then for final clean-up you can use didDestroy.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Yeah, you'll also need a callback for when the actor is shown again after being restored from the bfcache, so I might add callbacks like pagehide and pageshow.

Comment 15

[Bob Owen (:bobowen)]

De-assigning myself as I don't think I'm in a position to work on the required functionality.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Nika, do you have any suggestions on who might be able to work on this bug? This seems like a pretty unfortunate regression to be carrying over from release to release.

Comment 17

[Chris Peterson [:cpeterson]]

The Find in Page code doesn't handle when the page is moved to a different BrowsingContext or to bfcache.

We should fix this for ESR 91.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1722880 - Part 1: Add IsInBFCache to WindowContext, and make it non-SHIP compatible, r=smaug

This field will be useful to JS code such as JSWindowActors which need to be
able to detect when their WindowContext is no longer active.

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1722880 - Part 2: Expose windowContext getter to JSWindowActors, r=smaug

This will be used by the Pdfjs actor to get isInBFCache

Depends on D124098

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1722880 - Part 3: Support BFCache, unloading, and tab adoption in Pdfjs actors, r=gijs

Depends on D124099

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c492a8d097f
Part 1: Add IsInBFCache to WindowContext, and make it non-SHIP compatible, r=smaug,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/f3c225ca3f3f
Part 2: Expose windowContext getter to JSWindowActors, r=smaug
https://hg.mozilla.org/integration/autoland/rev/045ccf5745b3
Part 3: Support BFCache, unloading, and tab adoption in Pdfjs actors, r=Gijs

Comment 22

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/4c492a8d097f
https://hg.mozilla.org/mozilla-central/rev/f3c225ca3f3f
https://hg.mozilla.org/mozilla-central/rev/045ccf5745b3

Comment 23

[Alice0775 White]

[Tracking Requested - why for this release]: Find in page does not work on web pages after viewing a pdf.
Request tracking according to comment #17.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR91 approval when you get a chance. It grafts cleanly as-landed.

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9238750 [details]
Bug 1722880 - Part 1: Add IsInBFCache to WindowContext, and make it non-SHIP compatible, r=smaug

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Core feature broken after viewing a pdf
• User impact if declined: Find in page does not work on web pages after viewing a pdf.
• Fix Landed on Version: 93
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Patches are not super small, but are fairly well-defined, and have been tested in 93.
• String or UUID changes made by this patch: None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9238750 [details]
Bug 1722880 - Part 1: Add IsInBFCache to WindowContext, and make it non-SHIP compatible, r=smaug

Not the smallest set of patches, but seems like a pretty bug to fix on ESR. Approved for 91.2esr.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/9dad15ed607e
https://hg.mozilla.org/releases/mozilla-esr91/rev/79fdbb5253c7
https://hg.mozilla.org/releases/mozilla-esr91/rev/c622b3d0a30a

Comment 28

[Daniel Bodea [:danibodea]]

This issue was reproduced in Firefox Release v92.0 on Windows 10 and Ubuntu 20 and verified on Nightly v94.0a1, Beta v93.0b8 and ESR v 91.2.0esr (20210927140538) from Threeherder on Windows 10 and Ubuntu 20.