1752149 - Changes in PDF forms are not saved when overwriting an existing saved file from a loaded file:/// pdf.

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect)

Description

[Frank Tank]

User Agent: Mozilla/5.0 (Windows NT 10.0; rv:96.0) Gecko/20100101 Firefox/96.0

Steps to reproduce:

open pdf file in FF that contains form fields
enter content to form fields
safe pdf file

Actual results:

pdf is not saving added content in form fields if the pdf file does already exist
example:
save pdf first time = content in forms will be saved
save pdf again and FF promts to overwrite existing file = content will not be safed!
(if saving with a new filname = content will be saved)

Expected results:

added forms content should be saved when overwriting an existing pdf file

Comment 1

[Marco Castelluccio [:marco]]

Is this happening with a specific PDF file?

Comment 2

[Frank Tank]

No I think its happening with all files. But I've only checked two different pdf files with forms. Both had the same isssue.

Comment 3

[Marco Castelluccio [:marco]]

Could you attach them here? In order to reproduce the bug, we'll need access to them.

Comment 4

[Frank Tank]

Attachment: lastschriftmandat steuer 142_103-03.pdf

Comment 5

[Frank Tank]

Attachment: pdf 01.pdf

Comment 6

[Marco Castelluccio [:marco]]

It works for me on Linux, and Calixte couldn't reproduce on Windows either.

Maybe you could take a screencast to show us exactly what steps you are taking to reproduce this?

Comment 7

[Calixte Denizet (:calixte)]

I can't reproduce with both pdfs on windows 11 and windows 10 with nightly (98) and release (96).

Comment 8

[Frank Tank]

Attachment: pdf 02.pdf

Comment 9

[Calixte Denizet (:calixte)]

I don't have any idea on what could be wrong on the pdf.js side...
:gijs, maybe you'll have an idea ?

Comment 10

[:Gijs (out until Jan 5th; he/him)]

(In reply to Calixte Denizet (:calixte) from comment #9)

I don't have any idea on what could be wrong on the pdf.js side...

Well, the PDFjs code has this fallback when saving: https://searchfox.org/mozilla-central/rev/2e0ceee0158293a37466aaf9495dbeffef744b7c/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#345

where it'll save using the original URL, instead of the blob URL which (I assume) includes form filled data. That could potentially explain this, but I don't know why there would be a problem getting the original URL for the reporter but not when we try to reproduce... It also doesn't make sense that this would only happen when replacing the existing file, because the selection of the destination file should happen after that code...

My other guess would be that the webbrowserpersist code is doing something strange (like not completely overwriting the original file) but it seems unlikely that that would lead to a valid PDF with different annotation data?

:gijs, maybe you'll have an idea ?

Unfortunately my guesses are pretty wild, given other people can't reproduce. Reporter, is it possible for you to share a screencast of the exact steps you're taking?

One other thing that might be useful to know is, if you save a copy of the PDF with filled in form data "A", and then save another copy with filled-in form data "B" on top of it, does the resulting saved file have data "A", data "B", or no data in it at all?

Comment 11

[Frank Tank]

One other thing that might be useful to know is, if you save a copy of the PDF with filled in form data "A", and then save another copy with filled-in form data "B" on top of it, does the resulting saved file have data "A", data "B", or no data in it at all?

If i get you right - if opening the file, enter A in a field, save as xxx02.pdf, then enter B and saving on top of the copy it saves data B

Comment 12

[Frank Tank]

I have tried to reproduce with a new profile. The behaviour is different now. It saves the file automatically without a promt as a new file with (1) at the end of the name.
At the moment i cant share screenshots - lack of time :-(

Comment 13

[Brendan Dahl [:bdahl]]

Since we're not able to reproduce, please provide screenshots when you have time.

Comment 14

[Frank Tank]

Attachment: 2022-02-07 19_19_50-pdf 01.pdf- open pdf .png

open pdf file

Comment 15

[Frank Tank]

Attachment: 2022-02-07 19_18_51-pdf 01.pdf - fill form with 100.png

fill form with "100"

Comment 16

[Frank Tank]

Attachment: 2022-02-07 19_17_39-Öffnen von pdf 01.pdf - click safe button.png

click safe button and overwrite existing

Comment 17

[Frank Tank]

Attachment: 2022-02-07 19_17_04-pdf 01.pdf - reopen pdf.png

re-open saved file -> empty

Comment 18

[Marco Castelluccio [:marco]]

A screencast might be more useful than a series of screenshots, as it will have more information.

Something else that could help:

1. Save the PDF file, take note of the last modified time from the file properties
2. Wait a few minutes
3. Enter text in the field, overwrite the previous file
4. Check the last modified time from the file properties to make sure the file was actually overwritten

Comment 19

[Frank Tank]

Attachment: screencast.mp4

Comment 20

[Marco Castelluccio [:marco]]

Unfortunately the screencast is partial. Maybe you could try using https://obsproject.com/.

Comment 21

[Frank Tank]

(In reply to Marco Castelluccio [:marco] from comment #18)

A screencast might be more useful than a series of screenshots, as it will have more information.

Something else that could help:

1. Save the PDF file, take note of the last modified time from the file properties
2. Wait a few minutes
3. Enter text in the field, overwrite the previous file
4. Check the last modified time from the file properties to make sure the file was actually overwritten

1. time is not modified and i think not overwritten
2. time is not modified and i think not overwritten

Comment 22

[Frank Tank]

second line should not be 2. but 4.

Comment 23

[Frank Tank]

Marco, can you download it to your disk? For me this works. I think the build in player has a problem with it.

Comment 24

[Marco Castelluccio [:marco]]

(In reply to Frank Tank from comment #23)

Marco, can you download it to your disk? For me this works. I think the build in player has a problem with it.

I couldn't play it with VLC either, but MPV is able to play it fully.

(In reply to Frank Tank from comment #21)

(In reply to Marco Castelluccio [:marco] from comment #18)

Something else that could help:

1. Save the PDF file, take note of the last modified time from the file properties
2. Wait a few minutes
3. Enter text in the field, overwrite the previous file
4. Check the last modified time from the file properties to make sure the file was actually overwritten

1. time is not modified and i think not overwritten
2. time is not modified and i think not overwritten

This means the file is not being actually written. Maybe it is write-protected or something like that? But then I'd expect Firefox to show an error message when it fails to save the file.

Comment 25

[:Gijs (out until Jan 5th; he/him)]

(In reply to Marco Castelluccio [:marco] from comment #24)

This means the file is not being actually written. Maybe it is write-protected or something like that? But then I'd expect Firefox to show an error message when it fails to save the file.

Based on the screencast, I think something else is going wrong, already before we get to this step: if you put data in a form field and press the save button, I expect a "save as" dialog, not a "what do you want to do with this PDF" dialog -- because we should be saving the blob URL with the PDF contents as well as the form input contents, and we cannot directly pass such a blob URL to an external application without first saving it to disk.

The fact that we're instead seeing a "what do you want to do with this file" dialog points to the code at https://searchfox.org/mozilla-central/rev/2a0b0ababd4541ecffb74cbe0820a9d0a25da636/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#343-400 making the "wrong" decision. In that case, it also makes sense that we aren't successful in overwriting anything, because the source URL (the file:///blah/blah/blah.pdf on the desktop) and the target URL (again file:///blah/blah/blah.pdf) are the same. So I think this is a PDF.js issue, and the PDF.js save code isn't successfully getting a blob URL even though there is data in the form. Marco, does that look correct to you?

Comment 26

[:Gijs (out until Jan 5th; he/him)]

Here are detailed STR that allow me to reproduce on Windows:

1. new firefox profile
2. disable browser.download.improvements_to_download_panel in about:config (to match release; this is also hiding the bug on nightly/beta98)
3. save the PDF 1 example locally to disk
4. open the local copy of the PDF in Firefox
5. put some form info in
6. click "save"/"download" button in the PDF.js UI

ER:
save as dialog

AR:
"what do you want to do with this file"

I verified with the browser debugger that this is happening because data.sourceEventType that we receive in the download method in the stream converter is "download" instead of "save".

Moving back to PDF.js for further triage. :-)

FWIW, I think we should consider just defaulting to the "save" behaviour, irrespective of the downloads improvement pref. This would also address bug 1752739, bug 1726902, and bug 1751061.

Comment 27

[Marco Castelluccio [:marco]]

Ah-ha! Thanks for the investigation. I'll mark this as S2 for now because it's bad to lose user data.

Comment 28

[Marco Castelluccio [:marco]]

Attachment: Bug 1752149 - Always launch the save dialog when pressing the PDF Viewer download button. r=#pdfjs-reviewers

Independently from the value of the browser.download.improvements_to_download_panel pref.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3978f169f641
Always launch the save dialog when pressing the PDF Viewer download button. r=pdfjs-reviewers,Gijs,bdahl

Comment 30

[Cristian Tuns]

Backed out for causing mochitest failures browser_download_open_with_internal_handler.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/mochitest/browser_download_open_with_internal_handler.js | Test timed out

Comment 31

[Marco Castelluccio [:marco]]

Given that bug 1733587 is riding the trains, we can call this fixed.

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

FWIW, I think we should consider just defaulting to the "save" behaviour, irrespective of the downloads improvement pref. This would also address bug 1752739, bug 1726902, and bug 1751061.

Can we call these fixed too?

I'll file a new bug for the follow-up cleanup, I likely won't have time to fix the test failure.

Comment 32

[:Gijs (out until Jan 5th; he/him)]

(In reply to Marco Castelluccio [:marco] from comment #31)

Given that bug 1733587 is riding the trains, we can call this fixed.

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

FWIW, I think we should consider just defaulting to the "save" behaviour, irrespective of the downloads improvement pref. This would also address bug 1752739, bug 1726902, and bug 1751061.

Can we call these fixed too?

I'll file a new bug for the follow-up cleanup, I likely won't have time to fix the test failure.

So at this point I think we have 3 bugs around the same set of code - this bug, bug 1757771 and bug 1751061. Once that trifecta of bugs is fixed (ie we actually remove and stop testing the old / other cases, and the weird edgecase documented in 1751061), we can do a tiny bit more work in bug 1752739 or bug 1726902 to fix those bugs - but they won't be fixed automatically.

I think we should make a decision about where we're fixing 1757771 and this and bug 1751061 - it can happen in any of these 3 bugs but we should pick one and mark the others as deps or dupes.

Comment 33

[Marco Castelluccio [:marco]]

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

(In reply to Marco Castelluccio [:marco] from comment #31)

Given that bug 1733587 is riding the trains, we can call this fixed.

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

FWIW, I think we should consider just defaulting to the "save" behaviour, irrespective of the downloads improvement pref. This would also address bug 1752739, bug 1726902, and bug 1751061.

Can we call these fixed too?

I'll file a new bug for the follow-up cleanup, I likely won't have time to fix the test failure.

So at this point I think we have 3 bugs around the same set of code - this bug, bug 1757771 and bug 1751061. Once that trifecta of bugs is fixed (ie we actually remove and stop testing the old / other cases, and the weird edgecase documented in 1751061), we can do a tiny bit more work in bug 1752739 or bug 1726902 to fix those bugs - but they won't be fixed automatically.

I think we should make a decision about where we're fixing 1757771 and this and bug 1751061 - it can happen in any of these 3 bugs but we should pick one and mark the others as deps or dupes.

Isn't this effectively already fixed with the download panel improvements pref enabled?

Comment 34

[Marco Castelluccio [:marco]]

Are the other bugs also fixed with the download panel improvements pref enabled?

Comment 35

[:Gijs (out until Jan 5th; he/him)]

(In reply to Marco Castelluccio [:marco] from comment #33)

I think we should make a decision about where we're fixing 1757771 and this and bug 1751061 - it can happen in any of these 3 bugs but we should pick one and mark the others as deps or dupes.

Isn't this effectively already fixed with the download panel improvements pref enabled?

No. Apart from the code removal for the non-dl-panel-improvements case, once you do remove the code there are failing tests, so it's not fixed. More of the details of the test failure that remains are in 1751061.

(In reply to Marco Castelluccio [:marco] from comment #34)

Are the other bugs also fixed with the download panel improvements pref enabled?

No. I tried to clarify that already:

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

Once that trifecta of bugs is fixed (ie we actually remove and stop testing the old / other cases, and the weird edgecase documented in 1751061), we can do a tiny bit more work in bug 1752739 or bug 1726902 to fix those bugs - but they won't be fixed automatically.