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
   

   View Slide

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
   

   View Slide

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
   

   View Slide

4. About Igalia
   

   View Slide

5. Onion Soup
   

   View Slide

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
   

   View Slide

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

   View Slide

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

   View Slide

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/
   

   View Slide

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

    View Slide

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
    

    View Slide

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
    

    View Slide

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

    View Slide

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)
    

    View Slide

15. Mojo migrations
    

    View Slide

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
    

    View Slide

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 mojo::Receiver
    mojo::InterfacePtr mojo::Remote
    mojo::InterfaceRequest mojo::PendingReceiver
    mojo::InterfacePtrInfo mojo::PendingRemote
    

    View Slide

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

    View Slide

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

    View Slide

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 mojo::ReceiverSet
    mojo::AssociateBinding mojo::AssociateReceiver
    mojo::AssociateInterfacePtr mojo::AssociateRemote
    mojo::StrongBinding mojo::SelfOwnedReceiver
    ... ...
    

    View Slide

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

    View Slide

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);
    };
    // C++
    void BarImpl::PokeFoo(mojo::PendingRemote foo) {
    mojo::Remote foo_remote(std::move(foo));
    foo_remote->Poke();
    }
    Mojo migrations
    Migration to new types
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

28. Beyond Onion Soup
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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, To...
    ● Result: work in progress, with 180+ CLs landed so far (~50%)
    https://crbug.com/891908
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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

    View Slide

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
    

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide