New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Upgrade to Chromium 61 #10213

Merged
merged 128 commits into from Nov 24, 2017

Conversation

Projects
None yet
@alexeykuzmin
Contributor

alexeykuzmin commented Aug 7, 2017

Current state

This branch depends on libcc from electron/libchromiumcontent#335.

Compilation and linking work on all platforms.
There is a couple of failing and temporary disabled tests.

TODOs

  • Remove async menu from docs? Fix it in #11242
  • Check Release builds
  • Fix Release build on ARM https://circleci.com/gh/electron/electron/2895
  • Resolve merge conflicts
  • [Skip for now, it's broken is master] Fix --inspect-brk [deepak1556]
  • Revert "FIXME: Inscrease compilation errors limit to 99"
  • Assertion failed: (err == 0), function uv_loop_delete, file ../../vendor/node/deps/uv/src/uv-common.c, line 661. when running the "chromium feature > web workers > Worker has node integration with nodeIntegrationInWorker" test Fix it in #11243
  • FIXME: Remove "async" option from menu.popup() (4c284cc)
  • relayKeyrelay_key (14e33da)
  • Fix arm64 build
  • Change target branch to master once Chromium 59 is merged.
  • Enable OSR.
  • (taken by @alexeykuzmin) Fix linking on Mac
  • Fix lint errors
  • (taken by @tonyganch) Fix compilation errors on Linux
  • Fix compilation errors on Windows

For review

Done

a3438e7 FIXME: DesktopNotificationDelegate has been removed
9a08f42 REVIEW: Reporting: Wire ReportingDelegate into ChromeNetworkDelegate
24dbc39 FIXME: Refactor client cert private key handling. [deepak1556]
44d1fd2 FIXME: Use sk_tool_utils::copy_to() instead of .deepCopyTo().
9b29aa8 REVIEW: Move handling of DraggableRegionsChanged notification from "view" to "frame".
49d2d98 REVIEW: Remove obsolete Blink popup blocker.
1bed73a FIXME: Remove "async" option from menu.popup()
54a6d14 REVIEW: Fix 'constructor cannot be redeclared' error
4f20949 REVIEW: Move MediaDeviceIDSalt from ProfileIOData to ProfileImpl.
ae62ad4 FIXME: Remove corsEnabled option from webFrame.registerURLSchemeAsPrivileged()

Tests

Disabled

  • <webview> tag > found-in-page event > emits when a request is made Fix it in #11244
  • nativeImage module > addRepresentation() > * (3 tests) 2357b7c fix doesn't seem to work, it makes "nativeImage module > toDataURL() returns a PNG data URL" fail; /cc @alespergl Fix it in #11245
  • crashReporter module > {with,without} sandbox > should send minidump when renderer crashes Fix all crashReporter tests in #11246
  • crashReporter module > {with,without} sandbox > should send minidump when node processes crash
  • crashReporter module > {with,without} sandbox > should send minidump with updated extra parameters
  • crashReporter module > {with,without} sandbox > should not send minidump if uploadToServer is false

Failing

  • modules support > third-party module > ffi > does not crash
  • <webview> tag > loads devtools extensions registered on the parent window
  • BrowserWindow module > extensions and dev tools extensions > works when used with partitions
  • BrowserWindow module > extensions and dev tools extensions > BrowserWindow.addDevToolsExtension > when the devtools is docked> creates the extension
  • BrowserWindow module > extensions and dev tools extensions > BrowserWindow.addDevToolsExtension > when the devtools is undocked > creates the extension
  • BrowserWindow module > nativeWindowOpen option > loads native addons correctly after reload
  • crashReporter module > without sandbox > should send minidump when node processes crash
  • crashReporter module > with sandbox > should send minidump when node processes crash
  • ipc module > remote listeners > detaches listeners subscribed to destroyed renderers, and shows a warning
  • asar package > node api > child_process.fork > opens a normal js file
  • asar package > node api > child_process.fork > supports asar in the forked js
  • asar package > node api > process.env.ELECTRON_NO_ASAR > disables asar support in forked processes
  • asar package > node api > process.env.ELECTRON_NO_ASAR > disables asar support in spawned processes
  • asar package > original-fs module > is available in forked scripts
  • modules support > third-party module > runas > can be required in renderer
  • modules support > third-party module > runas > can be required in node binary
  • node feature > child_process > child_process.fork > * (9 tests)
  • node feature > child_process> child_process.spawn > supports spawning Electron as a node process via the ELECTRON_RUN_AS_NODE env var
  • node feature > net.connect> emit error when connect to a socket path without listeners
  • <webview> tag > nodeintegration attribute > loads native modules when navigation happens
@vulture

This comment has been minimized.

vulture commented Aug 10, 2017

I know this is still a WIP, so nbd if you'd rather I just wait, but I was hoping to get a jump on the latest version for testing etc. I noticed a couple issues with this branch currently:

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Aug 10, 2017

@vulture Yes, it is WIP and it's going to stay WIP for some time, probably weeks.
Right now it's in a quite messed up state, it's basically just a set of compilation fixes and it cannot be built.

I personally don't recommend to try to use it (even for testing) until the Chromium 61 is released, which is scheduled on September 5th.

// script to run while `window.location` is still "about:blank".
blink::WebDocument document = frame->GetDocument();
blink::WebElement html_element = document.DocumentElement();
if (html_element.IsNull()) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Aug 11, 2017

Contributor

@zcbenz It's Ch61 upgrade related so not very urgent.

Is this check equivalent to to that you did in 0bcc9b7?
DidCreateDocumentElement() method was removed from content::RenderViewObserver
(it is still present in content::RenderFrameObserver).

This comment has been minimized.

@zcbenz

zcbenz Aug 14, 2017

Contributor

Yeah it should be the same, Chromium is in the process of deprecating RenderViewObserver for RenderFrameObserver, so eventually we should migrate code of atom_render_view_observer.cc to use atom_render_frame_observer.cc.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Aug 14, 2017

Contributor

Cool, thanks.

@tonyganch tonyganch changed the base branch from upgrade-to-chromium-59 to master Aug 16, 2017

@@ -144,7 +144,7 @@ class PdfViewerUI::ResourceRequester
std::unique_ptr<content::ResourceHandler> handler =
base::MakeUnique<content::StreamResourceHandler>(
request.get(), stream_context->registry(), origin);
request.get(), stream_context->registry(), origin, false);

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Aug 23, 2017

Contributor

Needs a review.

@CapOM

This comment has been minimized.

CapOM commented Sep 14, 2017

Hi is there a way to try electron with chromium 61 using npm install electron@1.X.0 ?

I would like to try ES6 support, so far it fails for:

<script type="module" id="test"></script> <script type="module">import * from "test"</script>

=> "Uncaught SyntaxError: Unexpected token import"

I am using electron 1.8.0 with "webPreferences: {blinkFeatures: ModuleScripts"} passed to new BrowserWindow.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 14, 2017

Hi is there a way to try electron with chromium 61 using npm install electron@1.X.0 ?

Not currently, the upgrade is still a WIP

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Sep 22, 2017

@MarshallOfSound
Oh, hi Sam! So many thanks for helping us with the upgrade )

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 22, 2017

@alexeykuzmin Haha, fixed a few issues on Windows still a weird "cannot use __try on objects that require unwinding" error that I looked into but couldn't figure out the change that broke the code 🤔

@vulture

This comment has been minimized.

vulture commented Sep 22, 2017

@MarshallOfSound simplified version: you cant mix __try and C++ implicit object destructors basically... , like e.g. using a std::string inside a __try will trigger that error.

a simple fix is to put C++ object usage inside a separate function, then __try { callfunction(); } __except(....)

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 22, 2017

@vulture The code with the __try is deep inside chromium logic though, does that mean the only fix is to patch it out or? Never seen this error before 😆

@vulture

This comment has been minimized.

vulture commented Sep 22, 2017

Just don't mix C++ object implicit construction/destruction in the same function as the __try. Like the short example I gave above.
__try { std::string a = ""; } __except ...
change to:
dostuff() { std::string a = ""; }
__try { dostuff(); } __except ...
something like that. I have no idea what the code you're looking at looks like though.

@thomasjo thomasjo referenced this pull request Sep 28, 2017

Closed

Upgrade to Chromium 61 #10640

@alexeykuzmin alexeykuzmin requested review from electron/browserview as code owners Sep 28, 2017

@@ -180,12 +180,13 @@ void CommonWebContentsDelegate::SetOwnerWindow(NativeWindow* owner_window) {
void CommonWebContentsDelegate::SetOwnerWindow(
content::WebContents* web_contents, NativeWindow* owner_window) {
owner_window_ = owner_window ? owner_window->GetWeakPtr() : nullptr;
NativeWindowRelay* relay = new NativeWindowRelay(owner_window_);
auto relay = base::MakeUnique<NativeWindowRelay>(owner_window_);
auto relayKey = relay->key;

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 10, 2017

Contributor

According to the Style Guide the name should be relay_key.
https://google.github.io/styleguide/cppguide.html#Variable_Names

@@ -184,7 +184,6 @@ void AtomBrowserClient::OverrideWebkitPrefs(
content::RenderViewHost* host, content::WebPreferences* prefs) {
prefs->javascript_enabled = true;
prefs->web_security_enabled = true;
prefs->javascript_can_open_windows_automatically = true;

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 11, 2017

Contributor

@zcbenz
This pref was removed in https://chromium-review.googlesource.com/c/chromium/src/+/512347
Can we just remove it too?

This comment has been minimized.

@zcbenz

zcbenz Oct 16, 2017

Contributor

Yes we can just remove it.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 12, 2017

@gerhardberger I remember you helped us with the Offscreen Rendering during the upgrade to Chromium 59.
Now we are working on upgrade to Chromium 61. If you have time, could you please help us again? )
Compilation on all platforms works with Offscreen Rendering disabled, but fails with it enabled
(build flag is set to "0" in /features.gypi).

@gerhardberger

This comment has been minimized.

Member

gerhardberger commented Oct 13, 2017

@alexeykuzmin okay, I will take a look at it on the weekend!

assert.deepEqual(results, {
warningMessage: expectedMessage,
warningMessage: 'Attempting to call a function in a renderer window that has been closed or released.',

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 27, 2017

Member

I was looking at this test yesterday, although changing to something like this does actually make the tests pass, the location of the error (remote-event-handler.html:11:33) should probably still be in the message, it looked like for some reason that location lookup was failing. Not sure if we want to just deal with that after the initial 61 release though as it's hardly a blocking issue

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 30, 2017

Contributor

Not sure if we want to just deal with that after the initial 61 release though as it's hardly a blocking issue

@MarshallOfSound
The root cause of the error might be a blocking issue though =/

@LongChair

This comment has been minimized.

LongChair commented Oct 28, 2017

If i didn't mess up reading that PR, it seems electron still depends on X11 as brightray seems to require the X11 libs like here https://github.com/electron/electron/blob/upgrade-to-chromium-61/brightray/brightray.gyp#L4

Is it planned to allow electron to run on the displaymanagers supported by Chromium (ie Wayland .... ) ?

@robinwassen robinwassen referenced this pull request Oct 28, 2017

Closed

update chrome to 62 #10957

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 29, 2017

@LongChair Please keep that discussion to the issue you raised, not other peoples pull requests 😄

#10915

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Nov 24, 2017

I'm rebasing this PR to fix the conflicts introduced by the mips changes. The build state is not supposed to change.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Nov 24, 2017

I'm turning on the just-added 2xlarge resource class for the arm CI jobs, which should be able to fix the failing arm release build.

@zcbenz

zcbenz approved these changes Nov 24, 2017

@codebytere codebytere merged commit c18afc9 into master Nov 24, 2017

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@codebytere codebytere deleted the upgrade-to-chromium-61 branch Nov 24, 2017

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Nov 24, 2017

I'll create tasks to fix and enable disabled tests right after this PR is merged.

Here they are: #11242, #11243, #11244, #11245, #11246

@SatoshiKawabata

This comment has been minimized.

SatoshiKawabata commented Dec 1, 2017

@alexeykuzmin When will this be included to beta?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Dec 1, 2017

@SatoshiKawabata when the team is ready to release a beta. No dates, sorry.

@SatoshiKawabata

This comment has been minimized.

SatoshiKawabata commented Dec 1, 2017

@alexeykuzmin OK. I'm looking forward to this. Thanks.

@SatoshiKawabata

This comment has been minimized.

SatoshiKawabata commented Dec 5, 2017

@alexeykuzmin Does v1.8.2-beta.3 include this PR?

@HitomiTenshi

This comment has been minimized.

HitomiTenshi commented Dec 5, 2017

@SatoshiKawabata no, you can see it's still Chrome 59 when you open the dev tools.

electron_2017-12-05_11-24-14

You'll also have to wait for #10836 to get sorted out. Patience my man.
The new Electron version with Chrome 61 will probably get released as 1.9.x because of the Chrome and Node JS upgrades.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Dec 5, 2017

@SatoshiKawabata No, it doesn't. Versions 1.8.x are based on Chromium 59.

@SatoshiKawabata

This comment has been minimized.

SatoshiKawabata commented Dec 5, 2017

I see. Patience! Thanks.

@karel-3d

This comment has been minimized.

karel-3d commented Dec 12, 2017

Hello. Just for information. Does Electron 1.9.X with Chrome 61 support webusb?

@enihcam

This comment has been minimized.

enihcam commented on abcda09 Jun 9, 2018

@alexeykuzmin @zcbenz

Since you have upgraded Gtk+ from 2 to 3, Can you also migrate GConf to GSettings?

GConf has been deprecated for 5 years, and you should really not depend on it.

Here is the GConf-to-GSettings migration guide: https://wiki.gnome.org/Initiatives/GnomeGoals/GSettingsMigration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment