Slide 1

Slide 1 text

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

Slide 2

Slide 2 text

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

Slide 3

Slide 3 text

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

Slide 4

Slide 4 text

About Igalia

Slide 5

Slide 5 text

Onion Soup

Slide 6

Slide 6 text

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

Slide 7

Slide 7 text

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

Slide 8

Slide 8 text

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

Slide 9

Slide 9 text

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/

Slide 10

Slide 10 text

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

Slide 11

Slide 11 text

● 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

Slide 12

Slide 12 text

● 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

Slide 13

Slide 13 text

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

Slide 14

Slide 14 text

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)

Slide 15

Slide 15 text

Mojo migrations

Slide 16

Slide 16 text

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

Slide 17

Slide 17 text

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

Slide 18

Slide 18 text

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

Slide 19

Slide 19 text

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

Slide 20

Slide 20 text

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 ... ...

Slide 21

Slide 21 text

// 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

Slide 22

Slide 22 text

// 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

Slide 23

Slide 23 text

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

Slide 24

Slide 24 text

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

Slide 25

Slide 25 text

● 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

Slide 26

Slide 26 text

● 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

Slide 27

Slide 27 text

● 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

Slide 28

Slide 28 text

Beyond Onion Soup

Slide 29

Slide 29 text

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

Slide 30

Slide 30 text

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

Slide 31

Slide 31 text

{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

Slide 32

Slide 32 text

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

Slide 33

Slide 33 text

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

Slide 34

Slide 34 text

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

Slide 35

Slide 35 text

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

Slide 36

Slide 36 text

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

Slide 37

Slide 37 text

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

Slide 38

Slide 38 text

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

Slide 39

Slide 39 text

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

Slide 40

Slide 40 text

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%)

Slide 41

Slide 41 text

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 ...

Slide 42

Slide 42 text

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