Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Fujii Hironori via webkit-dev]

On Tue, Feb 28, 2023 at 6:22 AM Andres Gonzalez 
wrote:

>
> "To use `WeakPtr` or `ThreadSafeWeakPtr`, make the class inherit from
> `CanMakeWeakPtr` or `CanMakeThreadSafeWeakPtr`, whichever is
> appropriate.  Note that classes that want to implement both
> `ThreadSafeRefCounted` and `ThreadSafeWeakPtr` must inherit from
> `ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<>`.”
>
> I don’t think there is CanMakeThreadSafeWeakPtr{T>. For thread safe
> WeakPtr behavior, you have to inherit from
> ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr.
>

Good catch. It's impossible to create a thread-safe weak ptr without a
thread-safe reference counter. review+. The problem of the wiki is that we
can't create a PR for it.

I was looking for the page since it was renamed to
https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Andres Gonzalez via webkit-dev]

Thanks Ryosuke for the wiki page. Can you please clarify:

"To use `WeakPtr` or `ThreadSafeWeakPtr`, make the class inherit from 
`CanMakeWeakPtr` or `CanMakeThreadSafeWeakPtr`, whichever is appropriate. 
 Note that classes that want to implement both `ThreadSafeRefCounted` and 
`ThreadSafeWeakPtr` must inherit from 
`ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<>`.”

I don’t think there is CanMakeThreadSafeWeakPtr{T>. For thread safe WeakPtr 
behavior, you have to inherit from 
ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr.

—Andres.


> On Jan 30, 2023, at 9:36 PM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
> 
>> On Jan 30, 2023, at 6:29 PM, Fujii Hironori via webkit-dev 
>>  wrote:
>> 
>> On Tue, Jan 31, 2023 at 8:28 AM Ryosuke Niwa via webkit-dev 
>>  wrote:
>>> 
>>> I’ve posted https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules
>>> 
>>> 
>> Very nice.  Can I add the following exception for 
>> webkit.UncountedLambdaCapturesChecker rule?
>> 
>> https://lists.webkit.org/pipermail/webkit-dev/2020-September/031405.html
>> 
>>> We probably also need to figure out a way to exempt all lambda functions
>>> that never get stored anywhere. We have a bunch of helper functions like
>>> WTF::map which just calls lambdas on each item while iterating over an
>>> array, etc... and there is no need to create a separate Ref / RefPtr in
>>> those cases since lambdas are never stored and re-used later.
> 
> Done.
> 
> - R. Niwa
> 
> ___
> 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] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa via webkit-dev]

> On Jan 30, 2023, at 6:29 PM, Fujii Hironori via webkit-dev 
>  wrote:
> 
> On Tue, Jan 31, 2023 at 8:28 AM Ryosuke Niwa via webkit-dev 
>  wrote:
>> 
>> I’ve posted https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules
>> 
>> 
> Very nice.  Can I add the following exception for 
> webkit.UncountedLambdaCapturesChecker rule?
> 
> https://lists.webkit.org/pipermail/webkit-dev/2020-September/031405.html
> 
> > We probably also need to figure out a way to exempt all lambda functions
> > that never get stored anywhere. We have a bunch of helper functions like
> > WTF::map which just calls lambdas on each item while iterating over an
> > array, etc... and there is no need to create a separate Ref / RefPtr in
> > those cases since lambdas are never stored and re-used later.

Done.

- R. Niwa

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Fujii Hironori via webkit-dev]

On Tue, Jan 31, 2023 at 8:28 AM Ryosuke Niwa via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

>
> I’ve posted
> https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules
>
>
Very nice.  Can I add the following exception for
webkit.UncountedLambdaCapturesChecker rule?

https://lists.webkit.org/pipermail/webkit-dev/2020-September/031405.html

> We probably also need to figure out a way to exempt all lambda functions
> that never get stored anywhere. We have a bunch of helper functions like
> WTF::map which just calls lambdas on each item while iterating over an
> array, etc... and there is no need to create a separate Ref / RefPtr in
> those cases since lambdas are never stored and re-used later.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa via webkit-dev]

> On Jan 30, 2023, at 12:11 PM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
>> On Jan 30, 2023, at 10:47 AM, Alexey Proskuryakov  wrote:
>> 
>> Reviving this old thread, reading it again made me wish for two things:
>> 
>> - a wiki document that describes what we are trying to do, not just a thread 
>> which patches the proposal with clarifications;
> 
> Yeah, let me go make one.

I’ve posted https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules

>> - a discussion of why we can postpone figuring out what to do with 
>> containers (like Vector or HashMap> RenderFragmentContainer*>).
> 
> This was probably an oversight on my part. The intention is to make member 
> variables / local variables of container type should also be using smart 
> pointers in its type: e.g. Vector> instead of Vector. 
> WeakHashMap> instead of 
> HashMap. I’ll try to clarify this in 
> the new doc I make.

Fixed in the new wiki documentation.

- R. Niwa

>>> 23 сент. 2020 г., в 1:54 PM, Jan Korous  написал(а):
>>> 
>>> Hi all,
>>> 
>>> I am an engineer at Security Tools team at Apple responsible for the 
>>> tooling support for this effort.
>>> 
 On Sep 23, 2020, at 12:34 PM, Darin Adler  wrote:
 
> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa  wrote:
> 
> There are quite a few cases where data members are references but then 
> those can also be replaced by a simple member function which retrieves 
> the value of the smart pointer member variable and returns a reference.
 
 I think this should be an explicit recommendation in the project of 
 refactoring to follow these rules.
 
> For now, a trivial function is defined as a member function defined in 
> the class declaration whose definition simply returns a member variable 
> (the result of get() or a copy if the member variable is a smart pointer).
 
 That seems like a rule that’s too narrow. I would not want a function to 
 become non-trivial just because I moved it from being inline within the 
 class definition to an inline below the class definition in the same 
 header.
 
 This rule worries me a lot right now; it seems like it could result in an 
 explosion of local variable copies of arguments.
 
> We probably also need to figure out a way to exempt all lambda functions 
> that never get stored anywhere. We have a bunch of helper functions like 
> WTF::map which just calls lambdas on each item while iterating over an 
> array, etc... and there is no need to create a separate Ref / RefPtr in 
> those cases since lambdas are never stored and re-used later.
 
 Does seem important. I am pretty sure I have seen this concept in other 
 languages. We often try to use const Function& for one type of lambda 
 argument and Function&& for the other type, but that’s far from complete.
>>> 
>>> Re: lambda captures - I think we have two ideas here.
>>> 
>>> 1. Allow WeakPtr captures.
>>> This makes sense to do but it implies we have to add the notion of 
>>> ownership to the rules. The thing is that WeakPtr is safe on its own (and 
>>> technically reference-counted) but it can’t be used as a safety measure in 
>>> the way of RefPtr or Ref since it doesn’t own the memory (not even in a 
>>> shared manner).
>>> 
>>> 2. Allow raw pointer/reference captures.
>>> This makes sense given you use generic algorithms in the codebase. I will 
>>> implement a new version of the checker - currently it is still based on 
>>> simple AST analysis and for this kind of reasoning we’ll need to use 
>>> symbolic execution in Clang Static Analyzer.
>>> 
>>> Thanks,
>>> 
>>> Jan
>>> 
 — Darin
>>> 
>>> ___
>>> 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] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa via webkit-dev]

> On Jan 30, 2023, at 10:47 AM, Alexey Proskuryakov  wrote:
> 
> Reviving this old thread, reading it again made me wish for two things:
> 
> - a wiki document that describes what we are trying to do, not just a thread 
> which patches the proposal with clarifications;

Yeah, let me go make one.

> - a discussion of why we can postpone figuring out what to do with containers 
> (like Vector or HashMap).

This was probably an oversight on my part. The intention is to make member 
variables / local variables of container type should also be using smart 
pointers in its type: e.g. Vector> instead of Vector. 
WeakHashMap> instead of 
HashMap. I’ll try to clarify this in the 
new doc I make.

- R. Niwa

>> 23 сент. 2020 г., в 1:54 PM, Jan Korous  написал(а):
>> 
>> Hi all,
>> 
>> I am an engineer at Security Tools team at Apple responsible for the tooling 
>> support for this effort.
>> 
>>> On Sep 23, 2020, at 12:34 PM, Darin Adler  wrote:
>>> 
 On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa  wrote:
 
 There are quite a few cases where data members are references but then 
 those can also be replaced by a simple member function which retrieves the 
 value of the smart pointer member variable and returns a reference.
>>> 
>>> I think this should be an explicit recommendation in the project of 
>>> refactoring to follow these rules.
>>> 
 For now, a trivial function is defined as a member function defined in the 
 class declaration whose definition simply returns a member variable (the 
 result of get() or a copy if the member variable is a smart pointer).
>>> 
>>> That seems like a rule that’s too narrow. I would not want a function to 
>>> become non-trivial just because I moved it from being inline within the 
>>> class definition to an inline below the class definition in the same header.
>>> 
>>> This rule worries me a lot right now; it seems like it could result in an 
>>> explosion of local variable copies of arguments.
>>> 
 We probably also need to figure out a way to exempt all lambda functions 
 that never get stored anywhere. We have a bunch of helper functions like 
 WTF::map which just calls lambdas on each item while iterating over an 
 array, etc... and there is no need to create a separate Ref / RefPtr in 
 those cases since lambdas are never stored and re-used later.
>>> 
>>> Does seem important. I am pretty sure I have seen this concept in other 
>>> languages. We often try to use const Function& for one type of lambda 
>>> argument and Function&& for the other type, but that’s far from complete.
>> 
>> Re: lambda captures - I think we have two ideas here.
>> 
>> 1. Allow WeakPtr captures.
>> This makes sense to do but it implies we have to add the notion of ownership 
>> to the rules. The thing is that WeakPtr is safe on its own (and technically 
>> reference-counted) but it can’t be used as a safety measure in the way of 
>> RefPtr or Ref since it doesn’t own the memory (not even in a shared manner).
>> 
>> 2. Allow raw pointer/reference captures.
>> This makes sense given you use generic algorithms in the codebase. I will 
>> implement a new version of the checker - currently it is still based on 
>> simple AST analysis and for this kind of reasoning we’ll need to use 
>> symbolic execution in Clang Static Analyzer.
>> 
>> Thanks,
>> 
>> Jan
>> 
>>> — Darin
>> 
>> ___
>> 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] Smart Pointer Analysis Tool for WebKit

[Darin Adler via webkit-dev]

I am hoping this ends up being practical. In the CSS code I have recently 
worked on there are many short-lived references, and in some refactoring I was 
not able to change them all to smart pointers without a measurable performance 
degradation in Speedometer, so the code might need redesign to use different 
idioms.

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Alexey Proskuryakov via webkit-dev]

Hi,

Reviving this old thread, reading it again made me wish for two things:

- a wiki document that describes what we are trying to do, not just a thread 
which patches the proposal with clarifications;

- a discussion of why we can postpone figuring out what to do with containers 
(like Vector or HashMap).

- Alexey

> 23 сент. 2020 г., в 1:54 PM, Jan Korous  написал(а):
> 
> Hi all,
> 
> I am an engineer at Security Tools team at Apple responsible for the tooling 
> support for this effort.
> 
>> On Sep 23, 2020, at 12:34 PM, Darin Adler  wrote:
>> 
>>> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa  wrote:
>>> 
>>> There are quite a few cases where data members are references but then 
>>> those can also be replaced by a simple member function which retrieves the 
>>> value of the smart pointer member variable and returns a reference.
>> 
>> I think this should be an explicit recommendation in the project of 
>> refactoring to follow these rules.
>> 
>>> For now, a trivial function is defined as a member function defined in the 
>>> class declaration whose definition simply returns a member variable (the 
>>> result of get() or a copy if the member variable is a smart pointer).
>> 
>> That seems like a rule that’s too narrow. I would not want a function to 
>> become non-trivial just because I moved it from being inline within the 
>> class definition to an inline below the class definition in the same header.
>> 
>> This rule worries me a lot right now; it seems like it could result in an 
>> explosion of local variable copies of arguments.
>> 
>>> We probably also need to figure out a way to exempt all lambda functions 
>>> that never get stored anywhere. We have a bunch of helper functions like 
>>> WTF::map which just calls lambdas on each item while iterating over an 
>>> array, etc... and there is no need to create a separate Ref / RefPtr in 
>>> those cases since lambdas are never stored and re-used later.
>> 
>> Does seem important. I am pretty sure I have seen this concept in other 
>> languages. We often try to use const Function& for one type of lambda 
>> argument and Function&& for the other type, but that’s far from complete.
> 
> Re: lambda captures - I think we have two ideas here.
> 
> 1. Allow WeakPtr captures.
> This makes sense to do but it implies we have to add the notion of ownership 
> to the rules. The thing is that WeakPtr is safe on its own (and technically 
> reference-counted) but it can’t be used as a safety measure in the way of 
> RefPtr or Ref since it doesn’t own the memory (not even in a shared manner).
> 
> 2. Allow raw pointer/reference captures.
> This makes sense given you use generic algorithms in the codebase. I will 
> implement a new version of the checker - currently it is still based on 
> simple AST analysis and for this kind of reasoning we’ll need to use symbolic 
> execution in Clang Static Analyzer.
> 
> Thanks,
> 
> Jan
> 
>> — Darin
> 
> ___
> 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] Smart Pointer Analysis Tool for WebKit

[Jan Korous]

Hi all,

I am an engineer at Security Tools team at Apple responsible for the tooling 
support for this effort.

> On Sep 23, 2020, at 12:34 PM, Darin Adler  wrote:
> 
>> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa  wrote:
>> 
>> There are quite a few cases where data members are references but then those 
>> can also be replaced by a simple member function which retrieves the value 
>> of the smart pointer member variable and returns a reference.
> 
> I think this should be an explicit recommendation in the project of 
> refactoring to follow these rules.
> 
>> For now, a trivial function is defined as a member function defined in the 
>> class declaration whose definition simply returns a member variable (the 
>> result of get() or a copy if the member variable is a smart pointer).
> 
> That seems like a rule that’s too narrow. I would not want a function to 
> become non-trivial just because I moved it from being inline within the class 
> definition to an inline below the class definition in the same header.
> 
> This rule worries me a lot right now; it seems like it could result in an 
> explosion of local variable copies of arguments.
> 
>> We probably also need to figure out a way to exempt all lambda functions 
>> that never get stored anywhere. We have a bunch of helper functions like 
>> WTF::map which just calls lambdas on each item while iterating over an 
>> array, etc... and there is no need to create a separate Ref / RefPtr in 
>> those cases since lambdas are never stored and re-used later.
> 
> Does seem important. I am pretty sure I have seen this concept in other 
> languages. We often try to use const Function& for one type of lambda 
> argument and Function&& for the other type, but that’s far from complete.

Re: lambda captures - I think we have two ideas here.

1. Allow WeakPtr captures.
This makes sense to do but it implies we have to add the notion of ownership to 
the rules. The thing is that WeakPtr is safe on its own (and technically 
reference-counted) but it can’t be used as a safety measure in the way of 
RefPtr or Ref since it doesn’t own the memory (not even in a shared manner).

2. Allow raw pointer/reference captures.
This makes sense given you use generic algorithms in the codebase. I will 
implement a new version of the checker - currently it is still based on simple 
AST analysis and for this kind of reasoning we’ll need to use symbolic 
execution in Clang Static Analyzer.

Thanks,

Jan

> — Darin

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Darin Adler]

> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa  wrote:
> 
> There are quite a few cases where data members are references but then those 
> can also be replaced by a simple member function which retrieves the value of 
> the smart pointer member variable and returns a reference.

I think this should be an explicit recommendation in the project of refactoring 
to follow these rules.

> For now, a trivial function is defined as a member function defined in the 
> class declaration whose definition simply returns a member variable (the 
> result of get() or a copy if the member variable is a smart pointer).

That seems like a rule that’s too narrow. I would not want a function to become 
non-trivial just because I moved it from being inline within the class 
definition to an inline below the class definition in the same header.

This rule worries me a lot right now; it seems like it could result in an 
explosion of local variable copies of arguments.

> We probably also need to figure out a way to exempt all lambda functions that 
> never get stored anywhere. We have a bunch of helper functions like WTF::map 
> which just calls lambdas on each item while iterating over an array, etc... 
> and there is no need to create a separate Ref / RefPtr in those cases since 
> lambdas are never stored and re-used later.

Does seem important. I am pretty sure I have seen this concept in other 
languages. We often try to use const Function& for one type of lambda argument 
and Function&& for the other type, but that’s far from complete.

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa]

On Wed, Sep 23, 2020 at 10:32 AM Darin Adler  wrote:

> On Sep 16, 2020, at 11:32 PM, Ryosuke Niwa  wrote:
>
>1. Every data member to a ref counted object must use either Ref,
>RefPtr, or WeakPtr. webkit.NoUncountedMemberChecker
>
> 
>
> My only worry here is performance. Do we know yet if we can afford it?
>

We still need to find that out. So far, our deployment of smart pointers in
various DOM objects haven't caused perf regressions yet.

The worst case here is Ref, which is much worse than a reference since we
> end up having to use -> instead of . everywhere and you can’t see that
> there is no null involved.
>

In practice, this may not matter much because in many of our codebases,
most of references are used as function arguments, in which case, they're
still allowed without having to store in a local Ref / RefPtr. There are
quite a few cases where data members are references but then those can also
be replaced by a simple member function which retrieves the value of the
smart pointer member variable and returns a reference.

>
>1. Every ref counted base class, if it has a derived class, must
>define a virtual destructor. webkit.RefCntblBaseVirtualDtor
>
> 
>
> The style system has an optimization that intentionally violates this for
> performance reasons (StyleRuleBase).
>

Interesting. I wasn't aware of this example. We're planning to add some
kind of compiler-level annotations to exempt these warnings so we may need
to apply that here if it's important for performance.

>
>1. 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
>
> 
>
> What is a non-trivial function?
>

For now, a trivial function is defined as a member function defined in the
class declaration whose definition simply returns a member variable (the
result of get() or a copy if the member variable is a smart pointer).

>
>1. Every ref counted object must be captured using Ref or RefPtr for
>lambda. webkit.UncountedLambdaCapturesChecker
>
> 
>
> Ref, RefPtr, or WeakPtr, right?
>
> Same concern about Ref vs references.
>

This is an interesting point. We should probably amend the rule to allow
WeakPtr as well.

Jan: do we currently allow WeakPtr or just Ref / RefPtr?

We probably also need to figure out a way to exempt all lambda functions
that never get stored anywhere. We have a bunch of helper functions like
WTF::map which just calls lambdas on each item while iterating over an
array, etc... and there is no need to create a separate Ref / RefPtr in
those cases since lambdas are never stored and re-used later.

I wonder if there is a way for your tool to automatically figure that out?
e.g. notice that a function never stores lambda anywhere, and then
propagate that information as some kind of function attribute. Then any
function that doesn't store lambda anywhere and only calls those functions
that also don't store lambda anywhere can marked as "safe".

>
>1. Local variables - we’re still working on this (
>https://reviews.llvm.org/D83259)
>
> I am looking forward to learning more about the proposal here.
>
> Same concern about Ref vs. references.
>
> I really want to see before/after for some non-trivial source files with
> significant problems; where will this drive the most change and what will
> things look like after?
>

Right, for both performance measurements and code changes, we probably want
to fix warnings in a large quality in some important files and see
before/after.

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Darin Adler]

> On Sep 23, 2020, at 10:32 AM, Darin Adler  wrote:
> 
>> On Sep 16, 2020, at 11:32 PM, Ryosuke Niwa > > wrote:
>> Every data member to a ref counted object must use either Ref, RefPtr, or 
>> WeakPtr. webkit.NoUncountedMemberChecker 
>> My
>>  only worry here is performance. Do we know yet if we can afford it?
> 
> The worst case here is Ref, which is much worse than a reference since we end 
> up having to use -> instead of . everywhere and you can’t see that there is 
> no null involved.

I don’t mean performance worst case. I meant “outside of performance concerns, 
the worst downgrade of our programming idioms is Ref”.
>> Every ref counted base class, if it has a derived class, must define a 
>> virtual destructor. webkit.RefCntblBaseVirtualDtor 
>> The
>>  style system has an optimization that intentionally violates this for 
>> performance reasons (StyleRuleBase).

So are we saying we must not do that optimization, or will we have a way to 
suppress the warning for this?

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Darin Adler]

> On Sep 16, 2020, at 11:32 PM, Ryosuke Niwa  wrote:
> Every data member to a ref counted object must use either Ref, RefPtr, or 
> WeakPtr. webkit.NoUncountedMemberChecker 
> My
>  only worry here is performance. Do we know yet if we can afford it?

The worst case here is Ref, which is much worse than a reference since we end 
up having to use -> instead of . everywhere and you can’t see that there is no 
null involved.
> Every ref counted base class, if it has a derived class, must define a 
> virtual destructor. webkit.RefCntblBaseVirtualDtor 
> The
>  style system has an optimization that intentionally violates this for 
> performance reasons (StyleRuleBase).
> 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 
> What
>  is a non-trivial function?
> Every ref counted object must be captured using Ref or RefPtr for lambda. 
> webkit.UncountedLambdaCapturesChecker 
> Ref,
>  RefPtr, or WeakPtr, right?

Same concern about Ref vs references.
> Local variables - we’re still working on this 
> (https://reviews.llvm.org/D83259 )
I am looking forward to learning more about the proposal here.

Same concern about Ref vs. references.

I really want to see before/after for some non-trivial source files with 
significant problems; where will this drive the most change and what will 
things look like after?

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa]

On Thu, Sep 17, 2020 at 2:51 AM Emilio Cobos Álvarez 
wrote:

> Interesting. This looks fairly similar to some of the checkers we use in
> mozilla-central, fwiw.
>
> One interesting difference is that we opted for explicitly annotating
> the functions that can run script (think updateStyleIfNeeded(),
> dispatchEvent() etc equivalents) to be able to not warn for cases where
> using raw pointers is fine. See [1] for the current rules we're using.
>

That's an interesting alternative. We should consider that if the current
approach proves to cause perf issues.

So, I wonder... for a concrete example like [2], what is what would
> allow you to use shadowHost() without storing it on a local RefPtr
> otherwise, for example? Or is the plan to either pay the refcount churn,
> or silence the warnings on a per-case basis?
>

In this case, we'd like to store shadowHost() on a local RefPtr. "contains"
might be a common enough function that we could consider marking as safe
but in more generic case, there is no guarantee that contains would never
trigger a style or layout update, either of which may execute arbitrary
scripts. Also, script execution isn't the only way to get to memory
unsafely. Any piece of code that manipulates a complex enough data
structure may lead to use-after-free even if there was no arbitrary script
execution involved.

[1]:
>
> https://searchfox.org/mozilla-central/rev/f4b4008f5ee00f5afa3095f48c54f16828e4b22b/build/clang-plugin/CanRunScriptChecker.cpp#5-49
> [2]:
>
> https://webkit-search.igalia.com/webkit/rev/4c54a6d287d7fba30e1fb37d5afda692fb12a758/Source/WebCore/dom/Node.cpp#1041


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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa]

On Thu, Sep 17, 2020 at 2:11 AM Konstantin Tokarev 
wrote:

>
> Sounds great! A few questions:
> * Do I understand correctly that analyzer is a part of upstream clang and
> can work on any platform?
>

Yes.

* Does it require WebKit trunk or can work with older branches?


It should work with older branches as well as trunk since it doesn't rely
on any code-level annotations for now.

However, the plan is to add some annotations in the future to suppress
warnings where appropriate although we're hoping that we wouldn't need them
in too many places.

* Any plans to detect other kinds of misuses like circular references?
>

Not for now. For now, our primary focus is to detect dangerous use of
RefCounted objects and possibly any objects that can make WeakPtr in the
future.

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Emilio Cobos Álvarez]

Interesting. This looks fairly similar to some of the checkers we use in 
mozilla-central, fwiw.


One interesting difference is that we opted for explicitly annotating 
the functions that can run script (think updateStyleIfNeeded(), 
dispatchEvent() etc equivalents) to be able to not warn for cases where 
using raw pointers is fine. See [1] for the current rules we're using.


So, I wonder... for a concrete example like [2], what is what would 
allow you to use shadowHost() without storing it on a local RefPtr 
otherwise, for example? Or is the plan to either pay the refcount churn, 
or silence the warnings on a per-case basis?


Cheers,

  -- Emilio

[1]: 
https://searchfox.org/mozilla-central/rev/f4b4008f5ee00f5afa3095f48c54f16828e4b22b/build/clang-plugin/CanRunScriptChecker.cpp#5-49
[2]: 
https://webkit-search.igalia.com/webkit/rev/4c54a6d287d7fba30e1fb37d5afda692fb12a758/Source/WebCore/dom/Node.cpp#1041


On 9/17/20 8:32 AM, Ryosuke Niwa wrote:

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


 2. Every ref counted base class, if it has a derived class, must define
a virtual destructor. webkit.RefCntblBaseVirtualDtor


 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


 4. Every ref counted object must be captured using Ref or RefPtr for
lambda. webkit.UncountedLambdaCapturesChecker


 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 

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Konstantin Tokarev]

17.09.2020, 09:35, "Ryosuke Niwa" :
> 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
>
> * Every data member to a ref counted object must use either Ref, RefPtr, or 
> WeakPtr. webkit.NoUncountedMemberChecker
> * Every ref counted base class, if it has a derived class, must define a 
> virtual destructor. webkit.RefCntblBaseVirtualDtor
> * 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
> * Every ref counted object must be captured using Ref or RefPtr for lambda. 
> webkit.UncountedLambdaCapturesChecker
> * 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, then it would result in an undefined 
> behavior.
>
> struct A : public RefCounted {
>         Vector someData;
> };
>
> struct B : public A {
>         Vector 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((), 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 originalForm = m_form.get();
>     setForm(findAssociatedForm((), originalForm.get()));
>     HTMLElement& element = asHTMLElement();
>     auto* newForm = m_form.get();
>     if (newForm && newForm != 

[webkit-dev] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa]

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
   

   2. Every ref counted base class, if it has a derived class, must define
   a virtual destructor. webkit.RefCntblBaseVirtualDtor
   

   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
   

   4. Every ref counted object must be captured using Ref or RefPtr for
   lambda. webkit.UncountedLambdaCapturesChecker
   

   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, then it would result in an
undefined behavior.

struct A : public RefCounted {
Vector someData;
};

struct B : public A {
Vector 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((), 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 originalForm = m_form.get();
setForm(findAssociatedForm((),