January 2023 - webkit-dev - lists.webkit.org

InjectedBundle deprecation
by Alex Christensen 01 Aug '25

01 Aug '25

Hello WebKit developers! We are ramping up to do some serious work on Site Isolation which includes putting cross-origin iframes in a different process than the parent frame. Similar efforts have been done in other browser engines and some related work has already been done in WebKit, but not enough. This will do strange things to the InjectedBundle APIs, which have fundamental assumptions that the whole DOM is in one process and that communication only happens with one process at a time. We have never exposed InjectedBundle APIs as public API, but some other distributors of WebKit have. As we make progress on this project, we anticipate that anything in Source/WebKit/WebProcess/InjectedBundle/API is subject to deprecation and removal. This will also allow us to tighten the sandbox on the web process, resolve security and performance issues, and have cleaner code. Would the maintainers of the GTK and WPE APIs be willing to assist in migrating from those APIs and designing replacements in the UI process? Alex Christensen

5 17

WebKit Documentation
by Brandon Stewart 16 Mar '23

16 Mar '23

Hi WebKit Developers, Documentation is an important part of any open source project, especially for a larger project like WebKit. Being able to ramp up during the onboarding process, reading up on architectural decisions, and learning how to perform common procedures are all features the documentation should tackle. WebKit has a large set of documentation already, but it is scattered around a wide range of platforms (Trac, GitHub Wiki, markdown files in the code, Git commits, etc...), and some of the information is out of date. A few months ago, I started working on a new documentation solution based on the DocC documentation framework. This provides an easy way to add and edit documentation through markdown files. I have already ported a large section of Trac, all of the GitHub Wiki, and all of the non third party markdown files in the code over to this platform. I have tested this on macOS and Linux and have found it works extremely well. (Windows should be able to use WSL2 at the moment, while a few remaining issues get sorted out). The only dependency for this project is a recent installation of Swift. You can already download the documentation and preview it locally, but we are looking to publish it online for easy viewing. We were looking to get your feedback if we would want to publish the documentation on GitHub Pages or webkit.org. The documentation source can be found at https://github.com/webkit/documentation. - Brandon

13 22

Smart Pointer Analysis Tool for WebKit
by Ryosuke Niwa 01 Mar '23

01 Mar '23

Hi all, I’ve been working with Geoff (ggaren) and Jan Korous from Apple's compiler team to create a static analyzer which detects dangerous use of ref counted objects, and we’re looking for folks who are interested in trying out this tool and helping us refine the tool. Please let me know if you’re interested in using this tool & try applying code changes to WebKit. Our goal is to eventually integrate this tool into buildbot and EWS so that we can catch dangerous code before they get committed. *What is Dangerous?* So *what is* a *dangerous* use of ref counted objects you may ask? It’s any use of which we can’t trivially conclude that it doesn’t lead to a use-after-free. For now, we don’t detect dangerous use of non-ref counted objects including ones that can vend WeakPtr. It’s on us, humans, to decide which objects need to be ref counted or need to be CanMakeWeakPtr. Consider the following example. This code most certainly will lead to a use-after-free of “parent” in the third line because the code doesn’t keep the parent alive. Because isContentEditable updates style in some cases, it could lead to arbitrary script execution which could remove the parent from the document. Node* parent = element->parentElement(); if (parent->isContentEditable()) parent->scrollIntoView(); In general, relying on a complex data structure such as DOM tree to keep RefCounted objects alive while we call a non-trivial function is not safe. All it takes for the code to have a use-after-free is for someone to start updating style, layout, etc… inside the function either directly or indirectly. And we don’t want to make WebKit development really hard by forcing anyone who modifies a function to check every caller of the function and their callers, etc… to make sure it’s safe to do so. For this reason, it’s *dangerous* to store a raw pointer or a reference to a ref counted object as a local variable and use it after calling a non-trivial function. We did a similar analysis of a number of other patterns and usage of ref counted objects in WebKit and came up with the following basic rules for using ref counted objects in a safe manner. We’re hoping that these rules would be eventually incorporated into our coding style guideline: https://webkit.org/code-style-guidelines/ *Rules for Using Ref Counted Objects* 1. Every data member to a ref counted object must use either Ref, RefPtr, or WeakPtr. webkit.NoUncountedMemberChecker <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checke…> 2. Every ref counted base class, if it has a derived class, must define a virtual destructor. webkit.RefCntblBaseVirtualDtor <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checke…> 3. Every ref counted object passed to a non-trivial function as an argument (including "this" pointer) should be stored as a Ref or RefPtr in the caller’s local scope unless it's an argument to the caller itself by transitive property [1]. alpha.webkit.UncountedCallArgsChecker <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checke…> 4. Every ref counted object must be captured using Ref or RefPtr for lambda. webkit.UncountedLambdaCapturesChecker <https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checke…> 5. Local variables - we’re still working on this ( https://reviews.llvm.org/D83259) Below, I've dissected each one of these rules with the real warning emitted by the analysis tool in development. Please let me know if you have any comments / concerns. ------------------------------ (1) is pretty trivial. Every ref counted data member should be stored using Ref, RefPtr, or WeakPtr since it would be not trivially obvious that life cycles of two or more objects are correctly tied or managed together. ------------------------------ (2) is also pretty easy to understand. In the following example, if someone destroys an instance of B using Ref<A>, then it would result in an undefined behavior. struct A : public RefCounted<A> { Vector<int> someData; }; struct B : public A { Vector<int> otherData; }; ------------------------------ For (3), passing a ref counted object as a raw pointer or reference to a function as an argument, the tool may generate a warning like this: Source/WebCore/html/FormAssociatedElement.cpp:181:13: warning: [WebKit] call argument is a raw pointers to a ref-countable type [-Wfunction-arg-ptr-to-refcntbl] setForm(findAssociatedForm(&asHTMLElement(), originalForm.get())); ^ This warns that void setForm(HTMLFormElement*) is called with the result of findAssociatedForm, which returns HTMLFormElement* without storing it anywhere. If setForm can somehow cause HTMLFormElement to be deleted, then this can result in the use-after-free in setForm. void FormAssociatedElement::resetFormOwner() { RefPtr<HTMLFormElement> originalForm = m_form.get(); setForm(findAssociatedForm(&asHTMLElement(), originalForm.get())); HTMLElement& element = asHTMLElement(); auto* newForm = m_form.get(); if (newForm && newForm != originalForm && newForm->isConnected()) element.document().didAssociateFormControl(element); } Why, you may ask, we don't put HTMLFormElement* in a Ref or RefPtr in setForm instead? This is because a lot of code has calls to a function with a local variable. Because the callee doesn’t know whether the caller has already stored each argument in Ref / RefPtr or not, we need to be safe and store them again in Ref / RefPtr, resulting in an unnecessary ref-churn. Additionally, if we took the approach of the callee being responsible for keeping every argument alive, then every non-trivial member function of a ref counted object must have "protectedThis" at the beginning of the function, which would be particularly wasteful if those member functions are sometimes called by other member functions. Once this rule is applied everywhere, for example, we can get rid of “protectedThis” from our codebase because it would be redundant. Furthermore, this rule transitively allows a raw pointer or a reference passed in as an argument to be used as arguments to call another function without storing it in Ref or RefPtr. For example, the following function satisfies this rule because both oldDocument and newDocument are raw references passed to this function as arguments, and therefore the caller of this function must have already stored as Ref / RefPtr as local variables. void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument, Document& newDocument) { ASSERT_WITH_SECURITY_IMPLICATION(&document() == &newDocument); if (m_shouldDelayLoadEvent) { oldDocument.decrementLoadEventDelayCount(); newDocument.incrementLoadEventDelayCount(); } unregisterWithDocument(oldDocument); registerWithDocument(newDocument); HTMLElement::didMoveToNewDocument(oldDocument, newDocument); updateShouldAutoplay(); } ------------------------------ For (4), capturing a ref counted object as a raw pointer or a reference, the tool may generate a warning like this: Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:115:74: warning: [WebKit] captured a raw pointer to a ref-countable type [-Wlambda-capture-ptr-to-refcntbl] cookieManager->deleteCookie(m_owningDataStore->sessionID(), cookie, [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)]() mutable { ^ This is warning about webPage object getting captured using a raw reference in the following function: void HTTPCookieStore::deleteCookie(const WebCore::Cookie& cookie, CompletionHandler<void()>&& completionHandler) { auto* pool = m_owningDataStore->processPoolForCookieStorageOperations(); if (!pool) { if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session) deleteCookieFromDefaultUIProcessCookieStore(cookie); else m_owningDataStore->removePendingCookie(cookie); RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] () mutable { completionHandler(); }); return; } auto* cookieManager = pool->supplement<WebKit::WebCookieManagerProxy>(); cookieManager->deleteCookie(m_owningDataStore->sessionID(), cookie, [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)]() mutable { completionHandler(); }); } Here, we should have stored WebProcessPool in Ref instead. - R. Niwa

9 17

git-webkit pr now supports --no-ews / --ews flags
by Aakash Jain 27 Jan '23

27 Jan '23

Hi Everyone, git-webkit pr now supports --no-ews / --ews flags (as of 259115@main). This is similar to --no-ews flag we had in webkit-patch. You can also manually add skip-ews label on any PR when you don't need EWS to run anymore on it. --no-ews flag will also automatically add the skip-ews label on the corresponding GitHub PR. Please feel free to use skip-ews GitHub label or --no-ews flag in git-webkit pr, when you don't need to run EWS for any reason (e.g.: PR failed a critical EWS indicating need for new iteration, and you don't need to run rest of the EWSes on that commit). If you notice any issues, please let me know or file bugs (and assign to me). Thanks Aakash

2 2

Stop Using Raw Pointers & References in New Code
by Ryosuke Niwa 27 Jan '23

27 Jan '23

Hi all, Alex recently added ThreadSafeWeakPtr <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/ThreadSafeWeakPtr…>. That should address the last remaining use case of raw pointers (T*) and references (T&). I suggest we stop using raw pointers and references in any local or heap stored variable. Only cases where raw pointers and references are permitted are function arguments — assuming every local variable / variable in stack is stored in a smart pointer, function arguments are safe to be raw pointers / references via transitive property. See Dangerous Use of Smart Pointers <https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html> for a reference. So, to recap, if we wanted shared ownership, or multiple pieces of code have to keep an object alive, use Ref <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/Ref.h> / RefPtr <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/RefPtr.h> with RefCounted <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/RefCounted.h> or ThreadSafeRefCounted <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/ThreadSafeRefCoun…> whichever is appropriate. To store a weak reference to an object, use WeakPtr <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/WeakPtr.h> or ThreadSafeWeakPtr <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/ThreadSafeWeakPtr…>, whichever is appropriate. When weak semantics is not needed (i.e. we don’t need to clear the pointer when the object goes away) for an external reference, use CheckedRef <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/CheckedRef.h> / CheckedPtr <https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/CheckedPtr.h> so that we can release-assert when an object is about to get destroyed with outstanding external references. I suppose we can consider exceptions to JSCell and other objects managed by GC but that should be more of an exception than norm. We should stop writing unsafe code going forward. Any thoughts? - R. Niwa

6 21

Unsigned to avoid negative values
by Myles Maxfield 27 Jan '23

27 Jan '23

Hello! I recently learned that the C++ core guidelines recommend against using unsigned to avoid negative values. Section 4.4 on page 73 of The C++ Programming Language says unsigned types should be used for bitfields and not in an attempt to ensure values are positive. Some talks by people on the C++ standards committee (e.g., Herb Sutter) recommend against using unsigned types simply because the value is expected to by positive. Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all over the place, and I’m assuming a fair many of them are there to indicate that negative values are avoided. The C++ recommendation goes against my intuition that the type is there for clarity, to indicate expectations about the meaning and behavior of its value. But if it’s standard practice to just use int instead, perhaps we should update the style guide? What do you think? —Myles

8 12

Compile times and class-scoped enums
by Jer Noble 23 Jan '23

23 Jan '23

Hi all! I’ve noticed that compile times for the WebKit project have started creeping up again, so I and a few other WebKit contributors started looking into possible compile time improvements using the tools listed in <https://trac.webkit.org/wiki/AnalyzingBuildPerformance>. One cause of long compile times is when a commonly-included header (like Document.h) includes other headers (which include other headers, ad nauseam). Whereas much of the time those includes can be avoided by forward declaring types, for types (specifically enums) scoped within classes, this is impossible. I attempted to address this in <https://github.com/WebKit/WebKit/pull/8867>, which (on this machine) reduces the total compile time of Document.h in the WebCore project from about 1000s to about 200s. However, this change requires moving class-scoped enums out into the namespace scope. E.g.: > diff --git a/Source/WebCore/Modules/audiosession/DOMAudioSession.h b/Source/WebCore/Modules/audiosession/DOMAudioSession.h > index 01bf6960d3a4..d84e1eae78d5 100644 > --- a/Source/WebCore/Modules/audiosession/DOMAudioSession.h > +++ b/Source/WebCore/Modules/audiosession/DOMAudioSession.h > @@ -36,14 +36,17 @@ > > namespace WebCore { > > +enum class DOMAudioSessionType : uint8_t { Auto, Playback, Transient, TransientSolo, Ambient, PlayAndRecord }; > +enum class DOMAudioSessionState : uint8_t { Inactive, Active, Interrupted }; > + > class DOMAudioSession final : public RefCounted<DOMAudioSession>, public ActiveDOMObject, public EventTarget, public AudioSession::InterruptionObserver { > WTF_MAKE_ISO_ALLOCATED(DOMAudioSession); > public: > static Ref<DOMAudioSession> create(ScriptExecutionContext*); > ~DOMAudioSession(); > > - enum class Type : uint8_t { Auto, Playback, Transient, TransientSolo, Ambient, PlayAndRecord }; > - enum class State : uint8_t { Inactive, Active, Interrupted }; > + using Type = DOMAudioSessionType; > + using State = DOMAudioSessionState; > > ExceptionOr<void> setType(Type); > Type type() const; So that these enums can be forward declared in Document.h, rather than including the header wholesale. However, this requires a significant coding style change, both to existing code and new code, and as such, it should probably be discussed here. So, what do people think? Is the change in coding style (moving class-scoped enums out into namespace scope) worth doing if it means a significant increases in compile speeds? -Jer

4 4

New status field required for feature flags
by Elliott Williams 13 Jan '23

13 Jan '23

Hi webkit-dev, Brent (CC’d) and I are rolling out changes to how we represent feature flags in WebKit. As of https://commits.webkit.org/258448@main, the WebPreferences*.yaml files have been merged to one feature list: Source/WTF/Preferences/UnifiedWebPreferences.yaml Instead of having separate lists of experimental features, internal debug features, etc., each feature now has a `status` field. A feature’s status determines which UIs it is visible in (if any), and in the future will control its default value. Right now, these are the semantic meanings of the features status: embedder Adjust WebKit behavior for embedding application needs. unstable Feature in active development. Unfinished, no promise it is usable or safe. internal Tools for debugging the WebKit engine. Not generally useful to web developers. developer Tools for web developers. testable Enabled by default in test infrastructure, but not ready to ship yet. Should be accessible for STP users who wish to activate them for testing and feedback. preview Enabled by default in Safari Technology Preview, but not considered ready to ship yet. stable Enabled by default and ready for general use, but UI to toggle the feature remains to support debugging, A/B testing. shipping Enabled by default and ready for general use, but no UI to toggle. In general, will be removed 1 year after the change to shipping. When you’re adding or changing features, our source generator will force you to pick one of these status. The “public-facing” statuses (developer, testable, preview, stable) are visible from the various “experimental features” call sites declared by WebKit, and the rest are exposed as “internal debug features”. And I’m adding an additional WebPreferences API which lists all features, so clients can avoid the experimental/internal-debug dichotomy going forward. There’s one topic I’d like feedback on, which pertains to the non-Apple ports: Some features don’t seem to have the same “status” on all platforms. For example, pdf.js has been enabled for GTK and WPE for some time now, and should probably be considered `stable` on those platforms. But it’s off by default on Apple platforms, where it should be no more than `testable`. How do we resolve discrepancies like these? We could: - allow a feature’s status to be platform-conditional - allow each port to decide whether a feature is on or off by default (regardless of its status) - do nothing, and require clients to change feature statuses at runtime in order to modify default behavior I’d like to eventually use the committed feature list to power https://webkit.org/status/, which makes me want to avoid having platform-conditional statuses. It would also be nice for feature statuses to be a global truth, helping us speak collectively about what web platform features are supported. Elliott

3 3

Oriol Brufau is now a reviewer!
by Tim Nguyen 10 Jan '23

10 Jan '23

Hi! I’m happy to announce that Oriol Brufau is now a WebKit reviewer. 🎉 Oriol has shown great expertise on CSS cascade and serialization related things. Please feel free to reach out to Oriol for reviews! Cheers, Tim

1 0