[webkit-dev] RFC: Improving “RefPtr inside destructor” checking

[Geoff Garen via webkit-dev]

It’s a use-after-free error to create a RefPtr during ~T(), and have that 
RefPtr live past the end of ~T().

In debug builds, we try to catch this error by eagerly assertion 
!T::m_deletionHasBegun in the RefPtr constructor.

At the same time, our smart pointer rules sometimes require us to use RefPtr 
inside functions that are reachable from destructors. To suppress the assertion 
in these cases, we use RefPtrAllowingPartiallyDestroyed.

Problems I see with the current approach:
* We don’t do any checking in release builds
* RefPtrAllowingPartiallyDestroyed complicates our smart pointer rules, when 
we want them to be simple

I’d like to improve this situation.

Enable checking in release builds 

There are two ways we can check in release builds without adding overhead.

Option 1: deref() checks for outstanding pointers after running ~T(), and if 
there are some, crashes eagerly.

We did not like this behavior in CheckedPtr. It produced crash backtraces 
where we simply didn’t know what pointer had lived to long, or which code had 
made a mistake. But RefPtr is different. If Option 1 crashes, we know that 
the error of escaping a pointer after destruction happened inside the 
destructor that is crashing (or one of its callees). So maybe we have enough 
information to go on.

Option 2: Scribble and leak the object, and crash later, just like CheckedPtr.

Option 2 has the upside that the crashing backtrace will usually point directly 
to the errant pointer.

Option 2 seems fine too, but it has the downside that, if you escape a pointer 
in the destructor, and then make more pointers after the destructor, the 
pointer that ultimately crashes may be far removed from the pointer the 
originally escaped.

Any strong preferences, or should I just try something?

Remove RefPtrAllowingPartiallyDestroyed and the debug-only 
!T::m_deletionHasBegun check.

Once we have a form of checking that works in release builds, the existing 
debug-only assertion is arguably redundant. Getting rid of it and getting rid 
of RefPtrAllowingPartiallyDestroyed can simplify our smart pointer rules.

One downside to removing RefPtrAllowingPartiallyDestroyed it that it can be 
nice during code review to enforce a discipline that 
RefPtrAllowingPartiallyDestroyed should only be used on the stack. (This 
discipline is pretty obvious in a lot of cases, but not all — see 
WebCore::HTMLMediaElement::speakCueText and WebCore:: 
MediaSource::ensureWeakOnHTMLMediaElementContext.)

Any strong preferences, or should I just try something?

Thanks,
Geoff___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Use of Swift (for bridging) in the WebKit project

[Geoff Garen via webkit-dev]

Got it. Seems like, in this case, this is the best (i.e. only) way to do things.

For the general case, I’m not sure what the current “no new language” policy 
states, but I would favor some kind of amendment that said “…unless it’s the 
only way to do things because you’re calling out to something in that language, 
and in that case please try to make the usage as minimal as reasonably 
possible”.

Thanks,
Geoff

> On Jun 9, 2021, at 11:37 AM, Jer Noble  wrote:
> 
> 
> 
>> On Jun 9, 2021, at 11:31 AM, Geoff Garen  wrote:
>> 
>> In this specific case
>> 
>>  What is the API we’re trying to call into?
> 
> This is calling into the GroupActivities.framework API which was announced at 
> WWDC this week.
> 
>>  Is using Swift the only way to call into it?
> 
> This was true at the time we wrote it, as GroupActivities required using the 
> Combine framework, which does not have an Objective-C API. I'm learning that 
> this may not be the case in the future. If GroupActivities is modified to not 
> require Combine, I will gladly remove the .swift.
> 
>>  Is there any way to reduce the use of Swift to only the calls into it, 
>> and not the surrounding objects (which all seem to be marked @objc anyway)?
> 
> The surrounding objects have to be written in Swift (in order to call Swift 
> APIs), but callable from Objective-C/++, which is why they're marked as 
> @objc. I believe that they are as minimal as I can make them.
> 
> -Jer
> 
>> Thanks,
>> Geoff
>> 
>>> On Jun 8, 2021, at 4:27 PM, Sam Weinig via webkit-dev 
>>>  wrote:
>>> 
>>> Hi Jer,
>>> 
>>> I think it sounds like a reasonable rule to allow Swift for bridging 
>>> purposes only, with the caveat that we should prefer Objective-C/C where it 
>>> can be used.
>>> 
>>> The one other place that Swift seems reasonable for WebKit is in the 
>>> definition and refinement of Swift bindings to WebKit’s public API.
>>> 
>>> That is to say, for the time being, we should avoid Swift in tools and core 
>>> functionality.
>>> 
>>> Thanks for bringing this up on the list.
>>> 
>>> - Sam
>>> 
 On Jun 8, 2021, at 3:57 PM, Jer Noble via webkit-dev 
  wrote:
 
 Hi all!
 
 We're working on some new features that currently use APIs exposed through 
 Swift. We have not yet approved writing and committing WebKit code in 
 Swift, given runtime, library, and just plain mental overhead that comes 
 with adding a new language to the project. But I'd argue that doing so for 
 the purpose of allowing existing C++ code to call into Swift APIs is 
 probably not terrible.
 
 Should we relax our "no new language" policy, only for the purposes of 
 allowing our core language code to call into APIs in Swift?
 
 (ref: https://bugs.webkit.org/show_bug.cgi?id=226757)
 
 Thanks, and look forward to hearing from everyone,
 
 -Jer
 ___
 webkit-dev mailing list
 webkit-dev@lists.webkit.org
 https://lists.webkit.org/mailman/listinfo/webkit-dev
>>> 
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
> 

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Use of Swift (for bridging) in the WebKit project

[Geoff Garen via webkit-dev]

In this specific case

What is the API we’re trying to call into?

Is using Swift the only way to call into it?

Is there any way to reduce the use of Swift to only the calls into it, 
and not the surrounding objects (which all seem to be marked @objc anyway)?

Thanks,
Geoff

> On Jun 8, 2021, at 4:27 PM, Sam Weinig via webkit-dev 
>  wrote:
> 
> Hi Jer,
> 
> I think it sounds like a reasonable rule to allow Swift for bridging purposes 
> only, with the caveat that we should prefer Objective-C/C where it can be 
> used.
> 
> The one other place that Swift seems reasonable for WebKit is in the 
> definition and refinement of Swift bindings to WebKit’s public API.
> 
> That is to say, for the time being, we should avoid Swift in tools and core 
> functionality.
> 
> Thanks for bringing this up on the list.
> 
> - Sam
> 
>> On Jun 8, 2021, at 3:57 PM, Jer Noble via webkit-dev 
>>  wrote:
>> 
>> Hi all!
>> 
>> We're working on some new features that currently use APIs exposed through 
>> Swift. We have not yet approved writing and committing WebKit code in Swift, 
>> given runtime, library, and just plain mental overhead that comes with 
>> adding a new language to the project. But I'd argue that doing so for the 
>> purpose of allowing existing C++ code to call into Swift APIs is probably 
>> not terrible.
>> 
>> Should we relax our "no new language" policy, only for the purposes of 
>> allowing our core language code to call into APIs in Swift?
>> 
>> (ref: https://bugs.webkit.org/show_bug.cgi?id=226757)
>> 
>> Thanks, and look forward to hearing from everyone,
>> 
>> -Jer
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Request for position: Aligning high-resolution timer granularity to cross-origin isolated capability

[Geoff Garen via webkit-dev]

For the 100 microsecond value — our research suggests that you need a much 
higher value in vulnerable contexts.

For the guaranteed isolated case — have you considered the use of high 
precision time to carry out non-Spectre timing attacks?

Thanks,
Geoff

> On Mar 17, 2021, at 3:38 AM, Yoav Weiss via webkit-dev 
>  wrote:
> 
> Hey folks,
> 
> We recently changed  the HR-time spec 
>  to better align its resolution clamping with 
> cross-origin isolated capability 
> ,
>  and now I'm interested in shipping this change in Chromium.
> In practice that means that Chromium would be reducing its resolution in 
> non-isolated contexts (regardless of the platform's site-isolation status) to 
> 100 microseconds, and increasing it in cross-origin isolated contexts (even 
> in platforms without site-isolation, e.g. Android) to 5 microseconds.
> 
> As WebKit already clamps those timers to 1ms (AFAIK), I'd mostly like your 
> position on the latter. Would y'all be interested in increasing timer 
> granularity in contexts which have guarantees against pulling in cross-origin 
> resources without their opt-in?
> 
> I'd appreciate your thoughts on the matter.
> 
> Cheers :)
> Yoav
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Making WTF::StringImpl and WTF::AtomString thread-safe

[Geoff Garen via webkit-dev]

>> 1. It’s not just ref counting. 
>> 
>> To make String thread-safe, you also need to address all other data
>> members. That means all state in m_hashAndFlags, including the
>> 8bit/16bit state.
>> 
>> It appears that your testing strategy did not reveal this point so
>> far; so, you probably need to expand your plan for unit testing
>> concurrent access to a string, with a focus on writing tests that fail
>> with the current implementation.
> 
> I did consider this, I've also made m_hashAndFlags atomic in the
> attached patch. Indeed, tests for concurrent string usage and expected
> behaviour would be desirable, I would take that as a given (but it's
> worth mentioning).

Great!

>> 3. I’m surprised by the premise that thread-safe String is a
>> requirement for FontCache and/or the GPU Process.
>> 
>> It’s certainly a false premise that there’s consensus on this premise,
>> since I do not agree.
>> 
>> Can you share some problem statements regarding FontCache and/or the
>> GPU Process that explain the problem we’re trying to solve?
> 
> I can't talk about GPU Process, this was something that was mentioned to
> me when I was talking about methods of making FontCache thread-safe.
> 
> FontCache makes extensive use of AtomString for look-ups and
> comparisons. I had a few alternative ideas for making FontCache safe to
> use in a Worker, but after discussing them on Slack, it seemed like
> making FontCache safe for concurrent access and making String
> thread-safe were both desirable for future work. I really hope other
> people will chime in here, my personal preference would be to do
> something less invasive.

So the goal is to enable use of fonts (and FontCache) in Workers?

> Other ideas I had for making FontCache Worker-safe;
> - Add a FontCacheProxy object that calls onto the main-thread FontCache
> and blocks (not ideal for performance)

Yeah, that doesn’t sound great.

> - Make FontCache not rely on static data and have a completely separate
> FontCache per-Worker, created on-demand (not ideal for memory usage)

Seems OK. I think the main downside to this proposal is that an app that moves 
font-related work to a worker as a performance optimization will also 
experience a performance regression by hitting a cold FontCache.

> - Make FontCache thread-safe, but do it via introducing a completely
> separate thread-safe AtomString type and leave the current one as it is
> (I don't have a good grasp of how difficult this would actually be)

I had to chuckle at this point because the obvious name for this new 
thread-safe AtomString class would be AtomicString, the prior name of 
AtomString.

I think it might be worthwhile to prototype either a per-thread FontCache or a 
FontCache based on a custom string type or both. One reason it might be 
worthwhile is that it will reveal the non-string work that needs to happen to 
achieve thread safety (or at least thread isolation) in FontCache. For example, 
FontDataCache and its associated types look like they might need work. Another 
reason it might be worthwhile is that I believe that solving this problem just 
for FontCache will be a smaller project than making Strings generally 
thread-safe.

I think you’re right that making Strings generally thread-safe would be good 
for the project overall. I’m just worried that it might set back the FontCache 
project.

Thanks,
Geoff

> 
> Cheers,
> 
> Chris
> 
>> Thanks,
>> Geoff
>> 
>>> On Dec 1, 2020, at 9:09 AM, Chris Lord via webkit-dev 
>>>  wrote:
>>> 
>>> Hi all,
>>> 
>>> As part of the work for making FontCache thread-safe, it's necessary for
>>> there to be a thread-safe AtomString. After discussion, it seems that a
>>> thread-safe StringImpl is generally desirable and GPUProcess also has a
>>> need of it. I've filed a bug to track this work:
>>> https://bugs.webkit.org/show_bug.cgi?id=219285
>>> 
>>> Google have already done this for Blink and there's a nice plan and lots
>>> of discussion to read. Their plan document is linked in the bug. I think
>>> we'd be well-served by taking broadly the same approach, which is to
>>> make ref-counting on StringImpl atomic and to guard AtomStringTable
>>> access with a lock.
>>> 
>>> Making String thread-safe has implications, of course, and I'd like to
>>> open discussion on this - Making ref-counting atomic on StringImpl has a
>>> significant, negative impact on the expense of ref and deref operations.
>>> I'm interested in discussing how we should approach this in terms of
>>> tracking the work in Bugzilla and how to go about landing it. Perhaps
>>> people also have alternative ideas?
>>> 
>>> On the bug is a first-run at implementing the above approach, currently
>>> minus the follow-up of everywhere taking into consideration that
>>> String/AtomString are now thread-safe. The impact on StringImpl
>>> ref/deref performance has it running on my Xeon desktop machine at about
>>> 30-50% of non-atomic ref/deref performance. Speedometer 2.0 takes a 1-8%
>>> hit consideri

Re: [webkit-dev] Making WTF::StringImpl and WTF::AtomString thread-safe

[Geoff Garen via webkit-dev]

>> Because it’s so expensive, and because we have a no regression policy for 
>> performance, I don’t think there’s a way to land this change in pieces. It 
>> has to be a mega-patch, so we can test its performance as a whole.
> 
> Were you able to quantify anything additional about the performance 
> regression on Intel, for instance, if there were any types of String usage 
> that were particularly hard hit? If not (or you haven’t tried), do you think 
> there are any metrics we should look into gathering that might help pin point 
> where things could maybe be optimized? Ideally we would be able to identify 
> places where we unnecessarily ref-deref StringImpls where either a move or 
> judicious use of StringView would work just as well. 

I didn’t debug deeply. Maybe we’ll be able to reduce refcount changes. Maybe 
not.

> The Blink document (linked in the bugzilla bug) suggests they identified some 
> mitigations, but I am not clear they will be representative for us today.

It’s not obvious to me that what we’re doing and what Blink are doing are 
analogous. One difference is that we use the String class in our JavaScript 
engine. Another difference is that we have a no regression policy on benchmarks.

Thanks,
Geoff

> 
> Putting on my old bindings hat, one area that might be able to get some wins 
> here is how Strings (and other ref counted things, but let’s stay focused) 
> are passed from the JS bindings layer to the DOM. In most cases Strings 
> should be able to be moved into the DOM without refcount churn, but since 
> many of our functions take `const String&` or `const AtomString&` we lose out.
> 
> - Sam
> 
>> 
>> 3. I’m surprised by the premise that thread-safe String is a requirement for 
>> FontCache and/or the GPU Process.
>> 
>> It’s certainly a false premise that there’s consensus on this premise, since 
>> I do not agree.
>> 
>> Can you share some problem statements regarding FontCache and/or the GPU 
>> Process that explain the problem we’re trying to solve?
>> 
>> Thanks,
>> Geoff
>> 
>>> On Dec 1, 2020, at 9:09 AM, Chris Lord via webkit-dev 
>>>  wrote:
>>> 
>>> Hi all,
>>> 
>>> As part of the work for making FontCache thread-safe, it's necessary for
>>> there to be a thread-safe AtomString. After discussion, it seems that a
>>> thread-safe StringImpl is generally desirable and GPUProcess also has a
>>> need of it. I've filed a bug to track this work:
>>> https://bugs.webkit.org/show_bug.cgi?id=219285
>>> 
>>> Google have already done this for Blink and there's a nice plan and lots
>>> of discussion to read. Their plan document is linked in the bug. I think
>>> we'd be well-served by taking broadly the same approach, which is to
>>> make ref-counting on StringImpl atomic and to guard AtomStringTable
>>> access with a lock.
>>> 
>>> Making String thread-safe has implications, of course, and I'd like to
>>> open discussion on this - Making ref-counting atomic on StringImpl has a
>>> significant, negative impact on the expense of ref and deref operations.
>>> I'm interested in discussing how we should approach this in terms of
>>> tracking the work in Bugzilla and how to go about landing it. Perhaps
>>> people also have alternative ideas?
>>> 
>>> On the bug is a first-run at implementing the above approach, currently
>>> minus the follow-up of everywhere taking into consideration that
>>> String/AtomString are now thread-safe. The impact on StringImpl
>>> ref/deref performance has it running on my Xeon desktop machine at about
>>> 30-50% of non-atomic ref/deref performance. Speedometer 2.0 takes a 1-8%
>>> hit considering error margins, but I'm fairly certain it's mostly on the
>>> higher end of that and I've not run enough iterations just yet.
>>> Jetstream 1.1 seems practically unaffected, I can't run 2.0 with or
>>> without the patch, it appears to hang the browser on the bomb-workers
>>> test (at least if it completes, it's not in a reasonable time-frame). I
>>> would guess that results may vary wildly depending on platform and
>>> available atomic access primitives. As one might expect, the impact is
>>> far less on a debug build.
>>> 
>>> I think the initial patch of making it thread-safe vs. the follow-up of
>>> altering various areas to take it into account could/should be split,
>>> but I assume we'd want to land them at the same time. This is cumbersome
>>> with how WebKit Bugzilla currently works and I'd like to hear what
>>> people think and how similar changes have been made in the past.
>>> 
>>> Thoughts?
>>> 
>>> Regards,
>>> 
>>> Chris
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org 
>> https://lists.webkit.org/mailman/listinfo/webkit-dev 
>> 
___

Re: [webkit-dev] Making WTF::StringImpl and WTF::AtomString thread-safe

[Geoff Garen via webkit-dev]

A few thoughts here:

1. It’s not just ref counting. 

To make String thread-safe, you also need to address all other data members. 
That means all state in m_hashAndFlags, including the 8bit/16bit state.

It appears that your testing strategy did not reveal this point so far; so, you 
probably need to expand your plan for unit testing concurrent access to a 
string, with a focus on writing tests that fail with the current implementation.

2. Performance is a challenge.

I’ve tested making String thread-safe pretty extensively. On modern Apple 
Silicon, it’s free. But on Intel chips, it’s very expensive.

Because it’s so expensive, and because we have a no regression policy for 
performance, I don’t think there’s a way to land this change in pieces. It has 
to be a mega-patch, so we can test its performance as a whole.

3. I’m surprised by the premise that thread-safe String is a requirement for 
FontCache and/or the GPU Process.

It’s certainly a false premise that there’s consensus on this premise, since I 
do not agree.

Can you share some problem statements regarding FontCache and/or the GPU 
Process that explain the problem we’re trying to solve?

Thanks,
Geoff

> On Dec 1, 2020, at 9:09 AM, Chris Lord via webkit-dev 
>  wrote:
> 
> Hi all,
> 
> As part of the work for making FontCache thread-safe, it's necessary for
> there to be a thread-safe AtomString. After discussion, it seems that a
> thread-safe StringImpl is generally desirable and GPUProcess also has a
> need of it. I've filed a bug to track this work:
> https://bugs.webkit.org/show_bug.cgi?id=219285
> 
> Google have already done this for Blink and there's a nice plan and lots
> of discussion to read. Their plan document is linked in the bug. I think
> we'd be well-served by taking broadly the same approach, which is to
> make ref-counting on StringImpl atomic and to guard AtomStringTable
> access with a lock.
> 
> Making String thread-safe has implications, of course, and I'd like to
> open discussion on this - Making ref-counting atomic on StringImpl has a
> significant, negative impact on the expense of ref and deref operations.
> I'm interested in discussing how we should approach this in terms of
> tracking the work in Bugzilla and how to go about landing it. Perhaps
> people also have alternative ideas?
> 
> On the bug is a first-run at implementing the above approach, currently
> minus the follow-up of everywhere taking into consideration that
> String/AtomString are now thread-safe. The impact on StringImpl
> ref/deref performance has it running on my Xeon desktop machine at about
> 30-50% of non-atomic ref/deref performance. Speedometer 2.0 takes a 1-8%
> hit considering error margins, but I'm fairly certain it's mostly on the
> higher end of that and I've not run enough iterations just yet.
> Jetstream 1.1 seems practically unaffected, I can't run 2.0 with or
> without the patch, it appears to hang the browser on the bomb-workers
> test (at least if it completes, it's not in a reasonable time-frame). I
> would guess that results may vary wildly depending on platform and
> available atomic access primitives. As one might expect, the impact is
> far less on a debug build.
> 
> I think the initial patch of making it thread-safe vs. the follow-up of
> altering various areas to take it into account could/should be split,
> but I assume we'd want to land them at the same time. This is cumbersome
> with how WebKit Bugzilla currently works and I'd like to hear what
> people think and how similar changes have been made in the past.
> 
> Thoughts?
> 
> Regards,
> 
> Chris
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev