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

Upgrade to Chrome 58 #9116

Merged
merged 41 commits into from May 10, 2017

Conversation

Projects
None yet
3 participants
@zcbenz
Copy link
Contributor

zcbenz commented Apr 5, 2017

Debug build compiles on:

  • OS X
  • Linux
  • Windows

Release build compiles on:

  • OS X
  • Linux
  • Windows

Tests passing on:

  • OS X
  • Linux
  • Windows

Merge:

@zcbenz zcbenz force-pushed the chrome58 branch from d6a4a53 to a1a7c69 Apr 11, 2017

@zcbenz zcbenz force-pushed the chrome58 branch 4 times, most recently from f83b19c to 9a19e2b Apr 17, 2017

@@ -277,7 +280,7 @@ bool Converter<blink::WebMouseWheelEvent>::FromV8(
bool can_scroll = true;
if (dict.Get("canScroll", &can_scroll) && !can_scroll) {
out->hasPreciseScrollingDeltas = false;
out->modifiers &= ~blink::WebInputEvent::ControlKey;
out->setModifiers(out->modifiers() | blink::WebInputEvent::ControlKey);

This comment has been minimized.

@bbondy

bbondy Apr 24, 2017

Member

Is this logic intentionally changed? It is not equivalent.
Before it keeps all flags except for ControlKey, and now it adds ControlKey flag

This comment has been minimized.

@zcbenz

zcbenz Apr 24, 2017

Contributor

It is a mistake, thanks for finding it out!

@zcbenz zcbenz force-pushed the chrome58 branch from 900d6ea to 4c47f2d Apr 27, 2017

@zcbenz

This comment has been minimized.

Copy link
Contributor

zcbenz commented Apr 27, 2017

I have updated Node to v7.9.0 and increased node module version. The tests of native modules are failing because of the bumping of node module version.

This PR should be ready to merge, though I would expect to see unknown regressions due to Chrome and Node upgrade.

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 2, 2017

The tests of native modules are failing because of the bumping of node module version.

I've updated this branch to build the spec modules against a local headers tarball instead of a published one so hopefully this shouldn't happen again going forward when we bump the node module version.

@kevinsawicki kevinsawicki force-pushed the chrome58 branch from 4604ca5 to 164b97a May 2, 2017

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 3, 2017

Seeing some crashes from LocalArrayBufferTracker on Windows, also seen by @brenca:

   ntdll.dll!_RtlReportCriticalFailure@12()    Unknown
    ntdll.dll!_RtlpReportHeapFailure@4()    Unknown
    ntdll.dll!_RtlpHeapHandleError@4()  Unknown
    ntdll.dll!_RtlpLogHeapFailure@24()  Unknown
    ntdll.dll!_RtlFreeHeap@12() Unknown
    electron.exe!base::allocator::WinHeapFree(void *)   Unknown
    electron.exe!base::debug::Alias(void const *)   Unknown
    electron.exe!_free()    Unknown
    electron.exe!gin::ArrayBufferAllocator::Free(void * data, unsigned int length) Line 34  C++
    node.dll!v8::internal::LocalArrayBufferTracker::Process<<lambda_364280a90984476bdb46fa9c7db4695b> >(v8::internal::ArrayBufferTracker::ProcessBuffers::__l2::<lambda_364280a90984476bdb46fa9c7db4695b> callback) Line 66   C++
    node.dll!v8::internal::ArrayBufferTracker::FreeDeadInNewSpace(v8::internal::Heap * heap) Line 82    C++
    node.dll!v8::internal::Heap::Scavenge() Line 1773   C++
    node.dll!v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags) Line 1359  C++
    node.dll!v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector collector, v8::internal::GarbageCollectionReason gc_reason, const char * collector_reason, const v8::GCCallbackFlags gc_callback_flags) Line 1013    C++

Seems to occur possibly on the first GC in the main process, can reproduce pretty consistently by requiring a large library. Seeing it when running the specs on Windows against the release build when electabul gets required which in turn requires asar.

Will investigate further and try to narrow down.

Here is another one:

 	ntdll.dll!RtlFreeHeap�()	Unknown
 	electron.exe!base::allocator::WinHeapFree(void *)	Unknown
 	node.dll!v8::internal::LocalArrayBufferTracker::Process<<lambda_d5761fd40aa45311e3047f5db5a671ca> >(v8::internal::ArrayBufferTracker::ProcessBuffers::__l2::<lambda_d5761fd40aa45311e3047f5db5a671ca> callback) Line 67	C++
 	node.dll!v8::internal::ArrayBufferTracker::FreeDeadInNewSpace(v8::internal::Heap * heap) Line 82	C++
 	node.dll!v8::internal::Heap::Scavenge() Line 1773	C++
 	node.dll!v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags) Line 1359	C++
 	node.dll!v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector collector, v8::internal::GarbageCollectionReason gc_reason, const char * collector_reason, const v8::GCCallbackFlags gc_callback_flags) Line 1013	C++
 	node.dll!v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace space, v8::internal::GarbageCollectionReason gc_reason, const v8::GCCallbackFlags callbackFlags) Line 686	C++
 	node.dll!v8::internal::Factory::NewRawOneByteString(int length, v8::internal::PretenureFlag pretenure) Line 539	C++
 	node.dll!v8::internal::String::SlowFlatten(v8::internal::Handle<v8::internal::ConsString> cons, v8::internal::PretenureFlag pretenure) Line 2444	C++
 	node.dll!v8::internal::String::Flatten(v8::internal::Handle<v8::internal::String> string, v8::internal::PretenureFlag pretenure) Line 3534	C++
 	node.dll!v8::internal::Parser::ParseProgram(v8::internal::Isolate * isolate, v8::internal::ParseInfo * info) Line 626	C++
 	node.dll!v8::internal::parsing::ParseProgram(v8::internal::ParseInfo * info, bool internalize) Line 30	C++
 	node.dll!v8::internal::`anonymous namespace'::CompileToplevel(v8::internal::CompilationInfo * info) Line 1157	C++
 	node.dll!v8::internal::Compiler::GetSharedFunctionInfoForScript(v8::internal::Handle<v8::internal::String> source, v8::internal::Handle<v8::internal::Object> script_name, int line_offset, int column_offset, v8::ScriptOriginOptions resource_options, v8::internal::Handle<v8::internal::Object> source_map_url, v8::internal::Handle<v8::internal::Context> context, v8::Extension * extension, v8::internal::ScriptData * * cached_data, v8::ScriptCompiler::CompileOptions compile_options, v8::internal::NativesFlag natives) Line 1764	C++
 	node.dll!v8::ScriptCompiler::CompileUnboundInternal(v8::Isolate * v8_isolate, v8::ScriptCompiler::Source * source, v8::ScriptCompiler::CompileOptions options) Line 2151	C++
 	node.dll!v8::ScriptCompiler::CompileUnboundScript(v8::Isolate * v8_isolate, v8::ScriptCompiler::Source * source, v8::ScriptCompiler::CompileOptions options) Line 2188	C++
>	node.dll!node::ContextifyScript::New(const v8::FunctionCallbackInfo<v8::Value> & args) Line 533	C++
 	node.dll!v8::internal::FunctionCallbackArguments::Call(void(*)(const v8::FunctionCallbackInfo<v8::Value> &) f) Line 26	C++
 	node.dll!v8::internal::`anonymous namespace'::HandleApiCallHelper<1>(v8::internal::Isolate * isolate, v8::internal::Handle<v8::internal::HeapObject> function, v8::internal::Handle<v8::internal::HeapObject> new_target, v8::internal::Handle<v8::internal::FunctionTemplateInfo> fun_data, v8::internal::Handle<v8::internal::Object> receiver, v8::internal::BuiltinArguments args) Line 113	C++
 	node.dll!v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments args, v8::internal::Isolate * isolate) Line 136	C++
 	node.dll!v8::internal::Builtin_HandleApiCall(int args_length, v8::internal::Object * * args_object, v8::internal::Isolate * isolate) Line 128	C++
@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 8, 2017

Was able to track down the crash a bit more, here is a simpler reproduction:

const crypto = require('crypto')

for (let i = 1; i < 10000; i++) {
  crypto.randomBytes(i)
}

This crashes for me each time

kevinsawicki and others added some commits May 8, 2017

Use Node's memory allocator for ArrayBuffer
For Buffers created in Node, they are usually allocated in Node and
freed by Chromium's allocator, which will cause crashes when Node and
Chromium are using different allocators.

This commit makes Chromium use Node' allocator for ArrayBuffers.
@zcbenz

This comment has been minimized.

Copy link
Contributor

zcbenz commented May 9, 2017

I have pushed a fix for the crash in the main process, however when taking a heap snapshot in devtools, a crash would happen in renderer process:

 	ntdll.dll!00007ffbd39c1b70()	Unknown
 	ntdll.dll!00007ffbd39c4db2()	Unknown
 	ntdll.dll!00007ffbd39c59b0()	Unknown
 	ntdll.dll!00007ffbd397a810()	Unknown
 	node.dll!_free_base(void * block) Line 112	C++
 	[External Code]	
>	node.dll!v8::internal::NativeObjectsExplorer::FillRetainedObjects() Line 2312	C++
 	node.dll!v8::internal::NativeObjectsExplorer::IterateAndExtractReferences(v8::internal::SnapshotFiller * filler) Line 2354	C++
 	node.dll!v8::internal::HeapSnapshotGenerator::GenerateSnapshot() Line 2537	C++
 	node.dll!v8::internal::HeapProfiler::TakeSnapshot(v8::ActivityControl * control, v8::HeapProfiler::ObjectNameResolver * resolver) Line 84	C++
 	electron.exe!mate::internal::Invoker<mate::internal::IndicesHolder<0>,v8::Isolate * __ptr64>::DispatchToCallback(base::Callback<void __cdecl(v8::Isolate *),1,1> callback) Line 202	C++
 	electron.exe!mate::internal::Dispatcher<void __cdecl(v8::Isolate * __ptr64)>::DispatchToCallback(const v8::FunctionCallbackInfo<v8::Value> & info) Line 236	C++
 	node.dll!v8::internal::FunctionCallbackArguments::Call(void(*)(const v8::FunctionCallbackInfo<v8::Value> &) f) Line 26	C++
 	node.dll!v8::internal::`anonymous namespace'::HandleApiCallHelper<0>(v8::internal::Isolate * isolate, v8::internal::Handle<v8::internal::HeapObject> function, v8::internal::Handle<v8::internal::HeapObject> new_target, v8::internal::Handle<v8::internal::FunctionTemplateInfo> fun_data, v8::internal::Handle<v8::internal::Object> receiver, v8::internal::BuiltinArguments args) Line 113	C++
 	node.dll!v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments args, v8::internal::Isolate * isolate) Line 140	C++
 	node.dll!v8::internal::Builtin_HandleApiCall(int args_length, v8::internal::Object * * args_object, v8::internal::Isolate * isolate) Line 128	C++
 	[External Code]	
 	node.dll!v8::internal::Runtime_LoadIC_Miss(int args_length, v8::internal::Object * * args_object, v8::internal::Isolate * isolate) Line 2544	C++
 	[External Code]	

It seems that some native classes were allocated in Chromium but freed in Node.

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 9, 2017

I have pushed a fix for the crash in the main process

Awesome work @zcbenz, specs no longer crash for me on Windows release builds, I've merged electron/libchromiumcontent#289

A new 58 release came out today and I've queued up the builds in electron/libchromiumcontent#290, I'll merge that into this branch once it completes.

@kevinsawicki kevinsawicki merged commit 7d3b58a into master May 10, 2017

6 of 9 checks passed

electron-linux-x64 Build #6570395 failed in 118s
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #6569975 succeeded in 93s
Details
electron-linux-ia32 Build #6569976 succeeded in 91s
Details
electron-mas-x64 Build #4133 succeeded in 10 min
Details
electron-osx-x64 Build #4136 succeeded in 9 min 32 sec
Details
electron-win-ia32 Build #3118 succeeded in 10 min
Details
electron-win-x64 Build #3092 succeeded in 9 min 49 sec
Details

@kevinsawicki kevinsawicki deleted the chrome58 branch May 10, 2017

@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 10, 2017

This has been released to beta in 1.7.0, please open new issues for any bugs you hit.

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