Improving Chromium's code health: Onion Soup and beyond Antonio Gomes / Mario Sánchez Prada BlinkOn 11 / November 14th, 2019 Sunnyvale, California 

About Us CS Engineers & members of Igalia’s Chromium Team ● Antonio (tonikitoo@) ○ Firefox: MiniMo / Fennec ○ WebKit: Qt WebKit, WebKitEFL ○ Chromium: Embedding, Blink, Servicification, Onion Soup ● Mario (mario@) ○ GNOME: Contributor, GNOME Foundation member ○ WebKit: WebKitGTK+, Linux Accessibility ○ Chromium: Servicification, Onion Soup, Mojo migrations 

About Igalia ● Highly specialized Open Source consultancy ● Worker-owned, employee-run, flat structure ● Browser technologies teams: ○ Chromium, WebKit, Web Platform, Compilers ● Headquartered in A Coruña, Galicia (Spain) ● ~90 people working from all over the world 

About Igalia 

Onion Soup 

Onion Soup Started almost 4 years ago: Onion Soup 1.0: ● Motivation: After Blink was forked from WebKit, many layers of abstraction between Chromium and Blink are no longer needed ● Goal: “simplify the codebase, empower developers to implement features that run faster, and remove hurdles for developers interfacing with the rest of the Chromium”. ● Plan: replace legacy IPC with Mojo, move functionality from //content/renderer into Blink, clean up Blink public APIs 

Onion Soup: origins ● Unnecessary duplication, abstractions and APIs from Blink ● IPC with Blink always through //content/renderer 

Onion Soup: the vision ● Services, services, services everywhere! ● Direct communication with services via Mojo interfaces ● No unnecessary abstractions & layers 

Onion Soup: our work (since February 2019) ● Migrated or (helped to migrate) several modules: ● Removed unnecessary public APIs from Blink WebRtcAnswerOptions, WebRTCCertificateGenerator, WebDatabaseObserver, WebPushClient, WebSurroundingText... media/stream/ android/ media_recorder/ devtools/ appcache/ push_messaging/ webdatabase/ media_capture_from_element/ installedapp/ image_capture/ manifest/ media/webrtc/ p2p/ 

Onion Soup: Push Messaging ● Moved code from //content/{renderer,*/common} into Blink ● Removed 5 classes & 3 public WebPush* APIs 

● Migrated from old IPC to Mojo ● Changed the lifetime of key objects, eg ○ PeerConnectionTracker ○ PeerConnectionDependencyFactory ● Worked on phases (1/2): ○ independent classes ○ interdependent classes ■ light ■ hard ○ Blink API pollution clean up pass Onion Soup: WebRTC 

● Worked on phases (2/2): ○ “Blinkify” the code std::vector -> WTF::Vector std::string -> WTF::String …. base::Bind -> WTF::Bind / CrossThreadBind base::RefCountedThreadSafe -> WTF::ThreadSafeRefCounted ○ API clean up move code out of public/ remove API layers Onion Soup: WebRTC 

Onion Soup: progress ● Current status (Onion Soup 1.0): 87% done ● Progress tracking: Mojofication & Onion Soup 

Onion Soup: what’s next Started working on Onion Soup 2.0: ● Finish Onion Soup 1.0 accessibility/, java/, loader/, media/, service_worker/, worker/ ● Convert legacy IPC in //content/ to Mojo ● Switch Mojo to new syntax ● Slim down Blink public APIs ● Remove the {platform,core,modules}/exported layers ● [Maybe] Remove Blink mojom types (mojom::blink::X) 

Mojo migrations 

Context: ● Original types are confusing and allow error prone behaviour ● New types provide a clearer API and enforce correct usage Goal: Migrate all chromium away from the old mojo types Also: ● New types [binary] compatible with the new ones ● Implicit conversions between old and new types available https://crbug.com/955171 Mojo migrations Migration to new types 

Started with the migration of “bindings” and “remote pointers”: Reference document: Mojo Bindings Conversion Cheatseet Mojo migrations Migration to new types Old type New type mojo::Binding mojo::Receiver mojo::InterfacePtr mojo::Remote mojo::InterfaceRequest mojo::PendingReceiver mojo::InterfacePtrInfo mojo::PendingRemote 

Old types: mojo::InterfacePtr ptr; mojo::InterfaceRequest request = mojo::MakeRequest(&ptr); context->GetInterfaceProvider()->GetInterface(std::move(request)) ... // Possible to reuse the already-bound InterfacePtr for a new InterfaceRequest mojo::InterfaceRequest request2 = mojo::MakeRequest(&ptr); // OK Mojo migrations Migration to new types 

New types: mojo::Remote remote; mojo::PendingReceiver receiver = remote.BindNewPipeAndPassReceiver(); context->GetInterfaceProvider()->GetInterface(std::move(receiver)) ... mojo::PendingReceiver receiver2 = remote.BindNewPipeAndPassReceiver(); // FAIL (requires: remote.reset()) Mojo migrations Migration to new types 

Several other types had to be migrated as well: More details: Mojo Bindings Conversion Cheatseet Mojo migrations Migration to new types Old type New type mojo::BindingSet mojo::ReceiverSet mojo::AssociateBinding mojo::AssociateReceiver mojo::AssociateInterfacePtr mojo::AssociateRemote mojo::StrongBinding mojo::SelfOwnedReceiver ... ... 

// mojom interface FooFactory { GetFoo(Foo& request); }; // C++ class FooFactoryImpl : public mojom::FooFactory { public: explicit FooFactoryImpl( mojom::FooFactoryRequest request) : binding_(this, std::move(request)) {} // mojom::FooFactory: void GetFoo(mojom::FooRequest request) override { foo_bindings_.AddBinding(this, std::move(request)); } private: mojo::Binding factory_binding_; mojo::BindingSet foo_bindings_; }; // mojom interface FooFactory { GetFoo(pending_receiver receiver); }; // C++ class FooFactoryImpl : public mojom::FooFactory { public: explicit FooFactoryImpl( mojo::PendingReceiver receiver) : factory_receiver_(this, std::move(receiver)) {} // mojom::FooFactory: void GetFoo(mojo::PendingReceiver receiver) override { foo_receivers_.Add(this, std::move(receiver)); } private: mojo::Receiver factory_receiver_; mojo::ReceiverSet foo_receivers_; }; Mojo migrations Migration to new types 

// mojom interface Foo { Poke(); }; interface Bar { PokeFoo(Foo foo); }; // C++ void BarImpl::PokeFoo(mojom::FooPtr foo) { foo->Poke(); } // mojom interface Foo { Poke(); }; interface Bar { PokeFoo(pending_remote foo); }; // C++ void BarImpl::PokeFoo(mojo::PendingRemote foo) { mojo::Remote foo_remote(std::move(foo)); foo_remote->Poke(); } Mojo migrations Migration to new types 

Mojo migrations Migration to new types 6/1/2019 6/7/2019 ... 10/31/2019 11/7/2019 Blink 441 441 12 12 Content 1601 1599 --- 201 176 Other 3070 3039 721 647 TOTAL 5112 5079 --- 934 835 

Current status (between June 1st 2019 and Nov 7th 2019): ● Global progress: 84% ● Blink: 97% ● Content: 89% ● Other: 79% Progress tracking: Migration to new mojo types https://crbug.com/955171 https://crbug.com/978694 Mojo migrations Migration to new types 

● Context: Previously using InterfaceProvider to retrieve interfaces for several execution contexts: Frames SharedWorkers ServiceWorkers DedicatedWorkers ● InterfaceProvider relies on InterfaceFilter capabilities which are complex and haven’t proved to be needed beyond expressing document-scoped capabilities ● Also, InterfaceProvider APIs rely on old mojo types, migrating to BrowserInterfaceBroker would help with that too Mojo migrations Migration to BrowserInterfaceBroker 

● Goal: migrate from retrieving remote interfaces using the InterfaceProvider to the BrowserInterfaceBroker exposed by each execution context’s host. ● Progress: Migrated 57/121 cases (~47%): ○ //content: frames: 37/64 / workers: 7/12 ○ //chrome: 12/40 ○ //cast: 0/1 ○ //android: 1/4 https://crbug.com/936482 Mojo migrations Migration to BrowserInterfaceBroker 

● Problem: Blink-related mojo interfaces exposed via .mojom files scattered under public/{web,platform} and public/mojom ● Result: all (15) Blink-related .mojom files in public/mojom/: autoplay.mojom, mime_registry.mojom, app_banner.mojom, web_feature.mojom, ooom_intervention.mojom... https://crbug.com/919393 Mojo migrations Re-organized location of .mojom files 

Beyond Onion Soup 

Servicification (s13n) ● Network Service (//services/network) ○ Migrated to the network::SimpleURLLoader API ○ Enabled on Mac/Win/Linux/CrOS since March ○ Current status: completed ● Identity service (//services/identity). ○ Helped migrate to the new IdentityManager API ○ Weekly status report at identity-service-dev ML ○ Current status: completed 

Time representation ● Problem: different ways used to represent time throughout chromium: base::Time, WTF::Time and even doubles! ● Solution: Migrate to base::Time and remove wtf/time.h. ● Result: Migrated usage of WTF::Time to base::Time, doubles to base::Time{Ticks,Delta}, tests using clock overrides to using TestMockTimeTaskRunner and removed wtf/time.h. Partial migration of doubles in the Blink animation engine, though https://crbug.com/919383 

{Once,Repeating}Callback ● Problem: lots of places using Callback and Bind(), which are aliases of RepeatingCallback and BindRepeating(), when the registered callback will be called only one time: difficult to reason about ownership and object’s lifetime. ● Solution: audit usages of Callback and Bind() to find and convert cases to OnceCallback and BindOnce() when possible. ● Result: migrated net::CompletionCallback for now. https://crbug.com/714018 

CrossThreadBind{Once,Repeating} ● Problem: Blink’s WTF::Bind and WTF::BindRepeating() don’t ensure that callbacks posted to different task runners properly pass/copy their parameters across threads. ● Solution: implement CrossThreadBindOnce() and rename CrossThreadBindRepeating() so that both rely on the CrossDataCopier data structure, ensuring thread-safety. ● Result: Completed https://crbug.com/963574 

Migration of GC-able classes to FastMalloc ● Problem: all blink objects should be managed by the garbage collector or PartitionAlloc from the memory management perspective, otherwise objects can create dangling pointers or memory leaks. ● Solution: migrate usages of malloc() for garbage-collectable objects to using the FastMalloc partition instead. ● Result: Completed https://crbug.com/919389 

Migration to downcast helpers ● Problem: Old implementation was implemented using C++ macros (ToSubclass[OrNull,Checked]), which are unsafe and problematic to debug while testing/fuzzing ● Solution: replace old C++ casting macros with a new and safer implementation relying on C++ type traits: IsA, To... ● Result: work in progress, with 180+ CLs landed so far (~50%) https://crbug.com/891908 

Migration to precise-width integers ● Problem: C++ standard doesn’t specify the specific size of each integer type, and sizes of integral types in C++ can vary. ● Solution: Replace imprecise-width integers with precise-width ones in chromium: long long -> int64, short -> int16_t... ● Result: Completed (+ added presubmit warnings for banned types) https://crbug.com/929940 

Migration to WTF containers ● Problem: banned std:: containers being wrongly used in many places of Blink, in violation of the coding guidelines. ● Solution: Replace usage of std::map, std::multimap, std::unordered_{set,map}, std::set, std::deque, and std::vector with equivalent types from WTF. ● Result: Completed except for boundaries around Blink. (+ added presubmit warnings for banned types) https://crbug.com/952716 

Removal of WTF::RefVector ● Problem: WTF::RefVector was not broadly used in Blink as it’s essentially base::RefCountedData> ● Solution: Migrate WTF::RefVector to base::RefCountedData and remove the old type. ● Result: Completed https://crbug.com/955618 

Removal of CString from Blink ● Problem: despite having WTF::String, CString was used to represents series of 8-bit characters, usually ASCII or UTF-8, which required doing conversions when moving from/to Blink. ● Solution: replace CString with std::string so that calls to Blink methods like String::Ascii(), String::Latin1() or String::Utf8() now return an std::string. ● Result: Completed https://crbug.com/950077 

Remove unneeded Create() methods ● Problem: using Create() methods for garbage collectable classes that simply call MakeGarbageCollected to create the instances are not needed and against the coding style. ● Solution: replace usages of those Create() methods for with direct calls to MakeGarbageCollected. ● Result: Work in progress (~50%) https://crbug.com/939691 

Summary Area Progress Area Progress Onion Soup 1.0 WIP (87%) CrossThreadBind{Once,Repeating} Completed Mojo: migration to the new types WIP (84%) Migration of GC-able classes to FastMalloc Completed Mojo: BrowserInterfaceBroker WIP (47%) Migration to downcast helpers WIP (50%) Mojo: relocation of .mojom files Completed Migration to precise-width integers Completed Servification: Network & Identity Completed Migration to WTF containers Completed Time representation: wtf/time.h Completed Removal of WTF::RefVector Completed Time representation: doubles Partial Removal of CString from Blink Completed {Once,Repeating}Callback net:: only Removal of Create() methods WIP (50%) 

Credit Igalians contributing to this work: Abhijeet Kandalkar Antonio Gomes Gyuyoung Kim Henrique Ferreiro Jacobo Aragunde Julie Jeongeun Kim Mario Sánchez Prada Miyoung Shin Sergio Villar Senin Thanks to the many reviewers who helped Camille Lamy Colin Blundell Daniel Cheng Dave Tapuska David Roger Dominick NG Guido Urdaneta Jeremy Roman ... John Abd-El-Malek Ken Rockot Kentaro Hara Kinuko Yasuda Lowell Manners Matt Menke Oksana Zhuravlova Sylvain Defresne ... 

Thanks! ● Mail: {tonikitoo,mario}@igalia.com ● Blogs: https://blogs.igalia.com/{tonikitoo,mario} ● Twitter: @tonikitoo / @mariospr