December 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

Using C++20 in WebKit
by Alex Christensen 10 Mar '22

10 Mar '22

In April 2019 in https://bugs.webkit.org/show_bug.cgi?id=197131 <https://bugs.webkit.org/show_bug.cgi?id=197131> I increased WebKit’s minimum C++ language requirement from C++14 to C++17. In 2022 I’m planning to increase WebKit’s minimum C++ requirement from C++17 to C++20. Would April 2022 be a good time to do that?

4 14

[PSA] WebKitGTK layout testers available on the Bugzilla EWS bubbles
by Carlos Alberto Lopez Perez 24 Dec '21

24 Dec '21

Hi, Some of you might have noticed that since a few days ago on the Bugzilla there is a new EWS bubble for GTK WK2 layout tests. Until now the GTK port was without layout test coverage on the EWS. It has been a challenge to add this testers on the EWS because the GTK port layout tests are usually not 100% green and there are usually unexpected failures and also flakies. This means that the EWS was usually either reporting false positives (due to flakies) or exiting early (because of more than 30 unexpected failures on the clean tree or more than 30 accumulated between the ones caused by patch and the ones in the clean tree) We tried to do extra gardening efforts to try to mark all the flakies and also to try to keep it always green, but for several reasons we couldn't keep it on this always-green status for more than a few consecutive days. Also new flakies would keep appearing. So we ended deploying a different version of the EWS that has a much higher tolerance to pre-existent failures (up to 500 before exiting early) and also that tries hard to discard pre-existent failures and flakies by repeating each failure 10 times with patch and 10 times without it. [1] This new version of the EWS WK2 layout tester is only used on the GTK port for now, and we plan to use it also for the WPE port when we deploy layout tests on the EWS for it. So this has the advantage that this EWS should not report false positives or flakies. Any failure reported by it should be a consistent new failure (fails always with the patch but never without it after 10 runs with patch and 10 without it). If you see it reporting any false positive please let me know it. However, it has also the disadvantage that the view to see the layout test results is a bit confusing because you see there also failures unrelated with your patch (those are either pre-existent failures or flakies) My tip here is to only pay attention to the test failures reported by the EWS as new, which are those that are shown on the Bugzilla bubble when you hover the mouse over it (or those that appear on the e-mail that you receive from the EWS) Perhaps I'm thinking it can be a further improvement to add an extra step to do a final run with the patch just for the list of new failures, in order to present a clean view of the results with only the new failures. This can be also very useful to be able to properly use the script update-test-expectations-from-bugzilla to automatically apply the new test expectations from the EWS locally. Any comments welcome :) Best regards! [1] https://bugs.webkit.org/show_bug.cgi?id=231999

2 2

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

WebKit Objective-C++ style, pointer and reference placement
by Kimmo Kinnunen 21 Dec '21

21 Dec '21

Currently we have this: https://webkit.org/code-style-guidelines/#pointers-and-references <https://webkit.org/code-style-guidelines/#pointers-and-references> Pointers and References Pointer types in non-C++ code Pointer types should be written with a space between the type and the * (so the * is adjacent to the following identifier if any). This is not what was originally intended, and I’m planning to fix the document. The original idea was to say: “Objective-C classes should have space between the type name and the *”. I can update the document to reflect this. However, this rule is a bit difficult to enforce and quite difficult to format automatically. Also, subjectively for me, it is a bit hard to remember. Maybe also for other people, as current Objective-C++ is formatted inconsistently in this regard. To enforce this, I’d imagine we hardcode a semi-comprehensive list of Objective-C class names to check-webkit-style. For automatic formatting, the scripts can fix up clang-format diffs based on the hardcoded list. An alternative suggestion would be to change the rule and document as follows: * All pointers, references and rvalue-references in Objective-C++ are formatted as current C++ style mandates * All C and Objective-C code should be formatted with rules consistent to the C++ rules * The exception would be public Objective-C headers and WebKit SPI headers, which would be formatted as part of Apple internal API definition process The change above would be fitting to programmatic formatting and format verification, as it simplifies the rules and avoids the need of semantic analysis of the code for the formatting purposes. Please reply to this to discuss which way would be preferable. The bug: https://bugs.webkit.org/show_bug.cgi?id=234294 <https://bugs.webkit.org/show_bug.cgi?id=234294> Br, Kimmo

2 1

Re: [webkit-dev] Request for position on AudioContext.outputLatency
by Yoav Weiss 15 Dec '21

15 Dec '21

(Copying Hongchan's reply to the list) On Wed, Dec 15, 2021 at 5:44 PM Hongchan Choi <hongchan(a)chromium.org> wrote: > Hi Youenn, > > A first good step would be to add the corresponding fingerprinting markup >> to the web audio spec. >> > > Do you believe the current language is not enough? Then please file an > issue against the spec, so we can reopen the discussion in WG: > https://github.com/WebAudio/web-audio-api/issues > > https://w3c.github.io/mediacapture-output/ allows listing devices that a >> user granted. >> In case devices are exposed to JS, exposing latency for those devices >> seems fine. >> > > Doesn't it require a full permission from the user? I am not sure whether > exposing the latency value has the same severity as allowing the microphone > access. > > I also wonder whether output latency might be useful outside of >> AudioContext and whether it would make sense to expose this information on >> MediaDevices as well. Could that be a device selection choice? >> > > MediaTrackConstraints have "latency" field ( > https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/late…) > but I am not sure that is within the scope of the new API in question. > > The main issue is with the default system audio output. Passive filtering >> should not be allowed in that case. For instance, if the AudioContext is >> not producing audio, exposing latency does not make sense. >> > > When AudioContext is suspended or not allowed to play, the latency should > return zero value. This makes sense because the delay information from the > audio infra changes every audio callback, but the suspended AudioContext > won't have any calls from the device. Hence, the zero value. > > Can you elaborate what you meant by "passive filtering"? > > I am a bit unclear about when latency can change, for instance say audio >> context is running and the default system speaker is changed. If it can >> change, should there be some form of events to let the application know. Or >> should it be left to things like MediaDevices.ondevicechange? >> > > MediaDevices.ondevicechange is designed to serve the purpose you > mentioned. Upon this event, the user can enumerate the device listing again > to get the latest information - which is gated by the user permission. > > Cheers, > Hongchan > > PS: Whom should I talk to about the WebKit mailing list subscription? When > I log in at https://lists.webkit.org/mailman/listinfo/webkit-dev, I am > still getting 403. > > > On Wed, Dec 15, 2021 at 8:06 AM youenn fablet <youennf(a)gmail.com> wrote: > >> Thanks Hongchan, >> >> It is good to hear that these discussions are happening. >> A first good step would be to add the corresponding fingerprinting markup >> to the web audio spec. >> >> A few additional thoughts: >> >> https://w3c.github.io/mediacapture-output/ allows listing devices that a >> user granted. >> In case devices are exposed to JS, exposing latency for those devices >> seems fine. >> I also wonder whether output latency might be useful outside of >> AudioContext and whether it would make sense to expose this information on >> MediaDevices as well. Could that be a device selection choice? >> >> The main issue is with the default system audio output. Passive filtering >> should not be allowed in that case. For instance, if the AudioContext is >> not producing audio, exposing latency does not make sense. >> >> I am a bit unclear about when latency can change, for instance say audio >> context is running and the default system speaker is changed. If it can >> change, should there be some form of events to let the application know. Or >> should it be left to things like MediaDevices.ondevicechange? >> >> >> Le lun. 13 déc. 2021 à 20:00, Hongchan Choi <hongchan(a)chromium.org> a >> écrit : >> >>> Hi Youenn, >>> >>> Thanks for your response. >>> >>> Say I switch from builtin speakers to Bluetooth headset using MacOS >>>> system menu. >>>> >>> >>> If changing a device doesn't reflect the current status correctly, the >>> feature is not useful. That said, the accuracy/precision of a value being >>> served is up to the user agent's discretion. >>> >>> If so, the spec should identify this as potential fingerprinting and >>>> should provide mitigations. >>>> And we should evaluate fingerprinting-free alternatives. >>> >>> >>> We are aware of the issue and it is being discussed in the blink-dev I2S >>> thread. >>> >>> What does PING WG think about this? >>> >>> >>> Please note that Web Audio API is currently a W3C Recommendation. >>> - The WG went through several PING reviews in 2020, for example: >>> https://github.com/WebAudio/web-audio-api/issues/2061 >>> - One of device-related PING threads: >>> https://github.com/w3cping/tracking-issues/issues/50 >>> >>> As a result of the privacy discussion, the spec has a dedicated section >>> on security and privacy: >>> https://webaudio.github.io/web-audio-api/#priv-sec >>> >>> The relevant excerpt: >>> "Fingerprinting via latency is also possible; it might be possible to >>> deduce this from baseLatency and outputLatency. Mitigation strategies >>> include adding jitter (dithering) and quantization so that the exact skew >>> is incorrectly reported. Note however that most audio systems aim for low >>> latency, to synchronise the audio generated by WebAudio to other audio or >>> video sources or to visual cues (for example in a game, or an audio >>> recording or music making environment). Excessive latency decreases >>> usability and may be an accessibility issue." >>> >>> Cheers, >>> Hongchan >>> >>> >>> On Mon, Dec 13, 2021 at 1:15 AM youenn fablet <youennf(a)gmail.com> wrote: >>> >>>> Looking at >>>> https://www.w3.org/TR/webaudio/#dom-audiocontext-outputlatency, it >>>> states that: >>>> > If the audio output device is changed the outputLatency >>>> <https://www.w3.org/TR/webaudio/#dom-audiocontext-outputlatency> attribute >>>> value will be updated accordingly. >>>> >>>> The use case seems ok, but I worry about fingerprinting if it means >>>> this allows a web page to passively identify user speakers. >>>> Say I switch from builtin speakers to Bluetooth headset using MacOS >>>> system menu. >>>> >>>> If so, the spec should identify this as potential fingerprinting and >>>> should provide mitigations. >>>> And we should evaluate fingerprinting-free alternatives. >>>> What does PING WG think about this? >>>> >>>> Le lun. 13 déc. 2021 à 09:39, Yoav Weiss via webkit-dev < >>>> webkit-dev(a)lists.webkit.org> a écrit : >>>> >>>>> (Sent on behalf of Hongchan Choi, who failed to subscribe to this >>>>> mailing list) >>>>> >>>>> Hey folks! >>>>> >>>>> AudioContext.outputLatency is to inform the time at which the first >>>>> sample in the buffer is actually processed by the audio output device. This >>>>> is useful when synchronizing the audio generated by Web Audio to other >>>>> audio or video sources or to visual cues [1]. >>>>> >>>>> This is already implemented in FireFox and we're looking to ship it in >>>>> Chrome soon [2][3]. Would you all be interested in this feature? >>>>> >>>>> Thanks, >>>>> Hongchan >>>>> >>>>> [1] https://www.w3.org/TR/webaudio/#dom-audiocontext-outputlatency >>>>> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1324552 >>>>> [3] >>>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/dTQniJNVVMY/m/hPFwY1… >>>>> >>>>> _______________________________________________ >>>>> webkit-dev mailing list >>>>> webkit-dev(a)lists.webkit.org >>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev >>>>> >>>>

1 0