June 2022 - webkit-dev - lists.webkit.org

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

Deployment of new EWS Non-Unified builder
by dpino 08 Sep '22

08 Sep '22

Hi, Last year we started a thread to discuss the possibility of deploying a new EWS bot that builds WebKit with Non-Unified sources [1]. This thread explains the technical reasons why a non-unified build bot is desirable. If you're aware of the problems introduced by unified sources compilation, skip the next paragraph. Unified sources compilation consists of merging several source files together into one big file to improve building times. Usually the same sources files are grouped together, but compilation flags or downstream changes can create different source file groups. When that happens, you may hit a build error. Since unified sources group files together, missing headers in one file can be satisfied by another file in the same group. What's happening in WebKit development currently is that source code that breaks non-unified compilation frequently lands without even being noticed. The breaks are usually related to missing headers. Embedders that need to support WebKit builds with different flags or maintain downstream changes are more likely to hit these compilation errors. Last year's attempt to deploy an EWS Non-Unified builder ended up in a deployment of a post-commit bot plus two workers running in the UAT. It was actually hard to get the Non-Unified builder working [2], something that we achieved at the beginning of this year (kudos to Adrián and Lauro). Since then we have been maintaining the post-commit bot very closely. There are periods of time where the bot has remained green for a long period of time. But lately maintaining the bot green has become harder and harder, specially due to the big refactorizations that have happened in WebKit source code lately. The bot very rarely stays green longer than 2 or 3 days without close maintenance. We believe that we all should share the responsibility of keeping the non-unified build working, and therefore we want to proceed with the deployment of a EWS Non-Unified builder. Initially, we'll provide two workers on a modern host with 8 cores assigned each. We have found this setup faster for most patches than many of the existing EWS queues, as it only runs a build in non-unified mode, without test or upload steps. If this were not fast enough, we would consider increasing the number of workers in the queue. We set up a page[3] with a rough comparison to the regular (unified) bot and the build times are pretty similar. Diego [1] https://www.mail-archive.com/webkit-dev@lists.webkit.org/msg30077.html [2] https://bugs.webkit.org/show_bug.cgi?id=226088 [3] https://people.igalia.com/lmoura/webkit-bots-dashboard/unified.html

14 34

Request for Position: MSE-in-Workers
by Matt Wolenetz 27 Jul '22

27 Jul '22

Hi webkit-dev, This is a request for Webkit's position on the MSE-in-Workers feature: Explainer: https://github.com/wicg/media-source/blob/mse-in-workers-using-handle/mse-i… Main MSE spec issue tracking this feature: https://github.com/w3c/media-source/issues/175 ChromeStatus entry: https://chromestatus.com/features/5177263249162240 Draft specification: https://github.com/w3c/media-source/pull/282 TAG review: https://github.com/w3ctag/design-reviews/issues/656 Demo page (Chromium has experimental implementation): https://wolenetz.github.io/mse-in-workers-demo/mse-in-workers-demo.html Please let us know if you have any feedback! Thanks, Matt

1 1

Request for position: MediaTrackSupportedConstraints.backgroundBlur
by Bhaumik, Rijubrata 05 Jul '22

05 Jul '22

This is a request for WebKit's position on adding support for Background Blur API on MediaStreamTrack. The PR that introduced this behavior to the medicapture-extensions spec and the explainer was co-authored by Apple engineer Youenn Fablet and approved by Jan-Ivar Bruaroey (Mozilla). Explainer https://github.com/riju/backgroundBlur/blob/main/explainer.md Specification https://w3c.github.io/mediacapture-extensions/#exposing-mediastreamtrack-so… Link to entry on the Chrome Platform Status https://chromestatus.com/feature/5077577782263808 WebRTC meeting minutes: https://www.w3.org/2022/05/17-webrtc-minutes.html#t07 Thanks, Riju.

2 1

Request for Position: User Preference Media Features Client Hints Headers
by Thomas Steiner 29 Jun '22

29 Jun '22

Hi WebKittens, We are interested in hearing your thoughts on User Preference Media Features Client Hints Headers, a set of HTTP Client Hints headers around user preference media features as defined by Media Queries Level 5. Specification Title: User Preference Media Features Client Hints Headers Specification or proposal URL: https://tomayac.github.io/user-preference-media-features-headers/ Explainer URL: https://github.com/tomayac/user-preference-media-features-headers/blob/main… TAG review: https://github.com/w3ctag/design-reviews/issues/632 Mozilla Standards Position: https://github.com/mozilla/standards-positions/issues/526 Thank you very much for your time and consideration! Cheers, Tom

1 5

new standards-positions repo
by Theresa O'Connor 28 Jun '22

28 Jun '22

Hi all, At a contributors' meeting a while ago I proposed that we use a GitHub repository, much like Mozilla does, for folks to solicit standards positions from us. Now that the WebKit repo itself is on GitHub (great work, everybody!), our standards positions repo is up too: https://github.com/WebKit/standards-positions Whenever you encounter a spec or spec issue you think the WebKit community should come to consensus on, please file an issue there. Thanks, Tess

1 0

WebKit is now on GitHub
by Jonathan Bedard 27 Jun '22

27 Jun '22

r295779 is the last commit to WebKit’s Subversion repository. https://github.com/WebKit/WebKit is now the canonical home of the WebKit project! All commits must now go through Commit-Queue, Merge-Queue or Unsafe-Merge-Queue. For the next few weeks, svn.webkit.org and trac.webkit.org will remain accessible in a read-only state so we can audit any content we intend to migrate to GitHub. Let me know if there is any fallout, Jonathan Bedard WebKit Continuous Integration

4 5

Request for Position: CSS overflow for replaced elements
by Khushal Sagar 24 Jun '22

24 Jun '22

Hello WebKit! This is a follow up to previous RFP <https://lists.webkit.org/pipermail/webkit-dev/2022-March/032166.html> which introduced the object-overflow property to allow replaced elements to paint with ink overflow. Based on feedback in the CSSWG, the proposal has been updated to use the existing `overflow` property to provide the same functionality. The issue capturing the discussion is here <https://github.com/w3c/csswg-drafts/issues/7144>. We'd appreciate your feedback on the updated proposal. Thanks. Khushal Sagar Google Chrome team

1 0

Request for Position: Gamepad API Trigger-Rumble Haptics Extension
by Gabriel Brito 21 Jun '22

21 Jun '22

Hello web-kit devs, We would like to get the Webkit's position on an extension to the Gamepad API to support trigger-rumble haptic effects on the Web. Chromium is already implementing it. You can find more details in the links below: * Explainer: MSEdgeExplainers/explainer.md at main * MicrosoftEdge/MSEdgeExplainers (github.com)<https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/GamepadHapticsA….> * Chrome Status: Gamepad API Trigger-Rumble Extension - Chrome Platform Status (chromestatus.com)<https://chromestatus.com/feature/5162940951953408> Thank you, Gabriel

1 0

7 Days Until Retiring Subversion
by Jonathan Bedard 20 Jun '22

20 Jun '22

Hey folks, The time has come! On Thursday, June 23rd, we are planning on stopping all commits to the Subversion repository hosted on https://svn.webkit.org <https://svn.webkit.org/>, all future commits will at that point be made on https://github.com/WebKit/WebKit <https://github.com/WebKit/WebKit>. If the following is true about your development workflow: - You use github.com/WebKit/WebKit <http://github.com/WebKit/WebKit> as the remote for your git checkout of WebKit - You commit with Commit-Queue, Merge-Queue or Unsafe-Merge-Queue Then you do not need to change anything about your workflow. Changes which are being processed during the transition will be routed to the correct place by Commit and Merge Queue. I would like to specifically note that `webkit-patch` and the patch workflow in general will continue to work after Subversion is frozen. You can check which remote your checkout uses with ‘git remote -v’. Please see the migration guide below for details. If you are still reliant off of git.webkit.org <http://git.webkit.org/> remotes (either WebKit-https.git or WebKit.git), or if you still have a checkout based on https://svn.webkit.org <https://svn.webkit.org/>, it’s time to migrate those checkouts. See https://github.com/WebKit/WebKit/wiki/Migration <https://github.com/WebKit/WebKit/wiki/Migration> for details on how to migrate from an existing checkout or https://github.com/WebKit/WebKit/wiki/Contributing#checking-out-WebKit <https://github.com/WebKit/WebKit/wiki/Contributing#checking-out-WebKit> for starting fresh. We intend to keep our defunct remotes available for a few weeks to ease the transitions, but starting Thursday, these 3 remotes will not be receiving new commits. Please reach out if you have any issues, Jonathan Bedard WebKit Continuous Integration

2 1