Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Improving Chromium's code health: Onion Soup and beyond

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

Mario Sánchez Prada

November 14, 2019
Tweet

More Decks by Mario Sánchez Prada

Other Decks in Technology

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