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

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

Request for position on Import Conditions
by Daniel Clark 09 Mar '21

09 Mar '21

Hello WebKit-dev, I'd like to ask for WebKit's official position on import conditions, as described in the explainer at [1]. This proposal adds an inline syntax for module import statements to pass on more information alongside the module specifier. It includes support for JSON modules as a primary application, with the "type" condition used to mitigate the security issue described in [2] that previously blocked their adoption in HTML. The proposal is designed such that other conditions can be added in the future for additional use cases, some of which may not yet be anticipated. The champion group is particularly interested in your opinion on whether it's acceptable for import conditions to not be part of the module map key. The current state of our HTML spec integration PR at [3] is that they are not part of the module map key. We perform module map caching such that there will always be no more than a single fetch for a given module specifier regardless of failed module type checks. The thread at [4] discusses this issue in detail. We plan to request Stage 3 for this proposal at the upcoming TC39 meeting beginning on July 20. [1] Explainer: https://github.com/tc39/proposal-import-conditions [2] Modules MIME-type security issue: https://github.com/w3c/webcomponents/issues/839 [3] HTML Spec integration PR: https://github.com/whatwg/html/pull/5658 [4] HTML integration thread: https://github.com/whatwg/html/issues/5640 Mozilla standards-position ("worth prototyping") https://github.com/mozilla/standards-positions/issues/373 Thanks! Dan Clark

1 1

Position on emerging standard: Declarative Shadow DOM
by Mason Freed 24 Feb '21

24 Feb '21

Hello WebKit! I would like to request an official WebKit position on the Declarative Shadow DOM proposal. There have been some great comments and discussion from WebKit folks on the issue thread, but it is a bit unclear whether the overall proposal is something WebKit would support. This was brought up and discussed, e.g., on the DOM spec PR here: https://github.com/whatwg/dom/pull/858#issuecomment-623735890 Please see below for all of the relevant supporting documents and discussions. Explainer: https://github.com/mfreed7/declarative-shadow-dom/blob/master/README.md WhatWG DOM Issue discussion: https://github.com/whatwg/dom/issues/831 HTML Spec PR: https://github.com/whatwg/html/pull/5465 DOM Spec PR: https://github.com/whatwg/dom/pull/858 TAG review: https://github.com/w3ctag/design-reviews/issues/494 Request for Mozilla Position: https://github.com/mozilla/standards-positions/issues/335 Thanks, Mason Freed

2 9

Request for position on import maps
by Domenic Denicola 21 Jan '21

21 Jan '21

Hello webkit-dev, For the last couple of years myself some other Chrome folks have been working on the import maps proposal. This allows controlling the behavior of JavaScript import statements and import() expressions, in particular by allowing the page to customize the translation of the module specifiers used there into URLs. Developer reception of the feature has been very positive, with continual prompting for when it'll be widely available in more browsers, and a plethora of community-created tools and polyfills. Chrome is working toward shipping this in an imminent release, and we'd love any thoughts or contributions from the WebKit community. Relevant links: * Explainer: https://github.com/WICG/import-maps/blob/master/README.md * Spec (would eventually merge into HTML upon multi-implementer interest): https://wicg.github.io/import-maps/ * Test suite: https://github.com/web-platform-tests/wpt/tree/master/import-maps/ (although getting rearranged a good bit in https://github.com/web-platform-tests/wpt/pull/26239 ) * Mozilla standards position discussion, with lots of web developers contributing: https://github.com/mozilla/standards-positions/issues/146 * Some gathered evidence of developer interest: https://github.com/WICG/import-maps#community-polyfills-and-tooling Thanks for your time! -Domenic

3 3

Feedback on the performance.measureMemory API proposal
by Ulan Degenbaev 14 Dec '20

14 Dec '20

Hello webkit-dev, I'd like to solicit feedback as well as an official position from Webkit on our proposal for the new memory measurement API: https://github.com/WICG/performance-measure-memory The proposal generalizes the previous performance-memory proposal [1] that was abandoned [2] due to information leak of cross-origin resources. The new API addresses the information leak and process model issues raised in the previous feedback [3]. What is different in the new API? - It relies on COOP+COEP [4] for preventing cross-origin information leaks (see "Security Considerations"). Besides security, COOP+COEP also helps implementability because the browser needs to isolate the web page from other web pages. - It does not assume any particular process model and is defined using the standard concepts such as JS agent cluster and browsing context group. - It has an extensible and generic interface for breaking down memory usage by type (JS, DOM, GPU) and owner. Implementers can choose the level of detail they are willing to provide based on their performance trade-offs (see "Performance Considerations"). Thank you, Ulan. [1]: The previous proposal: https://github.com/WICG/performance-memory/blob/master/explainer.md [2]: Information leak affecting the previous proposal: https://github.com/mozilla/standards-positions/issues/85#issuecomment-42638… [3]: Feedback from Apple on the previous proposal: https://github.com/WICG/performance-memory/issues/14 [4]: COOP and COEP explained: https://docs.google.com/document/d/1zDlfvfTJ_9e8Jdc8ehuV4zMEu9ySMCiTGMS9y0G… Other Relevant Links Explainer: https://github.com/WICG/performance-measure-memory Open issues: https://github.com/WICG/performance-measure-memory/issues WebPerf WG F2F June 2019 presentation (for older iteration of the API): https://docs.google.com/document/d/1uQ7pXwuBv-1jitYou7TALJxV0tllXLxTyEjA2n1… TAG design review (for older iteration of the API): https://github.com/w3ctag/design-reviews/issues/386 Issue on Mozilla standards-positions: https://github.com/mozilla/standards-positions/issues/281

1 1

Request for position on the Origin-Isolation header
by Domenic Denicola 11 Dec '20

11 Dec '20

Hello webkit-dev, I've been working on a new header called Origin-Isolation, which is a way of allowing origins to opt-out of using document.domain and cross-origin sharing of WebAssembly.Module, and thus allowing the browser to put them into an origin-keyed agent cluster instead of a site-keyed one. This could in turn allow the browser to make better behind-the-scenes decisions for process isolation, or other resource allocation decisions, since sites no longer have any ways to synchronously communicate cross-origin. Relevant links: * Explainer: https://github.com/WICG/origin-isolation * HTML spec PR: https://github.com/whatwg/html/pull/5545 * Test suite: https://github.com/web-platform-tests/wpt/tree/master/origin-isolation * Gecko "worth prototyping" standards position: https://mozilla.github.io/standards-positions/#domenic-origin-isolation A natural question one might ask is how this relates to COOP+COEP? The explainer has that covered: https://github.com/WICG/origin-isolation#coop--coep Thanks for your time! -Domenic

3 4

Re: [webkit-dev] Github mirror is not updating
by Jonathan Bedard 30 Nov '20

30 Nov '20

Not at first, but at the moment, yes, it is intentional. We are working on migrating WebKit to a more permanent and complete Git repository, which involves editing history. Because of this, we didn’t want to further the confusion we already have with two different sets of git hashes. The new repository should be pushed in the next week or two In the mean time, https://git.webkit.org/WebKit-https.git <https://git.webkit.org/WebKit-https.git> (secure, but different hashes than GitHub) and https://git.webkit.org/WebKit.git <https://git.webkit.org/WebKit.git> (uses the same hashes as GitHub), are up-to-date mirrors. Jonathan > Hi, > > I noticed that the github mirror at https://github.com/webkit/webkit is not getting > the latest commits from WebKit (it is now about a month behind). Is that intentional? > > Thanks, > -- > Adrien / PulkoMandy

1 0

WebKit SVN Tree clousure (Nov 22nd - Nov 26th, 2020)
by Ling Ho 30 Nov '20

30 Nov '20

Hello WebKit, Some of our critical continuous integration services will be unavailable due to planned underlying infrastructure maintenance. Most EWS and post-commit bots will be down, so we will be closing the WebKit SVN tree to avoid accumulating difficult to diagnose regressions during this lengthy outage. The planned closure of the tree will begin at *8 pm (PST) on Sunday, Nov 22nd, 2020* until no later than *11 pm (PST) on Thursday, Nov 26th**, 2020. *Please email admin(a)webkit.org if you have any questions or concerns. Thanks, ... Ling Ho

3 4