[webkit-reviews] review granted: [Bug 47688] Add layout guard : [Attachment 70820] removed guards that caused crashes on the existing layout tests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 47688: Add layout guard
https://bugs.webkit.org/show_bug.cgi?id=47688

Attachment 70820: removed guards that caused crashes on the existing layout
tests
https://bugs.webkit.org/attachment.cgi?id=70820&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=70820&action=review

Maybe what we need for pointers other than this is a sort of smart pointer for
renderers. We’d use that pointer as the type instead of raw pointers
everywhere. Using the pointer after layout would lead to an assertion. That
would probably be easier to use correctly than these guards.

I’m going to say review+ but I think this is something we need to consider. I
don’t want to put a lot of confusing lines into our code so I’m a little wary
about this.

> WebCore/dom/Document.h:508
> +#ifndef NDEBUG
> +    void forbidLayout() { ++m_layoutGuardCount; }
> +    void allowLayout() { ASSERT(m_layoutGuardCount); --m_layoutGuardCount; }

> +    bool isLayoutForbidden() const { return m_layoutGuardCount; }
> +#endif

I prefer a style where the forbid/allow functions are always present, but empty
inline functions. This can cut down on the #ifdefs needed at call sites. Would
you consider doing it that way?

> WebCore/rendering/RenderObject.cpp:236
> +    RenderObjectLayoutGuard guard(this);

I think these are a little bit mysterious. How do we know where to put these
guards? The name is just vague enough that it’s difficult to understand why you
would add one somewhere. It would be good if the name was somehow more closely
tied with the concept of “I am holding a pointer to a renderer”.

Adding one to a function that says document()->page()->theme() seems pretty
mysterious.

I don’t think the fact that these functions are member functions is the key to
why we need this guarding.

I think that adding these guards to all these member functions is both too much
(adds them to functions where they clearly provide no value) and too little
(any function that holds and then reuses a pointer to a renderer needs a
similar guard, whether it’s the this pointer in a member function, or any other
way of holding a pointer).

> WebCore/rendering/RenderObject.h:117
> +#ifndef NDEBUG
> +    ~RenderObjectLayoutGuard();
> +private:
> +    const RenderObject* m_object;
> +#endif

It would be cleaner to just have the #ifndef around the data member. It would
be fine to have an inline empty destructor below. Keeping the class definition
cleaner makes sense to me.