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

Added preloader isolation #2087

Merged
merged 21 commits into from Jul 5, 2017

Conversation

Projects
None yet
5 participants
@frozeman
Member

frozeman commented Mar 30, 2017

This should fix ETH-02-007, by adding preloader isolation for browser and dapps.
We could do that even for MistUI in the future, but this is not crucial.

Please test the PR with many Dapps, as this also removes sync call support for web3.js.
(There is no way to make sync calls with the postMessage transfer we have to use now.)

@alexvandesande please test ENS dapp
@MaiaVictor please take a look at ETH-02-007 and try to reproduce.
@evertonfraga could you add tests for ETH-02-007 ?

What this PR does

  • This adds context isolation for preloader files as described here: electron/electron#8348
  • Adds electron version 1.6.11
  • Added the folder: /modules/preloader/injected which contains the files which will be injected in the websites context using webFrame.executeJavaScript("..")
  • This new EthereumProvider there talks to the preloader context through window.postMessage and the preloader talks to the mist backend using ipcRender, as before. This way these two contexts are completely isolated, but can communicate.
  • it removes the sync support for web3.js calls, and there is no way around it. So we have to communicate this in release notes!
  • The mistAPI was also ported this way and functions the same way.
  • We should think about removing the old mistAPI and ethereumProvider, which is still used in the non-isolated contexts.

Once im back i can port everything to the isolated preloader, but this increases security and works.

frozeman and others added some commits Mar 30, 2017

Luca Zeug
@evertonfraga

This comment has been minimized.

Member

evertonfraga commented Apr 3, 2017

@frozeman ok, trying to figure out the failing build, then I'll make the 02-007.

luclu and others added some commits Apr 5, 2017

@evertonfraga evertonfraga modified the milestone: 0.9.0 Apr 19, 2017

evertonfraga added a commit that referenced this pull request Apr 19, 2017

frozeman and others added some commits Jun 28, 2017

@evertonfraga

This comment has been minimized.

Member

evertonfraga commented Jun 29, 2017

It still looks really unstable to me. It gives hard crashes pretty often.

First test

  • launched mist
  • loaded ethlance.com: the website had crashed due to sync calls.
  • hit CMD+R to reload it, Mist crashed

Second test

  • launched Mist
  • webviews were all blank.
  • after reloading them, they opened properly
  • when in Etherscan, at an address page, I clicked on homepage link, mist crashed, without any information on logs
  • Electron crash log: mist-second-test.txt

Third test

Random exceptions found in mist.log while using it:

Exception in queued task: TypeError: Cannot read property '0' of null
    at http://localhost:3000/packages/blaze.js?hash=10a350f1c0d55cf44d3c99989b04ef2e7e0d23f6:2872:50
    at Object.Tracker.nonreactive (http://localhost:3000/packages/tracker.js?hash=9f8a0cec09c662aad5a5e224447b2d4e88d011ef:631:12)
    at Object.changedAt (http://localhost:3000/packages/blaze.js?hash=10a350f1c0d55cf44d3c99989b04ef2e7e0d23f6:2864:17)
    at Object.changedAt (http://localhost:3000/packages/observe-sequence.js?hash=7657a09b18583bf0e90cc7b86aa029572767a87f:368:17)
    at LocalCollection._CachingChangeObserver.changed (http://localhost:3000/packages/minimongo.js?hash=66cc6ab213289f154f49d61566dba8ff9dfc33b2:3756:28)
    at Object.self.applyChange.changed (http://localhost:3000/packages/minimongo.js?hash=66cc6ab213289f154f49d61566dba8ff9dfc33b2:3709:44)
    at http://localhost:3000/packages/minimongo.js?hash=66cc6ab213289f154f49d61566dba8ff9dfc33b2:410:13
    at Meteor._SynchronousQueue.runTask (http://localhost:3000/packages/meteor.js?hash=e3f53db3be730057fed1a5f709ecd5fc7cae1229:721:11)
    at Meteor._SynchronousQueue.flush (http://localhost:3000/packages/meteor.js?hash=e3f53db3be730057fed1a5f709ecd5fc7cae1229:749:10)
    at Meteor._SynchronousQueue.drain (http://localhost:3000/packages/meteor.js?hash=e3f53db3be730057fed1a5f709ecd5fc7cae1229:757:12)
[80527:0629/172113.652792:ERROR:CONSOLE(6639)] "Uncaught (in promise) TypeError: Cannot read property 'scriptId' of null", source: chrome-devtools://devtools/bundled/inspector.js (6639)
@evertonfraga

This comment has been minimized.

Member

evertonfraga commented Jul 4, 2017

Using the recommended node version from electron, I was getting the same sorts of exceptions on electron 1.6.X described above.

Didn't get any exception on electron 1.4.15. It is looking stable to me.

@evertonfraga evertonfraga merged commit 27668ba into develop Jul 5, 2017

1 of 3 checks passed

VersionEye There are all kind of security and license issues!
Details
codeclimate 226 new issues (30 fixed)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@frozeman frozeman deleted the isolatedPreloader branch Jul 6, 2017

@evertonfraga evertonfraga added Status: On hold and removed blocked labels Jan 8, 2018

@lock

This comment has been minimized.

lock bot commented Mar 31, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Mar 31, 2018

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