November 2021 - 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

Request for Position: Multi-Screen Window Placement API
by Ben Morss 12 Apr '22

12 Apr '22

Hi all, We'd love to get WebKit's position on the new Multi-Screen Window Placement API. The proposal is to make incremental improvements to existing screen information and window placement APIs, allowing web applications to be intentional about the experiences they offer to users of multi-screen devices. We'd like to make it simple to: - detect when screen properties changes - discover the presence of extended screen areas - request extended screen information - place a window on a specific screen - show a window fullscreen on a specific screen This is useful for applications like the following: - a slideshow app shows slides full-screen on a projector, while displaying speaker notes on a laptop - a creativity app pops up secondary windows, like palettes, on a separate screen - a videoconferencing app shows video on a large screen, and controls on a smaller screen - a medical app shows images on multiple displays - a financial app shows dashboards on multiple monitors Future work could involve support for foldable devices, showing a window that spans multiple screens, etc. For more details, please see these links: - Explainer: https://github.com/webscreens/window-placement/blob/main/EXPLAINER.md - Draft Community Group Report: https://webscreens.github.io/window-placement/ - Additional ideas: https://github.com/webscreens/window-placement/blob/main/additional_explora… Thanks for your feedback on this! All best, Ben

1 1

Re: [webkit-dev] Request For Position on CSS containment
by cathiechen 24 Dec '21

24 Dec '21

Hi webkit-dev, We made a lot of progress regarding CSS containment [1]. Rob and I have finished the layout containment and size containment [2].\o/ And the patches of paint containment and style containment are ready for review now [3]. So we think now it's time to move on to content-visibility: (https://www.w3.org/TR/css-contain-2/#content-visibility) Since content-visibility depends on paint and style containment, we will do some specification research first, then prototype it based on Rob's patches. Thank you for your feedback! [1] https://bugs.webkit.org/show_bug.cgi?id=172026 [2] https://bugs.webkit.org/show_bug.cgi?id=223569, https://bugs.webkit.org/show_bug.cgi?id=223570 [3] https://bugs.webkit.org/show_bug.cgi?id=224742, https://bugs.webkit.org/show_bug.cgi?id=226458 Best, Rob and Cathie

3 5

Raw string literals
by Alex Christensen 29 Nov '21

29 Nov '21

Right now, our style checker disapproves of raw string literals, which were introduced in C++11. It complains with this message: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". https://webkit.org/code-style-guidelines/ <https://webkit.org/code-style-guidelines/> says nothing on the subject. I find them quite useful and nice, especially with strings that contain lots of quotation marks that would otherwise need escaping. Would anyone oppose to my changing our style checker to allow them if I ever get around to it?

3 2

Request for position: Markup based Client Hints delegation for third-party content
by Ari Chivukula 29 Nov '21

29 Nov '21

I'd like to get WebKit's position on Markup based Client Hints delegation for third-party content. https://groups.google.com/a/chromium.org/g/blink-dev/c/FTNrw03Xs9s/m/O74Mp6… https://docs.google.com/document/d/1U3P9yvaT1NXG_qRmY3Lp6Me7M5kTnd3QrBb1yFU… https://wicg.github.io/client-hints-infrastructure/#accept-ch-state-algo This is to support content negotiation use cases such as differential serving of variable fonts, color vector fonts, responsive images, and other third-party content which requires client information lost by user agent reduction (an ongoing project). For example: variable fonts allow significantly less font information to be transferred without loss of functionality, but only works on specific operating systems. It’s already possible to set a Permissions Policy in the HTTP response header, but for sites without the ability to modify HTTP headers a HTML solution would be ideal. This proposes a meta tag which allows delegation of client hints to third-party origins. These tags could be included in code-snippets for embedded third-party content for ease of use. For example, to specify third party requests to https://foo.bar must include sec-ch-width you could include: <meta name="accept-ch" content="sec-ch-width=('self' 'https://foo.bar')"> You may still omit the permission policy and rely on the default allowlist as follows: <meta name="accept-ch" content="sec-ch-width"> Note that this is the equivalent of the following today: <meta http-equiv="accept-ch" content="sec-ch-width"> ~ Ari Chivukula (Their/There/They're)

1 0

WPT first test policy proposal
by Tim Nguyen 23 Nov '21

23 Nov '21

Hello everyone, I would like to start a discussion on a policy to enforce WPT usage as a first choice, that would be enforced via check-webkit-style on Changelog files. Why use WPT? Contributing to WPT has many benefits: interoperability/compatibility issues with WPT we write may be detected by other browser vendors and we would get faster feedback and turnaround to fix them creates/encourages discussion in case of disagreement with other browser vendors WebKit greatly benefits from WPT coverage, it is time to provide our own coverage to other browsers Improves WebKit’s score generally for WPT (which has tests mostly contributed by Chromium, Firefox at the moment) Are there reasons to not use WPT? Common reasons for not writing WPT that have been mentioned are: "WPTs are less pleasant to write.” This is not true imo, the WPT harness is documented (https://web-platform-tests.org/writing-tests/index.html <https://web-platform-tests.org/writing-tests/index.html>), unlike WebKit internals, making it easier for new contributors to figure out things. “When you actually move a regression test to WPT proper, commit history is lost, and you don't know what kind of user facing problem it's preventing any more.” Use `<link rel=“help” href=“…”>` with a reference to the webkit bug. Ensure you actually export the WPT and merge the WPT PR as well! There are things WPT can’t test True (e.g. tree dumps). My proposal below covers that. Here is my current proposal: Every LayoutTests/ changelog adding new test files would contain: WPT-exemption-reason: NONE (OOPS!) If that line is removed or no valid reason is provided, webkit-style would throw out an error. Valid reasons could be: WPT-exemption-reason: urgency (webkit.org/b/XXX <http://webkit.org/b/XXX>) I’m personally not in favour of this reason existing, but some have expressed interest in it, this may be used in case of urgency (security fixes etc.), where a bug must be filed to port it to WPT (alternatively the test path could be added to a new allowlist file). Eventually this reason would be removed if two way sync with the WPT repo is available. WPT-exemption-reason: test-harness-deficiency (webkit.org/b/XXX <http://webkit.org/b/XXX>) If the WebKit implementation of the WPT harness lacks a certain feature that should be implemented in the WPT harness, a new bug may be filed, or an existing bug may be referenced. Maintainers of the WPT harness may watch this reason to prioritize work needed on the harness. WPT-exemption-reason: requires-internals: NONE (OOPS!) requires-internals should have a valid explanation of which internals are needed. (Tree dumps would be a valid reason, although usually reftests can cover those) In addition: WPT-PR: https://github.com/web-platform-tests/wpt/pulls/XX <https://github.com/web-platform-tests/wpt/pulls/XX> must be present in LayoutTests/imported/w3c/Changelog, this is to ensure tests are not lost at the next import. This would also be enforced in webkit-style, although eventually this might be replaced by a fully automated process in the future similar to Chrome/Firefox. Any feedback is absolutely welcome! Cheers, Tim

3 2

Running the GTK port on macOS with Docker
by Philippe Normand 23 Nov '21

23 Nov '21

Hi, The WPE/GTK ports nowadays rely on a SDK that provides all the tools needed for development, it's used on the bots as well. Currently we run it with Flatpak, but technically, Docker can also be used. I've actually checked that a GTK MiniBrowser build downloaded from the bots can run on macOS with Docker, that involves setting up XQuartz, it's not great, but for quick testing, I wonder if that could be useful for the Apple folks? The GTK port could also be built on macOS using this docker setup, the SDK includes the GCC and clang versions used on the bots. You can already do this with a VM, but then if you want to use the SDK it adds another lever of virtualization, which is not great. So directly using the SDK through Docker seems more appealing, to me at least :) WPE can't run, I haven't managed to start Weston with its X11 backend in XQuartz... Ideally I'd prefer a native Wayland compositor for macOS but that doesn't seem to exist yet. Anyway, please let me know if some folks are interested. Philippe

2 4