Improving Chromium's code health: Onion Soup and beyond

This talk provides a high-level overview of the work we've been doing at Igalia during this year aimed at improving the health of the Chromium codebase. It consists of a variety of tasks such as the collaboration with the Onion Soup project, the migration to the new Mojo APIs, the removal of wtf/time.h or helping with the migration to the Identity and the Network services, among others.

Presented at the BlinkOn 11 conference on November 14th 2019 in Sunnyvale, California.

Video recording: https://youtu.be/0T-ZMW5PiDY

Transcript

1. Improving Chromium's code health: Onion Soup and beyond Antonio Gomes / Mario Sánchez Prada BlinkOn 11 / November 14th, 2019 Sunnyvale, California
2. 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
3. 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
4. About Igalia
5. Onion Soup
6. 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
7. Onion Soup: origins • Unnecessary duplication, abstractions and APIs from Blink • IPC with Blink always through //content/renderer
8. Onion Soup: the vision • Services, services, services everywhere! • Direct communication with services via Mojo interfaces • No unnecessary abstractions & layers
9. 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/
10. Onion Soup: Push Messaging • Moved code from //content/{renderer,*/common} into Blink • Removed 5 classes & 3 public WebPush* APIs
11. • 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
12. • 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
13. Onion Soup: progress • Current status (Onion Soup 1.0): 87% done • Progress tracking: Mojofication & Onion Soup
14. 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)
15. Mojo migrations
16. 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
17. 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<Interface> mojo::Receiver<Interface> mojo::InterfacePtr<Interface> mojo::Remote<Interface> mojo::InterfaceRequest<Interface> mojo::PendingReceiver<Interface> mojo::InterfacePtrInfo<Interface> mojo::PendingRemote<Interface>
18. Old types: mojo::InterfacePtr<Iface> ptr; mojo::InterfaceRequest<Iface> request = mojo::MakeRequest(&ptr); context->GetInterfaceProvider()->GetInterface(std::move(request)) ... // Possible to reuse the already-bound InterfacePtr for a new InterfaceRequest mojo::InterfaceRequest<Iface> request2 = mojo::MakeRequest(&ptr); // OK Mojo migrations Migration to new types
19. New types: mojo::Remote<Iface> remote; mojo::PendingReceiver<Iface> receiver = remote.BindNewPipeAndPassReceiver(); context->GetInterfaceProvider()->GetInterface(std::move(receiver)) ... mojo::PendingReceiver<Iface> receiver2 = remote.BindNewPipeAndPassReceiver(); // FAIL (requires: remote.reset()) Mojo migrations Migration to new types
20. 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<Interface> mojo::ReceiverSet<Interface> mojo::AssociateBinding<Interface> mojo::AssociateReceiver<Interface> mojo::AssociateInterfacePtr<Interface> mojo::AssociateRemote<Interface> mojo::StrongBinding<Interface> mojo::SelfOwnedReceiver<Interface> ... ...
21. // 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<mojom::FooFactory> factory_binding_; mojo::BindingSet<mojom::Foo> foo_bindings_; }; // mojom interface FooFactory { GetFoo(pending_receiver<Foo> receiver); }; // C++ class FooFactoryImpl : public mojom::FooFactory { public: explicit FooFactoryImpl( mojo::PendingReceiver<mojom::FooFactory> receiver) : factory_receiver_(this, std::move(receiver)) {} // mojom::FooFactory: void GetFoo(mojo::PendingReceiver<mojom::Foo> receiver) override { foo_receivers_.Add(this, std::move(receiver)); } private: mojo::Receiver<mojom::FooFactory> factory_receiver_; mojo::ReceiverSet<mojom::Foo> foo_receivers_; }; Mojo migrations Migration to new types
22. // 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> foo); }; // C++ void BarImpl::PokeFoo(mojo::PendingRemote<mojom::Foo> foo) { mojo::Remote<mojom::Foo> foo_remote(std::move(foo)); foo_remote->Poke(); } Mojo migrations Migration to new types
23. 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
24. 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
25. • 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
26. • 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
27. • 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
28. Beyond Onion Soup
29. 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
30. 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
31. {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
32. 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
33. 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
34. 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<T>, To<T>... • Result: work in progress, with 180+ CLs landed so far (~50%) https://crbug.com/891908
35. 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
36. 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
37. Removal of WTF::RefVector • Problem: WTF::RefVector was not broadly used in Blink as it’s essentially base::RefCountedData<WTF::Vector<T>> • Solution: Migrate WTF::RefVector to base::RefCountedData and remove the old type. • Result: Completed https://crbug.com/955618
38. 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
39. Remove unneeded Create() methods • Problem: using Create() methods for garbage collectable classes that simply call MakeGarbageCollected<T> 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<T>. • Result: Work in progress (~50%) https://crbug.com/939691
40. 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%)
41. 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 ...
42. Thanks! • Mail: {tonikitoo,mario}@igalia.com • Blogs: https://blogs.igalia.com/{tonikitoo,mario} • Twitter: @tonikitoo / @mariospr