April 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 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

New fast commit queue mode: fast-cq
by Aakash Jain 25 Oct '21

25 Oct '21

Hi Everyone, I am happy to inform you that I have added a fast-commit-queue mode to the Commit-Queue. In this mode Commit-Queue would land patches quickly (typically taking ~1 minute, e.g.: https://ews-build.webkit.org/#/builders/28/builds/10876) This mode skips building and testing. It basically does sanity check (like validating ChangeLog, Reviewer, Committer etc.) and commits the patch. This is intended to be used for scenarios where the patch is a build fix, urgent fix, quick typo/follow-up fix etc. To use this mode, you can use the --fast-cq parameter while uploading the patch using webkit-patch command. e.g.: 'webkit-patch upload --fast-cq'. For patches which are already uploaded to Bugzilla, you can rename the patch (by clicking on 'Details' button for the patch), and prefix patch name with [fast-cq], e.g.: '[fast-cq] Patch for landing'. Pro-tip: you can also rename the patch to include [fast-cq] prefix while marking the patch cq+, from patch 'Details' page, and this mode would work. Please let me know if you have any questions/feedback. Thanks Aakash

2 4

Request for position on Reporting API (now with structured headers!)
by Ian Clelland 02 Sep '21

02 Sep '21

Hi WebKit! In Chrome-land, we've been working on updating our implementation of the Reporting API to align with the spec changes made over the last year or so, which resolve a number of privacy issues, separating reports generated within the context of a single document from those which relate to overall network conditions, and isolating reports from unrelated documents. This also includes a new header, Reporting-Endpoints, whose value is not persisted between document loads. We're hoping to ship these updates sometime early in the new year, and would love to get the thoughts / opinions / feedback from WebKit developers. Thanks! Ian Some relevant links: * Explainer: https://github.com/w3c/reporting * Spec: https://w3c.github.io/reporting * Recently-opened TAG review: https://github.com/w3ctag/design-reviews/issues/585

1 1

Request for position: Cryptographically secure random UUIDs
by Benjamin Coe 27 Aug '21

27 Aug '21

Hello WebKit folks, This is a request for WebKit's position on introducing the JavaScript method randomUUID(), for generating RFC 4122 version 4 identifiers, to the crypto interface. - Explainer: https://github.com/WICG/uuid/blob/gh-pages/explainer.md - Specification: https://wicg.github.io/uuid/ - ChromeStatus: https://www.chromestatus.com/feature/5689159362543616 Look forward to feedback, -- Ben.

2 1

Request for position: CSS scrollbar-gutter
by Felipe Erias 15 Jul '21

15 Jul '21

Hi webkit-dev, This is a request for WebKit's position on the CSS "scrollbar-gutter" property. The spec status is Working Draft. This feature is already implemented in Chrome behind a flag. Spec: https://drafts.csswg.org/css-overflow-4/#scrollbar-gutter-property Explainer: https://github.com/felipeerias/scrollbar-gutter-explainer Existing WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=167335 Summary: The scrollbar-gutter property provides control over the presence of scrollbar gutters (the space which may be reserved to display a scrollbar). This gives Web authors more agency over how their layouts interact with the scrollbars provided by the browser, so they can e.g. prevent excessive layout changes as content expands while avoiding unwanted visuals when scrolling isn't needed. Thanks! Best. Felipe

3 7

Request for position: CSS spelling and grammar features
by dazabani 01 Jul '21

01 Jul '21

I’m seeking WebKit’s position on the following features in css-pseudo and css-text-decor: • ::spelling-error highlight pseudo-element [0] • ::grammar-error highlight pseudo-element [1] • text-decoration-line: spelling-error [2] • text-decoration-line: grammar-error [3] While these features span two specs, they might be worth considering together, because the new pseudo-elements depend on the new text- decoration-line values for UA stylesheet support (and to be most useful to authors). Note that there’s a bug with a WIP patch for the pseudo-elements [4]. [0] https://drafts.csswg.org/css-pseudo-4/#selectordef-spelling-error [1] https://drafts.csswg.org/css-pseudo-4/#selectordef-grammar-error [2] https://drafts.csswg.org/css-text-decor-4/#valdef-text-decoration-line-spel… [3] https://drafts.csswg.org/css-text-decor-4/#valdef-text-decoration-line-gram… [4] https://bugs.webkit.org/show_bug.cgi?id=175784 Cheers, Delan Azabani Igalia

2 1

Request for position on Cascade Layers
by Rune Lillesveen 22 Jun '21

22 Jun '21

Hello webkit-dev, I'm asking for WebKit's position on Cascade Layers drafted in CSS Cascade Level 5. We plan to start prototyping behind a flag in Chromium in Q1. Explainer: https://github.com/oddbird/css-sandbox/blob/main/src/layers/explainer.md Specification URL: https://drafts.csswg.org/css-cascade-5/#layering Request for TAG review: https://github.com/w3ctag/design-reviews/issues/597 Thanks, -- Rune Lillesveen

1 1

Re: [webkit-dev] Feedback on Blink's text fragment directive proposal
by David Bokan 09 Jun '21

09 Jun '21

[sending (again, sorry) from correct e-mail] I think Nick's replies <https://lists.webkit.org/pipermail/webkit-dev/2019-December/030998.html> mostly still apply, some updated answers to those questions. (1) We’re concerned about compatibility issues in a world where some > browsers support this but not all. Aware browsers will strip `:~:`, but > unaware browsers won’t. I saw that on the blink-dev ItS thread, it was > mentioned that at least one site (webmd.com) totally breaks if any > fragment ID is exposed to the page. This makes it difficult to create a > link that uses this feature but which is safe in all browsers: > - Since there is no feature detection mechanism, it’s hard for a webpage > to know whether it should issue such a link. It would have to be based on > UA string checks, which is regrettable. > - A link meant for a supporting browser can end up in a non-supporting > browser, at the very least by copy paste from the URL field, and perhaps > through other features to share a link. > We do have a feature detection <https://github.com/WICG/scroll-to-text-fragment/#feature-detection-and-futu…> mechanism for this. On the latter point, this is true but we think implementing fragment directive stripping (removing the part after and including `:~:`) is trivial even if the UA doesn't wish to implement the text-fragment feature. FWIW, we haven't seen or heard of another such example since. > (2) The portions of this Community Group report that monkey patch other > standards (HTML, URL and CSSOM View I think?) should be turned into PRs > against those specifications. Monkeypatching other specs is not a good way > to build specifications for the long term. > We still need support from another vendor to start merging the monkey patches into the real standards - if Apple's supportive I'd be happy to start on that immediately. > (3) Text fragment trumping a regular fragment ID seems a bit strange. The > more natural semantic would be that the text search starts at the fragment, > so if there are multiple matches it’s possible to scroll to a more specific > one. It’s not clear why the fragment is instead entirely ignored. > This was discussed in more detail in issue#75 <https://github.com/WICG/scroll-to-text-fragment/issues/75>; I agree with Nick's point that the disambiguation syntax is already specific enough that starting from a fragment isn't necessary. This also keeps us mostly-compatible with the TextQuoteSelector <https://www.w3.org/TR/annotation-model/#text-quote-selector> specified in WebAnnotations which I think may have benefits for interaction with annotation applications. On Mon, Aug 31, 2020 at 1:31 PM David Bokan <bokan(a)google.com> wrote: > I think Nick's replies > <https://lists.webkit.org/pipermail/webkit-dev/2019-December/030998.html> > mostly still apply, some updated answers to those questions. > > (1) We’re concerned about compatibility issues in a world where some >> browsers support this but not all. Aware browsers will strip `:~:`, but >> unaware browsers won’t. I saw that on the blink-dev ItS thread, it was >> mentioned that at least one site (webmd.com) totally breaks if any >> fragment ID is exposed to the page. This makes it difficult to create a >> link that uses this feature but which is safe in all browsers: >> - Since there is no feature detection mechanism, it’s hard for a webpage >> to know whether it should issue such a link. It would have to be based on >> UA string checks, which is regrettable. >> - A link meant for a supporting browser can end up in a non-supporting >> browser, at the very least by copy paste from the URL field, and perhaps >> through other features to share a link. >> > > We do have a feature detection > <https://github.com/WICG/scroll-to-text-fragment/#feature-detection-and-futu…> > mechanism for this. > > On the latter point, this is true but we think implementing fragment > directive stripping (removing the part after and including `:~:`) is > trivial even if the UA doesn't wish to implement the text-fragment feature. > FWIW, we haven't seen or heard of another such example since. > > >> (2) The portions of this Community Group report that monkey patch other >> standards (HTML, URL and CSSOM View I think?) should be turned into PRs >> against those specifications. Monkeypatching other specs is not a good way >> to build specifications for the long term. >> > > We still need support from another vendor to start merging the monkey > patches into the real standards - if Apple's supportive I'd be happy to > start on that immediately. > > >> (3) Text fragment trumping a regular fragment ID seems a bit strange. The >> more natural semantic would be that the text search starts at the fragment, >> so if there are multiple matches it’s possible to scroll to a more specific >> one. It’s not clear why the fragment is instead entirely ignored. >> > > This was discussed in more detail in issue#75 > <https://github.com/WICG/scroll-to-text-fragment/issues/75>; I agree with > Nick's point that the disambiguation syntax is already specific enough that > starting from a fragment isn't necessary. This also keeps us > mostly-compatible with the TextQuoteSelector > <https://www.w3.org/TR/annotation-model/#text-quote-selector> specified > in WebAnnotations which I think may have benefits for interaction with > annotation applications. > > On Mon, Aug 31, 2020 at 12:42 PM Darin Adler <darin(a)apple.com> wrote: > >> On Aug 31, 2020, at 9:33 AM, David Bokan <bokan(a)chromium.org> wrote: >> >> We've made lots of improvements to the spec, notably around issues raised >> in Mozilla's standards-position >> <https://github.com/mozilla/standards-positions/issues/194>. >> >> >> Do you think those improvements address Maciej’s concerns from last >> December? Since you didn’t say that explicitly I was wondering what your >> take on that was. >> >> — Darin >> >

6 18

New EWS Non-Unified builder
by dpino 04 May '21

04 May '21

Hi everyone, In Igalia we have been discussing the need of deploying a new builder which builds WebKit using non-unified sources, and we know that at least the folks at Sony are also in favor. One side effect of Unified Source building is that it hides compilation errors. The kinds of errors that usually get hidden by unified builds are missing headers inclusions and missing definitions of functions declared inline; the latter being tricky to debug because it results in mysterious linker errors. This is caused by unified builds stashing several .cpp files together for compilation, so the definitions and header inclusions done in one “leak” into the others. As for missing header inclusion errors, a source file might include a header definition as a co-lateral effect of being stashed together with another file that indeed includes the missing header. These hidden compilation errors may arise later at some point if unified source files are stashed together in a different manner. The current situation is requiring periodical maintenance. You can check build fixes commits due to unified source compilation with: $ git log --pretty=short --grep "Non-unified" Here are some examples: https://bugs.webkit.org/show_bug.cgi?id=222652 https://bugs.webkit.org/show_bug.cgi?id=222755 https://bugs.webkit.org/show_bug.cgi?id=221701 A new builder which builds WebKit with non-unified Source will highly help to improve this situation. Compilation errors will be detected as soon as possible and will save a lot of time not only for the developers who are currently doing this manual maintenance but for anyone who would like to build WebKit, and may stumble on compilation errors accidentally introduced due to unified sources. While correct compilation of the codebase can only be guaranteed with non-unified source builds, we do not propose switching the current EWS compilation builders to non-unified because it's slower and the EWS LayoutTests and API test bots use the products built by the EWS builders — we do not want to delay getting results from those. That's why we are proposing a new builder: it will run on parallel, resulting in no slowdown for the other EWS builders, which will keep using unified builds. How this new builder will impact developers? The EWS LayoutTest bots take at least 1 hour to complete a build. We think that as long as this new EWS Non-Unified builder is within that time budget, this new EWS wont' slow down development speed. Thoughts? Best regards, Diego

14 23