8000 Upgrade electron to 1.7.10 by leroix · Pull Request #16282 · atom/atom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Upgrade electron to 1.7.10 #16282

Merged
merged 10 commits into from
Jan 3, 2018
Merged

Upgrade electron to 1.7.10 #16282

merged 10 commits into from
Jan 3, 2018

Conversation

leroix
Copy link
Contributor
@leroix leroix commented Nov 28, 2017

☝️

Fixes #15696
Fixes #15737

@leroix leroix changed the title ⬆️ electron@1.7.9 Upgrade electron to 1.7.9 Nov 28, 2017
@leroix
Copy link
Contributor Author
leroix commented Nov 28, 2017

I think there are going to be a few failing tests that we'll need to figure out. That's what I'm seeing locally. Overall, it doesn't look too bad.

@leroix
Copy link
Contributor Author
leroix commented Nov 28, 2017

I realized later that this is still crashing when run in standard mode (non-dev).

@leroix
Copy link
Contributor Author
leroix commented Dec 5, 2017

I tried disabling snapshot generation in the build script, and Atom starts up without crashing in non-dev mode.

@leroix
Copy link
Contributor Author
leroix commented Dec 7, 2017 8000

I've been narrowing down the cause of the crashes, and I've gotten down to this diff:

diff --git a/src/config.coffee b/src/config.coffee
index b8bf8a7..67b00d3 100644
--- a/src/config.coffee
+++ b/src/config.coffee
@@ -1064,7 +1064,7 @@ class Config
         continue unless scopeSchema.hasOwnProperty('default')
         scopedDefaults[scope] = {}
         setValueAtKeyPath(scopedDefaults[scope], keyPath, scopeSchema.default)
-      @scopedSettingsStore.addProperties('schema-default', scopedDefaults)
+        #@scopedSettingsStore.addProperties('schema-default', scopedDefaults)
 
     if schema.type is 'object' and schema.properties? and isPlainObject(schema.properties)
       keys = splitKeyPath(keyPath)
@@ -1140,7 +1140,7 @@ class Config
 
     settingsBySelector = {}
     settingsBySelector[selector] = value
-    @scopedSettingsStore.addProperties(source, settingsBySelector, priority: @priorityForSource(source))
+    #@scopedSettingsStore.addProperties(source, settingsBySelector, priority: @priorityForSource(source))
     @emitChangeEvent()
 
   getRawScopedValue: (scopeDescriptor, keyPath, options) ->
diff --git a/src/package-manager.js b/src/package-manager.js
index 17a5f22..a4d5a8a 100644
--- a/src/package-manager.js
+++ b/src/package-manager.js
@@ -511,7 +511,7 @@ module.exports = class PackageManager {
 
     pack = metadata.theme ? new ThemePackage(options) : new Package(options)
     pack.preload()
-    this.preloadedPackages[packageName] = pack
+    //this.preloadedPackages[packageName] = pack
     return pack
   }

With these lines commented out, Atom no longer crashes.

@leroix
Copy link
Contributor Author
leroix commented Dec 8, 2017

Lately, I've been modifying the startup script "out/startup.js" directly, creating a new snapshot, and starting up Atom to debug the crashes.

To create a new snapshot:

./script/node_modules/electron-mksnapshot/bin/mksnapshot out/startup.js --startup_blob out/Atom.app/Contents/Frameworks/Electron\ Framework.framework/Resources/snapshot_blob.bin

The feedback loop is a bit faster.

@leroix
Copy link
Contributor Author
leroix commented Dec 10, 2017

Note: commenting out the package preload stops the crashes i.e.

//pack.preload()
//this.preloadedPackages[packageName] = pack

@leroix
Copy link
Contributor Author
leroix commented Dec 10, 2017

Yet another twist:

In the atom repo with a snapshot blob from a prior run of ./script/build:

$ npm install electron@1.7.9
$ cp out/Atom.app/Contents/Frameworks/Electron\ Framework.framework/Resources/snapshot_blob.bin './node_modules/electron/dist/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Resources/snapshot_blob.bin'
$ ./node_modules/electron/cli.js out/app/src/main-process/main.js

You should see Atom load without crashing.

@leroix
Copy link
Contributor Author
leroix commented Dec 13, 2017

I've narrowed things down a bit further. If I made the following changes in "./out/startup.js" and create a new snapshot, Atom does not crash:

Comment out this line:

//this.preloadedPackages[packageName] = pack

Change the parse function in atom-slick to:

     var parse = function(expression){
          if (expression == null) return null
          //expression = ('' + expression).replace(/^\s+|\s+$/g, '')
          expression = ('').replace(/^\s+|\s+$/g, '')
          return cache[expression] || (cache[expression] = new Expressions(expression))
      }

Then, I create a new snapshot and run Atom:

$ ./script/node_modules/electron-mksnapshot/bin/mksnapshot out/startup.js --startup_blob ./out/Atom.app/Contents/Frameworks/Electron\ Framework.framework/Resources/snapshot_blob.bin && ./out/Atom.app/Contents/MacOS/Atom .

@leroix
Copy link
Contributor Author
leroix commented Dec 15, 2017

I found another patch that doesn't crash. Besides all the language packages (no idea why those are causing crashes), it only removes two packages (github and whitespace).

diff --git a/package.json b/package.json
index cca8ea9..e1df733 100644
--- a/package.json
+++ b/package.json
@@ -109,7 +109,6 @@
     "exception-reporting": "0.42.0",
     "find-and-replace": "0.215.0",
     "fuzzy-finder": "1.7.3",
-    "github": "0.8.3",
     "git-diff": "1.3.6",
     "go-to-line": "0.32.1",
     "grammar-selector": "0.49.8",
@@ -134,41 +133,13 @@
     "tree-view": "0.221.3",
     "update-package-dependencies": "0.13.0",
     "welcome": "0.36.6",
-    "whitespace": "0.37.5",
     "wrap-guide": "0.40.3",
-    "language-c": "0.58.1",
     "language-clojure": "0.22.5",
-    "language-coffee-script": "0.49.3",
-    "language-csharp": "0.14.3",
-    "language-css": "0.42.8",
-    "language-gfm": "0.90.2",
-    "language-git": "0.19.1",
-    "language-go": "0.44.3",
-    "language-html": "0.48.3",
     "language-hyperlink": "0.16.3",
-    "language-java": "0.27.6",
-    "language-javascript": "0.127.7",
-    "language-json": "0.19.1",
-    "language-less": "0.34.1",
-    "language-make": "0.22.3",
     "language-mustache": "0.14.4",
-    "language-objective-c": "0.15.1",
-    "language-perl": "0.38.1",
-    "language-php": "0.42.2",
-    "language-property-list": "0.9.1",
-    "language-python": "0.45.5",
-    "language-ruby": "0.71.4",
     "language-ruby-on-rails": "0.25.2",
-    "language-sass": "0.61.3",
-    "language-shellscript": "0.25.4",
-    "language-source": "0.9.0",
-    "language-sql": "0.25.8",
     "language-text": "0.7.3",
-    "language-todo": "0.29.3",
-    "language-toml": "0.18.1",
-    "language-typescript": "0.2.3",
-    "language-xml": "0.35.2",
-    "language-yaml": "0.31.1"
+    "language-todo": "0.29.3"
   },
   "private": true,
   "scripts": {
diff --git a/src/initialize-application-window.coffee b/src/initialize-application-window.coffee
index f8f670c..ec5469b 100644
--- a/src/initialize-application-window.coffee
+++ b/src/initialize-application-window.coffee
@@ -29,7 +29,7 @@ if global.isGeneratingSnapshot
   require('dalek')
   require('find-and-replace')
   require('fuzzy-finder')
-  require('github')
+  #require('github')
   require('git-diff')
   require('go-to-line')
   require('grammar-selector')
@@ -54,7 +54,7 @@ if global.isGeneratingSnapshot
   require('tree-view')
   require('update-package-dependencies')
   require('welcome')
-  require('whitespace')
+  #require('whitespace')
   require('wrap-guide')
 
 clipboard = new Clipboard

@winstliu
Copy link
Contributor

All the whitelisted languages don't contain a settings folder, except for language-clojure.

@macrozone
Copy link

electron 1.7.10 has been released which seems to fix a problem with nvidia graphic cards on mac os high sierra. Maybe we could update to 1.7.10 directly instead of 1.7.9

Nathan Sobo and others added 9 commits December 21, 2017 09:59
This commit uses `content` containment (i.e. `layout paint style`) as
opposed to `strict` containment (i.e. `layout paint style size`) for
dummy scrollbar elements.

By removing `size` containment we are fixing a rendering bug that was
preventing the scrollbar from being sized correctly. This problem was
caught by a TextEditorComponent test
(https://circleci.com/gh/atom/atom/6393).
@as-cii
Copy link
Contributor
as-cii commented Dec 22, 2017

Okay, I think this might be ready for a test drive.

I have fixed the failing tests and, along with @nathansobo and @leroix, we finally figured out why snapshots were making Atom crash during startup (see electron/electron#11387 for all the details). The good news there is that it was a bug in V8 which is already fixed on V8 6.3 and we just have to wait until a version of Electron that embeds such version gets released (presumably Electron 1.9 or later).

@ungb @rsese @Ben3eeE @50Wliu: can you help us test this pull-request? After a cursory look everything seems reasonable. I briefly tested the IME interaction and it seemed to work correctly, although I'd love to get a more thorough QA pass on that, especially since I have removed some workarounds that we added specifically for Chrome 56 in 69799d3.

@as-cii as-cii changed the title Upgrade electron to 1.7.9 Upgrade electron to 1.7.10 Dec 22, 2017
@ungb
Copy link
Contributor
ungb commented Jan 2, 2018

For the following test, it seems that the l character resolves to - and doesn't seem to resolve to the keybinding. For h,j,k, this works correctly.

Key binding with composition characters are ignored #15189

macOS:

This should be tested on macOS

Steps to repro:

  1. Open your keymap.cson file and add the following:
# general
'atom-text-editor':
# use jkl; vim-like when pressing alt (word by word, paragraph by paragraph)
# or alt (letter by letter)
  'alt-h': 'core:move-left'
  'alt-j': 'core:move-down'
  'alt-k': 'core:move-up'
  'alt-l': 'core:move-right'
  1. Save keymap.cson and reload atom.
  2. Try the new keymappings under ABC - Extended keyboard layout

Expected: the keybindings work

Issues we've seen: it types out the modified keys and ignores the key mapping

I've tried stable and it seems to be the same behavior so this isn't a regression I suppose.

@nathansobo nathansobo merged commit 5743c41 into master Jan 3, 2018
@nathansobo nathansobo deleted the io-electron-1.7 branch January 3, 2018 23:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0