[webkit-reviews] review denied: [Bug 47688] Add a way to detect that a RenderObject* is being used across methods that might cause it to be destroyed : [Attachment 73031] added assertion to updateStyleIfNeeded and updateStyleForAllDocuments

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 47688: Add a way to detect that a RenderObject* is being used across
methods that might cause it to be destroyed
https://bugs.webkit.org/show_bug.cgi?id=47688

Attachment 73031: added assertion to updateStyleIfNeeded and
updateStyleForAllDocuments
https://bugs.webkit.org/attachment.cgi?id=73031&action=review

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

I’m going to say review- for now, but I think it’s OK to land this or something
like it.

> WebCore/dom/Document.cpp:1575
> +	   ASSERT(!doc->isRendererDestructionForbidden());
>	   documentsThatNeedStyleRecalc->remove(doc);
>	   doc->updateStyleIfNeeded();

This seems unhelpful written this way; updateStyleIfNeeded is already called
and will assert immediately. Adding another assertion outside the function adds
little.

The issue is documents that are not in the documentsThatNeedStyleRecalc
collection at all. When this function is called we need to assert that no
renderer destruction is happening on any documents. Maybe we can make “forbid
renderer destruction” global instead of per-document. I think that would work
better unless we find it’s impractical.

> WebCore/dom/Document.h:520
> +#ifndef NDEBUG
> +    void forbidRendererDestruction() { ++m_rendererGuardCount; }
> +    void allowRendererDestruction() { ASSERT(m_rendererGuardCount);
--m_rendererGuardCount; }
> +    bool isRendererDestructionForbidden() const { return
m_rendererGuardCount; }
> +#else
> +    void forbidRendererDestruction() {}
> +    void allowRendererDestruction() {}
> +    bool isRendererDestructionForbidden() const { return false; }
> +#endif

I think it’s cleaner to put the function *definitions* outside the class so we
don’t need #ifndef here in the class definition. Like this:

    void forbidRendererDestruction();

...

inline void Document::forbidRendererDestruction()
{
#ifndef NDEBUG
    ++m_rendererGuardCount;
#endif
}

> WebCore/page/FrameView.cpp:1795
> +    if (!elt->renderer())
> +	   return clipRect;

This change should land separately. It’s a not-directly-related bug fix and
needs a test case.

> WebCore/rendering/RenderObject.cpp:236
> +    ForbidRenderObjectDestruction forbidDestructionOf(this);

It’s kind of cute the way this makes a sentence. But I don’t like the fact that
a class is named with a verb. Normally we want class names to be noun phrases.

> WebCore/rendering/RenderObject.h:112
> +    ForbidRenderObjectDestruction(const RenderObject* object);

Should omit the argument name here.

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

I would put the private: inside the #ifndef myself.