Closed Bug 1722880 Opened 4 months ago Closed 3 months ago

Find in Page broken after opening a PDF

Categories

(Firefox :: PDF Viewer, defect, P2)

Firefox 90
Desktop
All
defect

Tracking

()

VERIFIED FIXED
93 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 93+ verified
firefox90 --- wontfix
firefox91 --- wontfix
firefox92 --- wontfix
firefox93 --- verified
firefox94 --- verified

People

(Reporter: mathis, Assigned: nika)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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

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.

Component: Untriaged → PDF Viewer

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

Status: UNCONFIRMED → NEW
Component: PDF Viewer → Find Backend
Ever confirmed: true
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → Desktop
Summary: Search broken after opening a PDF → Find in Page broken after opening a PDF
Component: Find Backend → Security: Process Sandboxing
Assignee: nobody → bobowencode

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?

Attachment #9234286 - Flags: feedback?(gijskruitbosch+bugs)

(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.

Flags: needinfo?(bobowencode)

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.

Component: Security: Process Sandboxing → PDF Viewer
Flags: needinfo?(nika)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bobowencode)
Product: Core → Firefox

(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. :-)

Flags: needinfo?(gijskruitbosch+bugs)
See Also: → 1724083

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 :-)

Attachment #9234286 - Flags: feedback?(gijskruitbosch+bugs)

(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?

Flags: needinfo?(gijskruitbosch+bugs)

(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...

Flags: needinfo?(gijskruitbosch+bugs)

(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.

Flags: needinfo?(nika)

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?

(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.

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.

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

Assignee: bobowencode → nobody

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.

Flags: needinfo?(nika)
Fission Milestone: --- → ?

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.

Assignee: nobody → nika
Severity: -- → S3
Fission Milestone: ? → ---
Priority: -- → P2
See Also: 1724083

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.

This will be used by the Pdfjs actor to get isInBFCache

Depends on D124098

Flags: needinfo?(nika)
Pushed by nlayzell@mozilla.com:
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
Attachment #9234286 - Attachment is obsolete: true
Flags: in-testsuite+

[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.

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

Flags: needinfo?(nika)

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
Flags: needinfo?(nika)
Attachment #9238750 - Flags: approval-mozilla-esr91?
Attachment #9238752 - Flags: approval-mozilla-esr91?
Attachment #9238753 - Flags: approval-mozilla-esr91?

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.

Attachment #9238750 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Attachment #9238752 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Attachment #9238753 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.