1781213 - Nothing happens when importing a file on the “about:logins” page

Status: VERIFIED FIXED

(Firefox :: about:logins, defect, P2)

Description

[Simona Rosu [:srosu], Ecosystem QA]

Attachment: ImportFromFile_Broken.gif

[Affected versions]:

• Firefox Nightly 105.0a1 (Build ID: 20220725213209)
• Firefox Beta 104.0b1 (Build ID: 20220725154459)

[Affected Platforms]:

• macOS 12.3.1
• Windows 10 x64
• Ubuntu 20.04 x64.

[Prerequisites]:

• Have a new Firefox profile.
• Have a CSV file that contains multiple logins.
• Have the “signon.management.page.fileImport.enabled” pref set to true.

[Steps to reproduce]:

1. Open the Firefox browser with the profile from prerequisites.
2. Navigate to the "about:logins" page.
3. Click the Menu button and select the “Import from a File…” option.
4. Select the CSV file from prerequisites and click on the “Open” button.
5. Observe what happens next.

[Expected result]:

• The logins from the CSV file are imported and the “Import” modal is displayed.

[Actual result]:

• The logins are not imported and the “Import” modal is not displayed.

[Regression Window]:

• The issue is not reproducible with older Nightly 104.0a1 builds. Considering this using mozregression tools I have found the regression range. Here are the results:
  • Last good revision: f33bef1f7d560e494bab0599e2022a3ea53902f9
  • First bad revision: c39b51a0b211e7bd45cb78f14eda63698d60dc1b
  • Pushlog: link

@Tom could you please take a look over this regression window since I am not sure which bug has caused this issue?

[Notes]:

• Also, the “Error: TelemetryStopwatch: key "PWMGR_IMPORT_LOGINS_FROM_FILE_MS" was already initialized” error is displayed in the Browser Console.
• This issue is reproducible in all the use cases (import wrongly CSV files, import a non-CSV file)
• Attached is a screen recording of the issue and a screenshot with the Browser Console error.

Comment 1

[Simona Rosu [:srosu], Ecosystem QA]

Attachment: BrowserConsoleError.png

Comment 2

[Tom Ritter [:tjr] (OOTO until 9/2)]

I do not have that menu option in my Firefox.... How do I get it? This is likely that commit, it can be confirmed by setting security.allow_eval_in_parent_process and security.allow_eval_with_system_principal to 'true'.

Testing in debug mode should crash with some information pointing to the source of the offending script. I don't see anything obvious in the code that looks like it's using eval...

Comment 3

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Hi Tom,

In order to have the "Import from a File…." option available you need to set the "signon.management.page.fileImport.enabled" pref from "about:config" page to true. If you already have the "about:logins" page opened after changing the pref, you might need to refresh the page.

Comment 4

[Janika Neuberger [:janika]]

This line seems to be using eval. The code that follows is not being executed anymore.

Comment 5

[Tom Ritter [:tjr] (OOTO until 9/2)]

Yes, this line would cause the problem.

d3 seems to have fixed this issue 8 years ago. We updated to the latest 3.x version a couple years ago, but the "latest 3.x" version was already wildly out of date. The correct fix here would be to update d3 and track it better; perhaps using Updatebot.

Comment 6

[Janika Neuberger [:janika]]

Updating the d3 library is probably overdue, i filed a Bug for this issue.

But the latest versions are using the eval function as well, so updating d3 won't fix the problem.
In the password manager d3 is only used to parse rows, so another solution could be to write our own Parser. This way we depend on one less third party library and we don't need to worry about the use of eval.

@Tom, what do you think about this solution. How can we fix it?

Comment 7

[Tom Ritter [:tjr] (OOTO until 9/2)]

(In reply to Janika Neuberger (:jneuberger) from comment #6)

Updating the d3 library is probably overdue, i filed a Bug for this issue.

But the latest versions are using the eval function as well, so updating d3 won't fix the problem.
In the password manager d3 is only used to parse rows, so another solution could be to write our own Parser. This way we depend on one less third party library and we don't need to worry about the use of eval.

@Tom, what do you think about this solution. How can we fix it?

If we are only using d3 to parse rows, the d3.parseRows function is marked as being eval-free.

Although yes, if we are only using d3 for this one small piece of functionality, eliminating it in favor of a simple function we write ourselves - especially since it's JavaScript and not memory-unsafe code - seems like a good path forward also.

Comment 8

[Sergey Galich [:serg]]

(In reply to Tom Ritter [:tjr] from comment #7)

Although yes, if we are only using d3 for this one small piece of functionality, eliminating it in favor of a simple function we write ourselves - especially since it's JavaScript and not memory-unsafe code - seems like a good path forward also.

+1 to "lets make own CSV parser"

Comment 9

[Janika Neuberger [:janika]]

(In reply to Tom Ritter [:tjr] from comment #7)

If we are only using d3 to parse rows, the d3.parseRows function is marked as being eval-free.

Sorry, I worded this incorrectly. we are using both d3.parseRows (eval-free) and d3.parse function (uses eval).

Although yes, if we are only using d3 for this one small piece of functionality, eliminating it in favor of a simple function we write ourselves - especially since it's JavaScript and not memory-unsafe code - seems like a good path forward also.

Thank you for your opinion, I will write a CSV parser then!

Comment 10

[Janika Neuberger [:janika]]

Attachment: Bug 1781213 - CSV Parser to import login csv files to password manager r=sgalich

Comment 11

[Janika Neuberger [:janika]]

Attachment: Bug 1781213 - Tests for CSV Parser r=sgalich

Depends on D154404

Comment 12

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

Setting Regressed by field after analyzing regression range found by mozregression in comment #0.

Comment 13

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

Set release status flags based on info from the regressing bug 1772378

Comment 15

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

Set release status flags based on info from the regressing bug 1772378

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e62402d32bdd
CSV Parser to import login csv files to password manager r=sgalich

Comment 19

[Sandor Molnar[:smolnar]]

Backed out for causing browser-chrome failures in browser/base/content/test/static/browser_all_files_referenced.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/73cb94517707bbb296e25bfccd3248f1ced78d9b

Push with failures

Failure log

INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected +0
[task 2022-10-18T18:07:03.776Z] 18:07:03     INFO - Stack trace:
[task 2022-10-18T18:07:03.777Z] 18:07:03     INFO - chrome://mochikit/content/browser-test.js:test_is:1485
[task 2022-10-18T18:07:03.778Z] 18:07:03     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:1032
[task 2022-10-18T18:07:03.780Z] 18:07:03     INFO - Not taking screenshot here: see the one that was previously logged
[task 2022-10-18T18:07:03.781Z] 18:07:03     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | file only referenced from unreferenced files: chrome://global/content/third_party/d3/d3.js referenced from chrome://devtools/content/memory/index.xhtml - 
[task 2022-10-18T18:07:03.782Z] 18:07:03     INFO - Stack trace:

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ecb7e708492
CSV Parser to import login csv files to password manager r=sgalich,credential-management-reviewers,dimi

Comment 21

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/0ecb7e708492

Comment 22

[Janika Neuberger [:janika]]

I whitelisted the d3.js library in browser/base/content/test/static/browser_all_files_referenced.js because it is still referenced by devtools.

Comment 23

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

The patch landed in nightly and beta is affected.
:janikaneuberger, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox107 to wontfix.

For more information, please visit auto_nag documentation.

Comment 24

[Simona Rosu [:srosu], Ecosystem QA]

I‘ve verified this issue using the latest Firefox Nightly 108.0a1 (Build ID: 20221023100414) on Windows 10 x64, macOS 12.5.1, and Linux Mint 20.2 x64.

• The logins from a CSV file are imported, and the “Import Complete” modal is displayed after ending the “Import from a File…” flow.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cebecfa0eb2
Tests for CSV Parser r=sgalich,credential-management-reviewers

Comment 26

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/7cebecfa0eb2