April 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

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 feedback on CompressionStream and DecompressionStream
by Adam Rice 02 May '22

02 May '22

I am trying to gauge feedback on compression streams with a view to shipping them in Chromium. Very briefly, they are a way to do gzip and gunzip in the browser. Less briefly, the explainer https://github.com/WICG/compression/blob/master/explainer.md goes into some detail of the how and why. The specification https://wicg.github.io/compression/ gives verbose detail of how. You may also find the W3C TAG review https://github.com/w3ctag/design-reviews/issues/410 interesting. Any and all feedback is welcome! FAQ: Q. Will this support brotli, zstd, etc.? A. Not yet. It's on the radar. Thanks, Adam Rice, Chromium

3 3

webkit-patch land behavior change
by Jonathan Bedard 27 Apr '22

27 Apr '22

Hey folks, As we move closer to transitioning away from Subversion, I’ve change ‘webkit-patch land’ to use commit-queue instead of directly committing a local change from a contributor’s machine (https://github.com/WebKit/WebKit/pull/392) ‘git-webkit land-unsafe’ will allow contributors to directly land to Subversion for the time being, although after we transition to GitHub, will also use commit-queue and prefix “fast-cq” to uploaded patches to bypass building and testing. Jonathan

2 2

Re: [webkit-dev] ChangeLog Deprecation Plan
by Jonathan Bedard 25 Apr '22

25 Apr '22

> It seems to me that you can write a script that just applies a patch while > ignoring changes to each change log file, then separately prepend each > change log entry. I don't see why such a script would behave differently > for reverting multiple commits or rebasing a feature onto a branch. In > those cases where codebase has enough divergence, merge conflicts are > probably more of an issue with the codebase itself, and not change log > entries. There are many types of changes which are not compatible with patches. “Write a change which applies a patch” is not a general solution. It wasn’t a general solution in Subversion, and it’s not a general solution in git. Speaking as the person who has to maintain our patch workflows, there are many edge cases to patch workflows that tend to bite you with precisely the kind of large changes which are difficult to manually sift through. Patch workflows also represent a huge risk for Apple’s software update branches where changes are cherry-picked by engineers who aren’t familiar with the technical details of the changes their cherry-picking. These workflows have already been using git for years at this point because patches do not work reliably for this type of work. > .. or robust solution. Bots need maintenance and intervention, and a bot with > commit access has another set of issues. Repository admins occasionally > rotating ChangeLogs is going to be less expensive than a bot doing it. > > Either way, it doesn't seem like this is a higher cost issue compared to > all WebKit contributors having to change their workflows. We are moving to git. Workflows _are_ changing. I’m interested in making sure the workflows can achieve the same things the old ones did, not in preserving the old workflows. > > folks familiar with git won?t realize what has happened until they see > their PR diff), ?mandatory? is a bit tougher because we can?t prevent folks > from filing a pull request without using tooling. At that point, we?re > where we are today with PR template that encourages tool usage, explains > how to craft a PR the ways tools do it and then reviewers acting as a > gate-keeper. The last point is more about project policy than it is tooling. > > We should automatically reject such PR requests. We should not automatically reject PRs made in good faith. One of the reasons we’re moving to GitHub is to make the project more approachable for new contributors. Having a machine rejects PRs because a contributor is not familiar with project norms goes against this goal. > … judgments about personal preferences for local development. From my > discussions with folks, it seems that the project is pretty evenly split on > --amend commits, with some contributors preferring them and others > preferring to make multiple commits. My purpose in bringing up --amend > commits is that one of the things I?ve heard folks find useful about > ChangeLogs is the ability to iteratively build them, which is something > that does have a parallel in raw git workflows. > > No, my option isn't between amending a commit vs. making multiple commits. > I really don't like making *any local commits* at all. I want to work on a > patch without ever making local commits, and upload whatever change I may > have relative to the main branch's HEAD is. Our tooling supports and will continue to support workflows where the tooling automatically makes commits for you. Discussions of other workflows is important because we don’t want to build extra tooling for the project if there is already a native git workflow which address a specific concern.

2 1

ChangeLog Deprecation Plans
by Jonathan Bedard 22 Apr '22

22 Apr '22

As we migrate WebKit from Subversion to git, I would like to migrate the project away from ChangeLogs. The reason for this is that ChangeLogs make some of the features of git hard to use, namely, cherry-picking commits between branches requires conflict resolution every time. Rotating ChangeLogs is also moderately difficult in git with locked down commit access like our project has, only repository administers would, in practice, be able to rotate ChangeLogs. Lastly, ChangeLogs are uncommon in git based projects, so new contributors will find them difficult to manage. Currently, our git tooling makes very little effort to either support ChangeLogs or to provide alternatives. I have listed bellow some of the reasons I understand folks to like ChangeLogs along with possible git-based solutions, if necessary. 1) Subversion commit messages are stored server side, local development needs a copy git doesn’t have this problem. We have a local record of commit messages in every checkout. 2) We need a way to comment on commit messages in review Current tooling sets the pull request description as the commit message, “Quote Reply” kind of provides a way to inline comment, although it’s not the formal review UI Proposal: Tooling should support a “COMMIT_MESSAGE” file in each pull request commit that becomes COMMIT_MESSAGE when a pull request is landed 3) Edit commit messages while creating a change, not just when committing the change The “overwrite” workflow already sort of support this idea by using amend commits instead of appending commits to an existing branch Proposal: The above “COMMIT_MESSAGE” file workflow would allow iterative building of a commit message before committing 4) Using git commands to view commit messages is hard There don’t seem to be many projects which have a solution for this. In practice, it seems that reducing the scope of messages shown by restricting history to a specific directory or even file is one solution, another is shorter commit messages Proposal: Have Tools/Scripts/git-webkit setup configure hooks in contributors local git repositories to lay down CommitMessages.history files on merge, checkout and commit which contain the last 5000 commit messages. We can put these in similar places to where ChangeLogs are today, although we would likely want them in fewer places because this will increase local compute time on many git operations. We could also make this a configurable setting so that engineers who are more comfortable with the raw command line tooling do not have to deal with slower git operations. I like the COMMIT_MESSAGE and hooks proposals because they are opt-in. Contributors who wish to use native git tooling to contribute and interact with the project do not have to use either tool, but the tools are compatible enough with native git workflows that contributors who find editing and viewing commit messages primarily in a text editor Looking forward to the discussion, Jonathan Bedard WebKit Continuous Integration

8 11

Request for position on Anonymous iframe
by Arthur Sonzogni 20 Apr '22

20 Apr '22

Greetings webkit-dev, We have been working on Anonymous iframe, and we would appreciate your feedback. This is an attribute to the <iframe> element causing its document to be associated with a new and ephemeral context. It resolves a problem developers have using Cross-Origin-Embedder-Policy (COEP). It allows embedding <iframe> that do not use COEP. To achieve this, we rely on adding a nonce to the storage key used to access shared storage by anonymous iframe. As part of the client-side storage partitioning effort (declined across storage APIs, network state and Cookie State), See https://github.com/ArthurSonzogni/anonymous-iframe Thanks Arthur @arthursonzogni

1 0

Re: [webkit-dev] ChangeLog Deprecation Plan
by Jonathan Bedard 20 Apr '22

20 Apr '22

> Message: 2 > Date: Mon, 18 Apr 2022 10:33:03 -0700 > From: Ryosuke Niwa <rniwa(a)webkit.org> > To: Jonathan Bedard <jbedard(a)apple.com> > Cc: WebKit Development <webkit-dev(a)lists.webkit.org> > Subject: Re: [webkit-dev] ChangeLog Deprecation Plans > Message-ID: > <CABNRm613onu_8wjXEuhJvOH7vztc3T9r_YLNqgOJ5if5AS04+Q(a)mail.gmail.com> > Content-Type: text/plain; charset="utf-8" > > On Mon, Apr 18, 2022 at 8:30 AM Jonathan Bedard via webkit-dev < > webkit-dev(a)lists.webkit.org> wrote: > >> As we migrate WebKit from Subversion to git, I would like to migrate the >> project away from ChangeLogs. The reason for this is that ChangeLogs make >> some of the features of git hard to use, namely, cherry-picking commits >> between branches requires conflict resolution every time. >> > > Isn't this something that can be easily resolved with a merge script? For simple cases, yes. But with something like a multi-commit revert or rebasing a feature onto a different branch, merge scripts stop working pretty quickly. It’s usually, It might be possible to write a merge script to handle these cases, but it’s by no means “easy” in the general case. It’s also worth noting that the cases that are more complicated also tend to be the ones with the most urgency attached to them. > > Rotating ChangeLogs is also moderately difficult in git with locked down >> commit access like our project has, only repository administers would, in >> practice, be able to rotate ChangeLogs. >> > > It seems like we can just automate this by introducing a change log > rotating bot, which has the same privilege as commit queue. Anything that starts with “we can just have a bot which…” is not a simple or robust solution. Bots need maintenance and intervention, and a bot with commit access has another set of issues. Repository admins occasionally rotating ChangeLogs is going to be less expensive than a bot doing it. > > So these two arguments seem rather weak. > > Lastly, ChangeLogs are uncommon in git based projects, so new contributors >> will find them difficult to manage. >> > > This might be the strongest argument in favor of deprecating change logs. > > 2) We need a way to comment on commit messages in review >> Current tooling sets the pull request description as the commit message, >> ?Quote Reply? kind of provides a way to inline comment, although it?s not >> the formal review UI >> *Proposal*: Tooling should support a ?COMMIT_MESSAGE? file in each pull >> request commit that becomes COMMIT_MESSAGE when a pull request is landed >> > > This needs to be a mandatory / automatic system, not opt-in. I want to > comment on commit messages as a reviewer. As a patch author, I don't care > whether it can be easily commented on or not. > > But if this is a required thing, then new contributors would have to learn > that this file is auto-generated or needs to be edited manually in some > cases so getting rid of change logs may not necessarily reduce the > cognitive load in comparison to keeping the status quo (i.e. keeping change > log files). We can make it automatic for folks using tooling (and in such a way that folks familiar with git won’t realize what has happened until they see their PR diff), “mandatory” is a bit tougher because we can’t prevent folks from filing a pull request without using tooling. At that point, we’re where we are today with PR template that encourages tool usage, explains how to craft a PR the ways tools do it and then reviewers acting as a gate-keeper. The last point is more about project policy than it is tooling. > > 3) Edit commit messages while creating a change, not just when committing >> the change >> The ?overwrite? workflow already sort of support this idea by using amend >> commits instead of appending commits to an existing branch >> *Proposal*: The above ?COMMIT_MESSAGE? file workflow would allow >> iterative building of a commit message before committing >> > > I absolutely despise --amend commits. It's the most annoying thing I have > to do whenever I'm creating patches in a git clone. I do not think it is the appropriate place for the WebKit project to make judgments about personal preferences for local development. From my discussions with folks, it seems that the project is pretty evenly split on --amend commits, with some contributors preferring them and others preferring to make multiple commits. My purpose in bringing up --amend commits is that one of the things I’ve heard folks find useful about ChangeLogs is the ability to iteratively build them, which is something that does have a parallel in raw git workflows. The COMMIT_MESSAGE approach, I believe, supports both --amend and amend workflows quite well. > > I like the COMMIT_MESSAGE and hooks proposals because they are opt-in. >> Contributors who wish to use native git tooling to contribute and interact >> with the project do not have to use either tool, but the tools are >> compatible enough with native git workflows that contributors who find >> editing and viewing commit messages primarily in a text editor >> > > I don't think COMMIT_MESSAGE can be opt-in for the aforementioned reasons. > > - R. Niwa

2 1

JSC broken on s390x
by Michael Catanzaro 19 Apr '22

19 Apr '22

Hi, I want to call attention to: https://bugs.webkit.org/show_bug.cgi?id=238956 It breaks JSC on s390x. If anybody is interested in this architecture, or cares about WebKitGTK updates in Fedora (this issue will be a blocker), any help would be much appreciated. :) I've also informed Red Hat that this architecture needs more help. Michael

2 2