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

Request for position on COEP reflection
by Arthur Sonzogni 01 Jun '22

01 Jun '22

Greetings webkit-dev, I am requesting your feedback about exposing: ```js self.crossOriginEmbedderPolicy; ``` It reflects the environment's crossOriginEmbedderPolicy's value. More details on: https://github.com/ArthurSonzogni/coep-reflection Thanks Arthur @arthursonzogni

2 1

안광림 is out of office.
by 안광림 31 May '22

31 May '22

Period : 2022/05/23 00:00 ~ 2022/07/15 00:00 [Korea Standard Time] 부재중입니다.

1 0

ChangeLog Deprecation in WebKit
by Jonathan Bedard 31 May '22

31 May '22

Starting tomorrow, Tuesday May 17th, WebKit will no longer be using ChangeLogs and will instead require commit messages in new commits. These were the norm when we started the project, but now they are unusual, antiquated, and get in the way of common git tooling. This means that we need a commit message in every patch or PR that's submitted for review. From prior discussion, it is very clear that there are legitimate downsides too, and I want to take this opportunity to suggest recommended paths forward. You will need to take action if you: - Have a pure-Subversion checkout - Draft patches without making local commits - Draft patches from multiple local commits - Have patches with ChangeLogs already in bugzilla - Have pull requests with ChangeLogs https://github.com/WebKit/WebKit/wiki/Migration <https://github.com/WebKit/WebKit/wiki/Migration> covers migration to GitHub workflows in general, but Tuesday’s change is quite a bit more narrow in its effects. Below I’ve listed the actions you may need to take - Have a pure-Subversion checkout Follow https://github.com/WebKit/WebKit/wiki/Migration#subversion <https://github.com/WebKit/WebKit/wiki/Migration#subversion> to clone github.com/WebKit/WebKit <http://github.com/WebKit/WebKit> and start developing from a GitHub checkout. The patch workflow works just as well from a GitHub checkout. One significant difference is that it's not possible to commit directly, but this is going away soon as GitHub becomes the source of truth, so it is probably not worth migrating to git-svn for. Also, some people find git-svn handy to convert between revision formats; we have `git webkit find` as a replacement. - Draft patches without making local commits Run `git-webkit setup` to configure your local git hooks to provide a WebKit commit-message template. Before running `webkit-patch upload`, run `git commit -a` to create a local commit with all local changes. To amend that commit, run `git commit -a —amend`. - Draft patches from multiple local commits EWS will test patches with multiple commits in them, but Commit-Queue will not land those patches. If you have a multi-commit patch (or branch) applied locally, you need to squash them. I’d recommend `git rebase -i <base>`. To do this, run `git rebase -i HEAD~3`, git will then open your editor and offer to `pick` the last 3 commits. Keep `pick` for the first listed commit, but use `squash` for the other two and close your editor, the last 3 commits will be squashed into a single commit. - Have patches with ChangeLogs Apply your patch with `webkit-patch apply-from-bug <bug-number>`. If we’ve already removed the ChangeLog files from the repository, this patch application will fail. Resolve the conflicts, and commit your local changes and draft a commit message. If we haven’t yet removed the ChangeLog files, your patch should apply. In that case, make sure you’ve run `git-webkit setup` and commit your local changes include the ChangeLog. This will create a commit message derived from your ChangeLogs. Then, remove the ChangeLog edits locally, amend your previous commit with `git commit -a —amend` and re-upload your ChangeLog free patch. - Have pull requests with ChangeLogs Check out your pull request, remove any ChangeLog edits locally and run `git-webkit pr --amend`. Jonathan WebKit Continuous Integration

2 1

안광림 is out of office.
by 안광림 27 May '22

27 May '22

Period : 2022/05/23 00:00 ~ 2022/07/15 00:00 [Korea Standard Time] 부재중입니다.

1 0

Request for position: Response.json() static method
by Adam Rice 27 May '22

27 May '22

The Response.json() static method permits constructing a Response object with the body populated with a JavaScript object serialized as JSON. Links: - Explainer: https://docs.google.com/document/d/1dTycWmyxLZNGTBW93fvtf1IQahx-vNwgu94yT1x… - Spec: https://fetch.spec.whatwg.org/#ref-for-dom-response-json - ChromeStatus entry: https://chromestatus.com/feature/5197912798658560

2 1

Request for position: MediaTrackConstraintSet.displaySurface
by Elad Alon 25 May '22

25 May '22

This is a request for WebKit's position on extending getDisplayMedia to also accept MediaTrackConstraintSet.displaySurface. The PR that introduced this behavior to the getDisplayMedia spec was approved by Apple engineer Youenn Fablet. Links: * Explainer: https://docs.google.com/document/d/1uI51R4YfFQtfiDTw1KfnLqzIu45OgxhYoUT7khl… * Spec: https://github.com/w3c/mediacapture-screen-share/pull/186/files * ChromeStatus entry: https://chromestatus.com/feature/5186392840732672

1 0