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

Remove the version labels in GitHub
by Darin Adler 21 Nov '22

21 Nov '22

Hi folks. I understand it can be super critical to understand what version someone was testing when reporting a WebKit bug. But I, at least, haven’t found the version field at bugs.webkit.org useful in the 20+ years I have been working on this project. Now we have that same noise in GitHub. All my pull requests have the label WebKit Nightly Build on them when I use the “git webkit pr” command to create them. And we have lots of labels with various version numbers, such as “Safari 8” and “417.x". Can we just get rid of all this? Is there anyone who’s finding these labels useful in GitHub? I suggest we remove these labels from the project. No objections to leaving the version field in bugs.webkit.org for the time being, but eventually I think we should probably get rid of that too. — Darin

5 8

Nicks in contributors.json
by Anne van Kesteren 26 Oct '22

26 Oct '22

Heya, Now that GitHub needs are addressed through a github field in contributors.json and WebKit moved from IRC to Slack, is there still a need for the nicks field? Based on a suggestion on Slack I'm thinking of removing it from https://webkit.org/team/ and I might as well clean up contributors.json at the same time. Kind regards, Anne

4 4

CMake EWS now use -Werror
by Michael Catanzaro 25 Oct '22

25 Oct '22

Hi developers, The Apple EWS bots have long used -Werror to make warnings fatal, discouraging inadvertently introducing new build warnings. But CMake EWS bots did not, until now. The new CMake behavior after https://commits.webkit.org/255961@main is: * By default, warnings are not fatal. * Warnings become fatal if you use the -DDEVELOPER_MODE=ON CMake flag. The build-webkit script always enables this. * An off switch exists, -DDEVELOPER_MODE_FATAL_WARNINGS=OFF, with a corresponding --no-fatal-warnings flag for build-webkit. Use this whenever you're bisecting, or when you're in a hurry and don't have time to deal with particular warnings. * All post-commit bots now build with --no-fatal-warnings to avoid losing test results to a compiler warning. The EWS does not, to discourage introduction of new warnings. Some common warnings to avoid: * -Wreturn-type is by far the most common GCC warning introduced into WebKit. This occurs when you expect a switch statement to always return, but forget to ensure that it really does when passed an invalid enum value. Clang does not complain about these, so only the bots using GCC will notice it. The normal solution is to add RELEASE_ASSERT_NOT_REACHED() to the bottom of the function. In especially hot functions, that might have performance impact, and you might need to write the code in an alternative way or use IGNORE_RETURN_TYPE_WARNINGS_BEGIN/END from Compiler.h. (I've considered disabling this warning due to how frequently we introduce code that trips it, but have left it because returning bogus data is very bad.) * -Wunused-parameter and -Wunused-variable warnings often occur around ENABLE() or USE() build guards. Use UNUSED_PARAM() and UNUSED_VARIABLE(), respectively, to suppress these. Note that Source/WebKit builds with -Wno-unused-parameter, but most of the rest of WebKit does not. * -Wredundant-move occurs when you write "return WTFMove(foo)" and the solution is to remove the WTFMove(). Clang only warns when the move blocks return value optimization, but GCC warns always. Of course there are plenty more warnings you might encounter, but those are the most common ones. If you find something you don't know how to deal with, don't hesitate to ask for help. False positives can be suppressed using IGNORE_WARNINGS_BEGIN() and IGNORE_WARNINGS_END() or one of the similar macros from Compiler.h. I believe the --no-fatal-warnings flag is not currently hooked up to anything on Apple ports. This would be good to improve. Michael

1 0

New `request-merge-queue` label
by Jonathan Bedard 25 Oct '22

25 Oct '22

Hey folks, There was a request to add an equivalent to Bugzilla's “cq?” in GitHub. This morning, I added the `request-merge-queue` label, https://github.com/WebKit/WebKit/labels?q=request-merge-queue. Since GitHub lets you filter PRs on labels (https://github.com/WebKit/WebKit/pulls?q=is%3Apr+is%3Aopen+label%3Arequest-…) this should allow non-committers to request committers to apply the `merge-queue` label. Jonathan WebKit Continuous Integration

1 0

RFC: Page Collections
by David Bokan 20 Oct '22

20 Oct '22

Hi WebKittens, A team of us in Chromium are thinking about a new linking primitive that would allow links to point to multiple URLs. To give an idea: sharing a link with a friend that, when clicked, opens a tab group with X pages (where X > 1). It's still early stages (so not looking for a standards-position), there's some ideas for how this might work but nothing's firm yet. I've put up an explainer <https://github.com/bokand/page-collection/> with our current thinking. Given that this affects linking, interop between browsers is top of mind so I'm reaching out early; is this an area someone in WebKit cares about? Does WebKit think these use cases are worth addressing? WDYT about some of the specifics? If someone in WebKit is interested we'd love to work together but would also appreciate any kind of informal feedback (here or as issues on the explainer are both fine). Thanks! David

1 0

Gardening our pull requests
by Darin Adler 17 Oct '22

17 Oct '22

Hi folks. I love to keep the WebKit project moving along, and one way that I personally like to contribute is helping get pull requests reviewed. I looked at the list of pull requests for the project, excluding closed requests, draft requests, and requests that have been reviewed: https://github.com/WebKit/WebKit/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse+re… There are 113 of those right now. It seems like we should convert many of these to drafts, close some others, and review most of the rest. I think it would make the project healthier if we didn’t have so many pull requests in an ambiguous state, including some that were last updated 4 months ago. What do you all think? Do you own some of these? Would you be willing to convert ones that represent work in progress to drafts to clean up this list? Other ideas for making this work well? — Darin

2 1

Request for position: Conditional Focus
by Elad Alon 13 Oct '22

13 Oct '22

This is a request for WebKit's position on introducing a new API called Conditional Focus. The API integrate with screen-capture (getDisplayMedia). It allows a capturing application to decide whether, once a display surface has been captured, that surface should be focused, or whether the capturing tab should retain focus. Links: * Explainer: https://github.com/WICG/conditional-focus/blob/main/README.md * Spec: * Generally, it's in: https://www.w3.org/TR/screen-capture/#dom-capturecontroller-setfocusbehavior * The two PRs that introduced this are: * https://github.com/w3c/mediacapture-screen-share/pull/235/files * https://github.com/w3c/mediacapture-screen-share/pull/240/files * ChromeStatus entry: https://chromestatus.com/feature/5646614535340032

1 1