Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
> On Jan 12, 2023, at 9:05 PM, Ryosuke Niwa via webkit-dev
> wrote:
>
>
>> On Jan 12, 2023, at 8:37 PM, Darin Adler wrote:
>>
>>> On Jan 12, 2023, at 9:35 PM, Ryosuke Niwa wrote:
>>>
>>> One alternative is to make bar() return RefPtr although that would be a bit
>>> heavy handed in the case of trivial function calls like this:
>>> document().frame()->ownerElement()
>>
>> I don’t quite follow. You just said that all arguments including this need
>> to have RefPtr or something like it.
>
> Right. The caller should keep an object alive.
>
>> What makes it OK to call ownerElement without a RefPtr? What is a “trivial
>> function” and how can we tell which functions are the trivial ones?
>
> We made an exception to “trivial functions” to allow chaining like the one
> above possible. A trivial function is any inlined accessor of a member
> variable the form: Document& document() { return m_document.get(); } or
> Frame* frame() { return m_frame.get(); }
>
> Anything more complicated than that, or for any out-of-lined functions, we
> should be storing “this” on a smart pointer type.
>
>
> Here’s an example:
> static void printNavigationErrorMessage(Frame& frame, const URL& activeURL,
> const char* reason)
> {
> String message = "Unsafe JavaScript attempt to initiate navigation for
> frame with URL '" + frame.document()->url().string() + "' from frame with URL
> '" + activeURL.string() + "'. " + reason + "\n";
>
> // FIXME: should we print to the console of the document performing the
> navigation instead?
> frame.document()->domWindow()->printErrorMessage(message);
> }
>
> This function, under our rule, should be rewritten like this:
> static void printNavigationErrorMessage(Frame& frame, const URL& activeURL,
> const char* reason)
> {
> String message = "Unsafe JavaScript attempt to initiate navigation for
> frame with URL '" + frame.document()->url().string() + "' from frame with URL
> '" + activeURL.string() + "'. " + reason + "\n";
>
> // FIXME: should we print to the console of the document performing the
> navigation instead?
> RefPtr { frame.document()->domWindow() }->printErrorMessage(message);
> }
>
> Here, it’s okay to call document() without first storing “frame” in a smart
> pointer because it is a function argument. By transitive property, the caller
> of this function should be keeping the frame object alive. It’s okay to call
> domWindow() because it’s a trivial accessor function of the form:
>
> DOMWindow* domWindow() const { return m_domWindow.get(); }
>
> But it’s not okay to call printErrorMessage without first storing DOMWindow
> in a RefPtr because it’s a non-trivial function, and domWindow() was not an
> argument to this function so there is no caller to guarantee the lifetime of
> it.
>
>
> Here’s another example:
> int Element::clientLeft()
> {
> document().updateLayoutIgnorePendingStylesheets();
>
> if (auto* renderer = renderBox()) {
> auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
> return
> convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft,
> *renderer).toDouble());
> }
> return 0;
> }
>
> This function should be rewritten to something like this assuming
> RenderObject is updated to support CheckedPtr:
> int Element::clientLeft()
> {
> Ref { document() }->updateLayoutIgnorePendingStylesheets();
>
> if (CheckedPtr renderer = renderBox()) {
> auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
> return
> convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft,
> *renderer).toDouble());
> }
> return 0;
> }
>
> Here, we must store document() in Ref before calling
> updateLayoutIgnorePendingStylesheets because
> updateLayoutIgnorePendingStylesheets is a non-trivial function, and
> document() is not one of the arguments. Similarly, we need to store
> renderBox() in a smart pointer since clientLeft() is a non-trivial function.
> But we don’t have to store “this” in Ref/RefPtr before calling
> convertToNonSubpixelValue because “this” is an implicit function argument.
Oops, correction. convertToNonSubpixelValue is a static function which takes
double & LegacyCSSOMElementMetricsRoundingStrategy so this is irrelevant there.
- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
> On Jan 12, 2023, at 8:37 PM, Darin Adler wrote:
>
>> On Jan 12, 2023, at 9:35 PM, Ryosuke Niwa wrote:
>>
>> One alternative is to make bar() return RefPtr although that would be a bit
>> heavy handed in the case of trivial function calls like this:
>> document().frame()->ownerElement()
>
> I don’t quite follow. You just said that all arguments including this need to
> have RefPtr or something like it.
Right. The caller should keep an object alive.
> What makes it OK to call ownerElement without a RefPtr? What is a “trivial
> function” and how can we tell which functions are the trivial ones?
We made an exception to “trivial functions” to allow chaining like the one
above possible. A trivial function is any inlined accessor of a member variable
the form: Document& document() { return m_document.get(); } or Frame* frame() {
return m_frame.get(); }
Anything more complicated than that, or for any out-of-lined functions, we
should be storing “this” on a smart pointer type.
Here’s an example:
static void printNavigationErrorMessage(Frame& frame, const URL& activeURL,
const char* reason)
{
String message = "Unsafe JavaScript attempt to initiate navigation for
frame with URL '" + frame.document()->url().string() + "' from frame with URL
'" + activeURL.string() + "'. " + reason + "\n";
// FIXME: should we print to the console of the document performing the
navigation instead?
frame.document()->domWindow()->printErrorMessage(message);
}
This function, under our rule, should be rewritten like this:
static void printNavigationErrorMessage(Frame& frame, const URL& activeURL,
const char* reason)
{
String message = "Unsafe JavaScript attempt to initiate navigation for
frame with URL '" + frame.document()->url().string() + "' from frame with URL
'" + activeURL.string() + "'. " + reason + "\n";
// FIXME: should we print to the console of the document performing the
navigation instead?
RefPtr { frame.document()->domWindow() }->printErrorMessage(message);
}
Here, it’s okay to call document() without first storing “frame” in a smart
pointer because it is a function argument. By transitive property, the caller
of this function should be keeping the frame object alive. It’s okay to call
domWindow() because it’s a trivial accessor function of the form:
DOMWindow* domWindow() const { return m_domWindow.get(); }
But it’s not okay to call printErrorMessage without first storing DOMWindow in
a RefPtr because it’s a non-trivial function, and domWindow() was not an
argument to this function so there is no caller to guarantee the lifetime of it.
Here’s another example:
int Element::clientLeft()
{
document().updateLayoutIgnorePendingStylesheets();
if (auto* renderer = renderBox()) {
auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
return
convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft,
*renderer).toDouble());
}
return 0;
}
This function should be rewritten to something like this assuming RenderObject
is updated to support CheckedPtr:
int Element::clientLeft()
{
Ref { document() }->updateLayoutIgnorePendingStylesheets();
if (CheckedPtr renderer = renderBox()) {
auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
return
convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft,
*renderer).toDouble());
}
return 0;
}
Here, we must store document() in Ref before calling
updateLayoutIgnorePendingStylesheets because
updateLayoutIgnorePendingStylesheets is a non-trivial function, and document()
is not one of the arguments. Similarly, we need to store renderBox() in a smart
pointer since clientLeft() is a non-trivial function. But we don’t have to
store “this” in Ref/RefPtr before calling convertToNonSubpixelValue because
“this” is an implicit function argument.
- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
> On Jan 12, 2023, at 9:35 PM, Ryosuke Niwa wrote: > > One alternative is to make bar() return RefPtr although that would be a bit > heavy handed in the case of trivial function calls like this: > document().frame()->ownerElement() I don’t quite follow. You just said that all arguments including this need to have RefPtr or something like it. What makes it OK to call ownerElement without a RefPtr? What is a “trivial function” and how can we tell which functions are the trivial ones? — Darin___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
> On Jan 12, 2023, at 6:50 PM, Michael Catanzaro wrote:
>
> On Thu, Jan 12 2023 at 12:35:09 PM -0800, Ryosuke Niwa via webkit-dev
> wrote:
>> So… instead of:
>> foo(bar());
>> do:
>> foo(RefPtr { bar() }.get());
>
> What's the value of creating a temporary RefPtr just to get at the underlying
> raw pointer? Isn't this overkill?
The benefit is that bar() will be kept alive while the duration of call to foo.
Without, whatever bar() returns can be dead before foo() finishes running,
which can result in use-after-free.
An obvious alternative is to use smart pointer types on each function argument.
But this has a few drawbacks:
The same rule can’t be applied to “this” since passing of “this" pointer is
implicit in C++.
Ref churn when multiple functions are called with the same object; e.g.
foo = foo()
bar(foo); // ref/deref here
baz(foo); // ref/deref here again
Ref churn when a function argument is passed to another function; e.g.
void foo(RefPtr&& obj)
{
bar(obj); // ref/deref here even though obj is guaranteed to be alive
throughout this function
}
- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
On Thu, Jan 12 2023 at 12:35:09 PM -0800, Ryosuke Niwa via webkit-dev
wrote:
So… instead of:
foo(bar());
do:
foo(RefPtr { bar() }.get());
What's the value of creating a temporary RefPtr just to get at the
underlying raw pointer? Isn't this overkill?
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
> On Jan 12, 2023, at 1:27 PM, Darin Adler wrote:
>
>> On Jan 12, 2023, at 3:35 PM, Ryosuke Niwa via webkit-dev
>> wrote:
>>
>>> On Jan 12, 2023, at 6:13 AM, Darin Adler wrote:
>>>
On Jan 12, 2023, at 12:21 AM, Ryosuke Niwa via webkit-dev
wrote:
assuming every local variable / variable in stack is stored in a smart
pointer, function arguments are safe to be raw pointers / references via
transitive property
>>>
>>> What about the case where the function argument is the return value from
>>> another function?
>>
>> In those cases, the value should be stored in a local variable using a smart
>> pointer first.
>>
>> So… instead of:
>> foo(bar());
>>
>> do:
>> foo(RefPtr { bar() }.get());
>
> This seems impractical. How will I remember to do this?
The smart pointer analysis tool I've been working with the clang team should be
able to tell you that once it becomes available. Until then, we probably need
to rely on reviewers to notice these patterns.
One alternative is to make bar() return RefPtr although that would be a bit
heavy handed in the case of trivial function calls like this:
document().frame()->ownerElement()
- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
> On Jan 12, 2023, at 3:35 PM, Ryosuke Niwa via webkit-dev
> wrote:
>
>> On Jan 12, 2023, at 6:13 AM, Darin Adler wrote:
>>
>>> On Jan 12, 2023, at 12:21 AM, Ryosuke Niwa via webkit-dev
>>> wrote:
>>>
>>> assuming every local variable / variable in stack is stored in a smart
>>> pointer, function arguments are safe to be raw pointers / references via
>>> transitive property
>>
>> What about the case where the function argument is the return value from
>> another function?
>
> In those cases, the value should be stored in a local variable using a smart
> pointer first.
>
> So… instead of:
> foo(bar());
>
> do:
> foo(RefPtr { bar() }.get());
This seems impractical. How will I remember to do this?
— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
> On Jan 12, 2023, at 6:13 AM, Darin Adler wrote:
>
>> On Jan 12, 2023, at 12:21 AM, Ryosuke Niwa via webkit-dev
>> wrote:
>>
>> assuming every local variable / variable in stack is stored in a smart
>> pointer, function arguments are safe to be raw pointers / references via
>> transitive property
>
> What about the case where the function argument is the return value from
> another function?
In those cases, the value should be stored in a local variable using a smart
pointer first.
So… instead of:
foo(bar());
do:
foo(RefPtr { bar() }.get());
- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Stop Using Raw Pointers & References in New Code
> On Jan 12, 2023, at 12:21 AM, Ryosuke Niwa via webkit-dev > wrote: > > assuming every local variable / variable in stack is stored in a smart > pointer, function arguments are safe to be raw pointers / references via > transitive property What about the case where the function argument is the return value from another function? — Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
