feat: HTTP preconnect feature for electronjs

[coderReview]

Description of Change

Added HTTP preconnect hint support.

Using the parameter numSocketsToPreconnect in BrowserWindow you can control how many sockets will be open.

Related to issue #16476.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• relevant documentation is changed or added
• PR title follows semantic commit guidelines
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added HTTP preconnect hint support

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[MarshallOfSound]

cc @deepak1556 @zcbenz for the network related changes, may conflict with ongoing network service refactor

[coderReview]

@MarshallOfSound can you explain how would that conflict? Is this feature being worked on?

[MarshallOfSound]

@coderReview I don't think anyone else is working on preconnect specifically but their is some pretty heft refactoring going on to enable the Network Service inside Electron.

Refs: #17431 for part 1

Just want to run it by @deepak1556 and @zcbenz that these two pieces of work won't conflict when one is merged. Depending on what they think it might make sense for this preconnect work to be rebased on top of their network service work.

I'll leave that up to them as I'm not clear on the network service work being done

[coderReview]

hi @MarshallOfSound made some new commits with a working version. I will provide also another repo with test cases ASAP.

[coderReview]

hello @MarshallOfSound we have created some tests cases for the changes made. Please check here https://github.com/krunt/electron-preconnect-test-cases.

Also, can you please help to pass all checks? They seem to pass but fail at the end (doing cleanup?)

Thank you

[coderReview]

hello @MarshallOfSound, @deepak1556 and @zcbenz. I saw that #17431 was merged. @krunt can you check if after the Merge the code is still working as expected for pre-connect? thanks

[coderReview]

@krunt also please fix the merge conflicts.

[jkleinsc]

Why are we pulling in chromium profile/profile manager? Couldn't this be done without pulling this in?

[coderReview]

@krunt can you answer the question above? Can we remove those files? thanks

[krunt]

profile manager is not needed. I will remove it.
profile class declaration is needed in several places in chrome/browser/*
I tried to minimize related changes of chrome/browser/* and their interfaces.
actually I am not creating Profile class instances, just cast content::BrowserContext to Profile
(as done in Profile::FromBrowserContext()) and pass to methods where profile is needed (checking for sure that is will be safe)

[coderReview]

@jkleinsc kindly check the code again. Thank you

[lukeapage]

@zcbenz can we get some input on if and how this conflicts with your network refactoring?
Considerable time and effort has gone into this and I don’t want this Pr to just get lost. Please let us know if there is anything we can do to make reviewing and answering the questions on conflicts easier.

[zcbenz]

@zcbenz can we get some input on if and how this conflicts with your network refactoring?

Since the preconnect feature is mostly implemented in Chromium, it won't have conflicts with NetworkService refactoring.

[coderReview]

@lukeapage I don't know exactly why the builds are failing. We will investigate further. Any help would be appreciated.

@krunt can you confirm that npm test works fine? Please post results here. Also, please resolve the merge conflicts. Thank you

@zcbenz thank you for the input.

[coderReview]

@lukeapage is there any way to restart the checks? I think there was some timeout issues that prevented them from running correctly. Thank you

[coderReview]

Merged with master and submitted again. Thanks

[coderReview]

@lukeapage @MarshallOfSound could you please indicate if we are still missing something in the PR?Thank you.

[coderReview]

We have only 2 failing checks:

Mac check failure:

# tests 1255
# pass 1255
# fail 0
[949:0430/093248.531507:WARNING:discardable_shared_memory_manager.cc(410)] Some MojoDiscardableSharedMemoryManagerImpls are still alive. They will be leaked.
...
Runner Failed: Main process specs
Error: Electron tests failed with code null.
    at Array.runMainProcessElectronTests (/Users/distiller/project/src/electron/script/spec-runner.js:136:11)
    at runElectronTests (/Users/distiller/project/src/electron/script/spec-runner.js:96:22)
    at main (/Users/distiller/project/src/electron/script/spec-runner.js:55:9)
    at process._tickCallback (internal/process/next_tick.js:68:7)
An error occurred inside the spec runner: Error: Electron test runners have failed
    at runElectronTests (/Users/distiller/project/src/electron/script/spec-runner.js:107:11)
    at process._tickCallback (internal/process/next_tick.js:68:7)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! electron@6.0.0-nightly.20190404 test: `node ./script/spec-runner.js "--ci" "--enable-logging"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the electron@6.0.0-nightly.20190404 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/distiller/.npm/_logs/2019-04-30T16_32_48_582Z-debug.log

And ia32 failure:

[00:40:25] # tests 1228
[00:40:25] # pass 1228
[00:40:25] # fail 0
[00:40:25] [3492:0430/121902.352:WARNING:discardable_shared_memory_manager.cc(410)] Some MojoDiscardableSharedMemoryManagerImpls are still alive. They will be leaked.
...
[00:40:25] Runner Failed: Main process specs
[00:40:25] Error: Electron tests failed with code 1.
[00:40:25]     at Array.runMainProcessElectronTests (C:\projects\src\electron\script\spec-runner.js:136:11)
[00:40:25]     at runElectronTests (C:\projects\src\electron\script\spec-runner.js:96:22)
[00:40:25]     at main (C:\projects\src\electron\script\spec-runner.js:55:9)
[00:40:25]     at process._tickCallback (internal/process/next_tick.js:68:7)
[00:40:25] An error occurred inside the spec runner: Error: Electron test runners have failed
[00:40:25]     at runElectronTests (C:\projects\src\electron\script\spec-runner.js:107:11)
[00:40:25]     at process._tickCallback (internal/process/next_tick.js:68:7)
[00:40:25] npm ERR! code ELIFECYCLE
[00:40:25] npm ERR! errno 1
[00:40:25] npm ERR! electron@6.0.0-nightly.20190404 test: `node ./script/spec-runner.js "--ci" "--enable-logging"`
[00:40:25] npm ERR! Exit status 1
[00:40:25] npm ERR! 
[00:40:25] npm ERR! Failed at the electron@6.0.0-nightly.20190404 test script.
[00:40:25] npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
[00:40:26] 
[00:40:26] npm ERR! A complete log of this run can be found in:
[00:40:26] npm ERR!     C:\Users\electron\AppData\Roaming\npm-cache\_logs\2019-04-30T16_19_02_774Z-debug.log
[00:40:26] Command exited with code 1

Is this some known issue of the test environment?

Thank you.

[MarshallOfSound]

The windows failure is known, the macos failure is a crash though which I haven't seen before

[coderReview]

Hi @MarshallOfSound thank you for your response. They seem to behave more or less the same. Would this be an issue to accepting the PR?

[MarshallOfSound]

@coderReview The macOS failure would require a bit of investigation as to whether this PR caused it. The failure is this. I'll rerun those tests

ok 39 app module getPath(name) returns the overridden path
0   Electron Framework                  0x000000011160a719 base::debug::CollectStackTrace(void**, unsigned long) + 9
1   Electron Framework                  0x00000001114c8b83 base::debug::StackTrace::StackTrace() + 19
2   Electron Framework                  0x000000011160a5b1 base::debug::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, void*) + 2385
3   libsystem_platform.dylib            0x00007fff596d3f5a _sigtramp + 26
4   CoreFoundation                      0x00007fff31a9633e -[NSInvocation dealloc] + 78
5   libdispatch.dylib                   0x00007fff59418d50 _dispatch_client_callout + 8
6   libdispatch.dylib                   0x00007fff5942fb90 _dispatch_queue_concurrent_drain + 890
7   libdispatch.dylib                   0x00007fff59420104 _dispatch_queue_invoke + 380
8   libdispatch.dylib                   0x00007fff59425ac4 _dispatch_queue_override_invoke + 467
9   libdispatch.dylib                   0x00007fff5941a941 _dispatch_root_queue_drain + 515
10  libdispatch.dylib                   0x00007fff5941a6ed _dispatch_worker_thread3 + 101
11  libsystem_pthread.dylib             0x00007fff596dd1ca _pthread_wqthread + 1387
12  libsystem_pthread.dylib             0x00007fff596dcc4d start_wqthread + 13
[end of stack trace]

[coderReview]

@MarshallOfSound I think that is expected.

The issues seems to be this:

Error: No valid signing identity available to run autoUpdater specs

[coderReview]

Hello, I wonder if there is something blocking this PR as we didn't have any feedback. Thank you very much.

[nornagon]

Hi, sorry I'm just getting around to reviewing this PR, but I don't think we can accept a 900-line patch without a plan for upstreaming. Maintaining that size of a patch is more maintenance burden than this feature is worth.

I'd recommend that you take a shot at upstreaming changes to Chromium such that we can reuse this code without a large patch. See here for info about contributing a change to Chromium.

[coderReview]

@nornagon We need to use only part of the chromium code, so most of the patch is to remove code. Pushing that to chromium src code won't be accepted as we are removing many functionalities that are specific to browsers and not applicable to Electron (one example is profile support). Is it acceptable that we add code to chromium_src/chrome/browser folder? This way it would make it easier to maintain. Thank you.

[nornagon]

Copying the code into chromium_src doesn't necessarily make it easier to maintain, because that code will not automatically inherit changes from upstream Chromium. Generally we only copy code to chromium_src that changes very rarely (1-2 times a year max), or which needs such substantial changes that it might as well be a brand new file.

I don't mean that you should upstream the patch as-is. See if you can refactor your patch so that it extracts the code you care about in such a way that it supports both Chrome and Electron's usages, and upstream that patch.

[coderReview]

Hi @nornagon I think we fall into your second category which needs such substantial changes that it might as well be a brand new file. We need to strip out all code that would involve building a lot of libraries that are specific to chromium browser. In the end we end up with a smaller/modified version of the original file. I think if we add that to chromium it wouldn't be used anywhere except for Electron.

Do you think chromium team will accept a code that won't be used in chromium but only in Electron?
If not can we modify the PR so we add a file to chromium_src/chrome/browser folder? I think we could check if it's a better solution.

Thank you for the support and review.

[nornagon]

It looks like there's a lot of logic in the ~10 files you're patching that would need to be copied over. If you can work out a way to only re-create logic that's entirely different, with very little or no duplication between Electron's copy and the original in Chrome, then I think that would be worthwhile.

Ultimately, what we need to consider is that:

1. Every line in every patch is a maintenance liability. Chrome's code, in general, changes very frequently and whenever upstream code changes, there's a good chance that a patch will become conflicting and need to be rebased on top of the change. Sometimes that's trivial to do—when the name of a parameter changes, for instance—but often it's complicated, such as when upstream APIs are refactored entirely, which happens regularly. The more patches we have and the more extensive those patches are, the more work we need to do every time Chrome changes.
2. Interfaces in //chrome change frequently and without consideration for API stability. APIs in //content are somewhat more stable and are generally designed with embedders such as Electron or Opera in mind. Code in //chrome is not intended to be used by embedders and as such, will change whenever it makes sense for Chrome to do so and without warning. When we copy code out of //chrome into chromium_src, in general that code has very tight binding to code in //chrome, and so that code is brittle, and liable to break when Chrome changes.

I'm unfortunately not well-enough equipped with knowledge about this particular feature to offer guidance on the best way to approach a refactor in upstream, but Electron's upgrades team can't accept features that will add substantially to the maintenance burden. We're only a few people compared with the 1000+ engineers that are changing Chromium and causing conflicts for us, and we need to pick our battles carefully.

[coderReview]

hello @nornagon @MarshallOfSound we have created a new PR with a different approach without the Chromium patch. We decided to create a different PR because of some conflicts. Please check it here #18671