Issue 2786: JSON parsing error while running test security

[jumde]

fix #2786
fix #2834

On linux, with shell set to true spawnSync timeout fails to kill the running browser window causing JSON parsing error since network_log is being written.

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Windows
  • macOS
  • Linux
• Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
  • Windows
  • macOS
  • Linux
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Requested a security/privacy review as needed.

Test Plan:

Verify npm run test-security runs without parsing errors.

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions.

[diracdeltas]

shell false is better for security anyway; can we set it to false on all platforms except mac? (regardless of whether network_log is true)

[jumde]

This also can be within a try | catch block, but haven't seen it fail till now.

[diracdeltas]

This doesn't seem to guarantee the JSON file is completely written by the network log process, just that it is parseable. What if instead it tried to re-parse the file some number of times with a time delay to wait until the JSON file is completely rewritten?

[jumde]

I tried reparsing the file 3 times after a wait of 10 secs on Win10, but I always got the file with terminating string },. I'll try increasing the timeout to check if it may be a result of my VM being slow.

[diracdeltas]

@jumde i see, so it seems the file is indeed complete but just that it has a weird terminating string on windows?

[diracdeltas]

in that case i suggest adding a comment in the code to say that this is a windows chromium issue

[diracdeltas]

if i understand this bug correctly, i think this code would be more clear as:

let jsonContent = fs.readFileSync(networkLogFile, 'utf8')
if (jsonContent.endsWith('},')) {
   // do the fixing
}
jsonOutput = JSON.parse(jsonContent)

instead of a try-catch, since the JSON parsing might potentially fail for other reasons

[diracdeltas]

adding @mihaiplesa as a reviewer to see whether this fixes the security test running on linux/win build machines

[jumde]

Deleting the files before starting the audit helps avoid stale results if the new file is not created.

[diracdeltas]

lgtm

[mihaiplesa]

Looking good, it's passing for Linux and Windows now.

Have to find a way to execute browser tests on Windows as they fail when run in Jenkins (which uses powershell in non-interactive mode).

[jumde]

Test Results:

1. MacOS:

• npm start
• npm test brave_unit_tests && npm test brave_browser_tests
• npm run test-security

2. Linux:

• npm start
• npm test brave_unit_tests && npm test brave_browser_tests
• npm run test-security

3. Windows:

• npm start
• npm test brave_unit_tests && npm test brave_browser_tests
• npm run test-security

On Windows:

1. Terminating npm start fails with this backtrace - this happens on master as well

Backtrace:
  CrashOnProcessDetach [0x00007FFD739E1120+0] (c:\b\src\base\win\dllmain.cc:89)
        DllMain [0x00007FFD739E1056+54] (c:\b\src\base\win\dllmain.cc:98)
        dllmain_dispatch [0x00007FFD7C4D2A18+152] (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\dll_dllmain.cpp:200)
        _DllMainCRTStartup [0x00007FFD7C4D2AF1+49] (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\dll_dllmain.cpp:254)
        RtlAnsiStringToUnicodeString [0x00007FFDB95E8F07+663]
        LdrShutdownProcess [0x00007FFDB95E35BC+300]
        RtlExitUserProcess [0x00007FFDB9606DAD+173]
        CtrlRoutine [0x00007FFDB57927C0+288]
        CtrlRoutine [0x00007FFDB5792763+195]
        BaseThreadInitThunk [0x00007FFDB86B7E94+20]
        RtlUserThreadStart [0x00007FFDB960A251+33]

2. brave-unit-tests are failing with this error on master as well

CXX obj/brave/test/brave_unit_tests/brave_common_static_redirect_network_delegate_helper_unittest.obj
FAILED: obj/brave/test/brave_unit_tests/brave_common_static_redirect_network_delegate_helper_unittest.obj

[mihaiplesa]

Let's get this in.

[diracdeltas]

minor: this should be true : instead of true:

i assume this doesn't work on windows without this change?

[jumde]

Updated! and yes, this is needed for windows.

[mihaiplesa]

looking good

[mihaiplesa]

We might need to uplift this to all other channel branches as it will be part of the build process.

[diracdeltas]

i recommending opening an uplift PR for this in dev channel (see https://github.com/brave/brave-browser/wiki/Triage-Guidelines for instructions), make sure it doesn't cause any build issues, and then opening uplift requests for the other channels