[webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa]

> On Jan 12, 2023, at 8:37 PM, Darin Adler <darin at apple.com> wrote:
> 
>> On Jan 12, 2023, at 9:35 PM, Ryosuke Niwa <rniwa at apple.com> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20230112/cd613591/attachment.htm>