Bug 47688 – Add a way to detect that a RenderObject* is being used across methods that might cause it to be destroyed

• Status: RESOLVED
• Resolution: LATER
• Component: Layout and Rendering

[Ryosuke Niwa]

We need a mechanism to detect unintentional layout triggered inside a member function of RenderObject and its derived classes.

[Ryosuke Niwa]

Created attachment 70789 [details]
implements the guard

[Ryosuke Niwa]

Just a note: I've already found a couple of crashes while making this patch.  I've removed guards from the functions that caused crashes to fix it in separate patches.  But I think this proves the point of adding this guard.

[Simon Fraser (smfr)]

Comment on attachment 70789 [details]
implements the guard

View in context: https://bugs.webkit.org/attachment.cgi?id=70789&action=review

> WebCore/ChangeLog:13
> +        We intend to instantiate RenderObjectLayoutGuard in all non-trivial member functions of RenderObject
> +        and its subclasses so that we detect layouts that happen while executing the member functions.

I wish we could do this in a way that wasn't so invasive. I'm not sure what that would be though.

> WebCore/dom/Document.h:506
> +    void guardLayout() { ++m_layoutGuardCount; }
> +    void unguardLayout() { --m_layoutGuardCount; }

This should ASSERT that the count doesn't go negative.

I don't really like the name. Maybe enterLayoutGuard/exitLayoutGuard?

> WebCore/dom/Document.h:507
> +    bool shouldAllowLayout() const { return !m_layoutGuardCount; }

inLayoutGuard()?

> WebCore/rendering/RenderObject.cpp:237
> +

Do this get optimized away to nothing in release builds? Maybe a macro would be better?

[Simon Fraser (smfr)]

We use the term "isForbidden" elsewhere.

[Eric Seidel (no email)]

Comment on attachment 70789 [details]
implements the guard

View in context: https://bugs.webkit.org/attachment.cgi?id=70789&action=review

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

We don't generally use const pointers.

"object" is superfluous here.

[Ryosuke Niwa]

Comment on attachment 70789 [details]
implements the guard

View in context: https://bugs.webkit.org/attachment.cgi?id=70789&action=review

>> WebCore/ChangeLog:13
>> +        and its subclasses so that we detect layouts that happen while executing the member functions.
> 
> I wish we could do this in a way that wasn't so invasive. I'm not sure what that would be though.

I wish that too.  On the bright side, RenderObjectLayoutGuard is flexible enough that it can be used for other purposes in the future.

>> WebCore/dom/Document.h:506
>> +#ifndef NDEBUG
>> +    void guardLayout() { ++m_layoutGuardCount; }
>> +    void unguardLayout() { --m_layoutGuardCount; }
> 
> This should ASSERT that the count doesn't go negative.
> 
> I don't really like the name. Maybe enterLayoutGuard/exitLayoutGuard?

Will fix.

>> WebCore/dom/Document.h:507
>> +    bool shouldAllowLayout() const { return !m_layoutGuardCount; }
> 
> inLayoutGuard()?

Will fix.

>> WebCore/rendering/RenderObject.cpp:237
>> +    RenderObjectLayoutGuard guard(this);
>> +
> 
> Do this get optimized away to nothing in release builds? Maybe a macro would be better?

Yes, I believe so.  RenderObjectLayoutGuard has no member variable and has an empty constructor in release build.  I'd expect any good optimizing compiler to get rid of it (probably in dead code/store elimination).

[Ryosuke Niwa]

(In reply to comment #5)
> > WebCore/rendering/RenderObject.h:112
> > +    RenderObjectLayoutGuard(const RenderObject* object);
> 
> We don't generally use const pointers.
> "object" is superfluous here.

const is needed in some call sites.  We could get rid of those const as well but I rather not do it in this patch.

[Darin Adler]

A similar mechanism is forbid/allowEventDispatch in EventTarget.h.

[Ryosuke Niwa]

(In reply to comment #4)
> We use the term "isForbidden" elsewhere.

(In reply to comment #8)
> A similar mechanism is forbid/allowEventDispatch in EventTarget.h.

Would it be better call them forbidLayout, allowLayout, and isLayoutForbidden then?

[Ryosuke Niwa]

(In reply to comment #6)
> >> WebCore/rendering/RenderObject.cpp:237
> >> +    RenderObjectLayoutGuard guard(this);
> >> +
> > 
> > Do this get optimized away to nothing in release builds? Maybe a macro would be better?
> 
> Yes, I believe so.  RenderObjectLayoutGuard has no member variable and has an empty constructor in release build.  I'd expect any good optimizing compiler to get rid of it (probably in dead code/store elimination).

Per jamesr's suggestion, I did verify that the disassembled code are the same before and after adding RenderObjectLayoutGuard (I tested with RenderTextControl::selection).

[Ryosuke Niwa]

Created attachment 70816 [details]
fixed per comments

[Ryosuke Niwa]

James suggested me to have a macro FORBID_LAYOUT defined as:

#ifndef NDEBUG
#define FORBID_LAYOUT(obj) RenderObjectLayoutGuard guard(obj)
#else
#define FORBID_LAYOUT(obj)

I had considered this as well but this macro still requires an argument and I didn't like the fact that it implicitly pollutes the local namespace.  I could change the name "guard" to something more unique like renderObjectLayoutGuard though.  Do other people have preferences?

[Ryosuke Niwa]

I need to remove the guard from repaint.  fast/events/key-events-in-input-button.html hits the assert otherwise.  This guard catches all sort of errors.

Now I found out that plugins/createScriptableObject-before-start.html triggers a bug in FrameView::windowClipRect.

RenderLayer* layer = elt->renderer()->enclosingLayer();

Here, renderer() is null and enclosingLayer() is called with this=0.  We magically survives because of
    const RenderObject* curr = this;
    while (curr) {

[Ryosuke Niwa]

Created attachment 70820 [details]
removed guards that caused crashes on the existing layout tests

[Darin Adler]

Comment on attachment 70820 [details]
removed guards that caused crashes on the existing layout tests

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.

[Simon Fraser (smfr)]

(In reply to comment #15)
> (From update of attachment 70820 [details])
> 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.

Oooh I like this idea.

[Ryosuke Niwa]

(In reply to comment #15)
> 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.

Are you saying we add in addition this guard?  Or are you saying that we move the guard into / implement alternative mechanism in the smart pointer?

> 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.

That's my only concern as well and jamesr and dhaytt have pointed out that people will forget adding this or may accidentally remove the guard.

> > 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?

Sure.  Will fix.

> > 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”.

Something like ForbidLayout forbidLayout(this) might work better ?

> 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).

However, it's hard to identify each function / portion of code that needs a guard.  Since guard adds very slight performance regression (we shouldn't be adding to inline functions, etc... that can't possibly cause a layout), I think it's okay to do too much.

The smart pointer helps but I don't think it covers the case where caller just uses renderer() once and layout happens within the renderer's function.

> > 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.

Will do.

(In reply to comment #16)
> > 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.
> 
> Oooh I like this idea.

+1.

[Eric Seidel (no email)]

I'm not sure I understand the smart pointer idea.

RenderPtr<RenderObject> renderer;

We'd override RenderPtr::operator->? to do the guarding for us?

If we deployed this everywhere wouldn't we hit the guards during layout then?

[Darin Adler]

(In reply to comment #17)
> (In reply to comment #15)
> > 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.
> 
> Are you saying we add in addition this guard?  Or are you saying that we move the guard into / implement alternative mechanism in the smart pointer?

At first I thought the smart pointer was an alternative to the guard, clearly a smart pointer doesn’t do any good for the “this” pointer; I think we need a proposal that covers both. RefPtr has the same problem with “this” pointers.

> > 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.
> 
> That's my only concern as well and jamesr and dhaytt have pointed out that people will forget adding this or may accidentally remove the guard.

Absolutely true. Not clear what to do.

> > 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”.
> 
> Something like ForbidLayout forbidLayout(this) might work better ?

I think the concept “forbid layout” is wrong. I think the concept is that we are holding on to a renderer pointer, and we need to forbid anything that could destroy the renderer out from under us.

> > 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).
> 
> However, it's hard to identify each function / portion of code that needs a guard. 

We need to explain where the guard is needed. Can you state the rule you think we are following?

> Since guard adds very slight performance regression

The guard does *not* add a performance regression to non-debug builds.

> The smart pointer helps but I don't think it covers the case where caller just uses renderer() once and layout happens within the renderer's function.

Actually, it *almost* does. If renderer() returns a smart pointer, that smart pointer isn’t destroyed until after the function returns. If we put the assertion into the smart pointer destructor, so that having a smart pointer around causes the assertion even if you don’t use the pointer, then the smart pointer would do the trick even without any changes inside member functions.

I think the smart pointer approach might work really well.

[Darin Adler]

(In reply to comment #18)
> RenderPtr<RenderObject> renderer;
> 
> We'd override RenderPtr::operator->? to do the guarding for us?

We might do the guarding during the lifetime of RenderPtr instead of just in ->. That would require being more careful with the lifetime of local variables.

> If we deployed this everywhere wouldn't we hit the guards during layout then?

I don’t think we would. The guard check would happen at the start of layout. Finishing layout doesn’t involve starting a new layout, and in fact if we started a new layout part way through while holding renderer pointers from the first layout, that would be a bad bug that it’s good for the check to catch.

[Eric Seidel (no email)]

(In reply to comment #19)
> I think the concept “forbid layout” is wrong. I think the concept is that we are holding on to a renderer pointer, and we need to forbid anything that could destroy the renderer out from under us.

Layout should be the only time renders are being created/destroyed.  But it's true, there are more-surgical protections possible.  Including simply checking the global guard in ~RenderObject() instead of layout(), or even more surgical: setting a bool on just the one renderer needing protection and checking that bool in ~RenderObject().

The advantage of checking in layout(), is that you catch these types of bugs even when we don't necessarily have a layout test which would expose the crash.  Either of the two ways I mentioned above, would only ever catch bugs in Debug mode which would otherwise crash in Release mode. :)  The layout() check, should catch a whole class of bugs in Debug mode, which could crash with more devious HTML than we may have in our LayoutTests.

Obviously if the layout() check catches any false-positives it's useless.  Or similarly, if there are other non-layout() ways to destroy renderers, than that's a problem.

[Eric Seidel (no email)]

By "layout()" in the above, I really mean layout() and resolveStyle() since resolveStyle() is where renderers get created/destroyed if I remember correctly.

[James Robinson]

I would really like to see some of the bugs this guard can catch and try to figure out what checks will help us find bugs and what checks are less likely to.  I think you've mentioned finding some tests that fail with this on, Ryosuke, could you link those bugs here or post some stack traces?

[Darin Adler]

(In reply to comment #22)
> By "layout()" in the above, I really mean layout() and resolveStyle() since resolveStyle() is where renderers get created/destroyed if I remember correctly.

Eric, from what you’re saying it seems you do not understand my RendererPtr idea.

The point of RendererPtr is not to use it when creating renderers. It’s to use it when holding a pointer to a renderer, to assert that an operation that can destroy renderers is not begun while you are still holding that pointer.

I don’t think there’s any reason to restrict the guard to layout. Any operation that can destroy renderers should trigger the assertion. That would include layout and document destruction.

[Darin Adler]

(In reply to comment #23)
> I would really like to see some of the bugs this guard can catch and try to figure out what checks will help us find bugs and what checks are less likely to.  I think you've mentioned finding some tests that fail with this on, Ryosuke, could you link those bugs here or post some stack traces?

James, I think it’s pretty clear that all of these guards will help us find the bugs. We really don’t have a wide variety of options here. Ryosuke’s first effort immediately found problems, so I think we have few doubts this will be effective.

What we’re debating is how to do this with a clear easy to follow rule.

[Ryosuke Niwa]

(In reply to comment #19)
> At first I thought the smart pointer was an alternative to the guard, clearly a smart pointer doesn’t do any good for the “this” pointer; I think we need a proposal that covers both. RefPtr has the same problem with “this” pointers.

> > The smart pointer helps but I don't think it covers the case where caller just uses renderer() once and layout happens within the renderer's function.
> 
> Actually, it *almost* does. If renderer() returns a smart pointer, that smart pointer isn’t destroyed until after the function returns. If we put the assertion into the smart pointer destructor, so that having a smart pointer around causes the assertion even if you don’t use the pointer, then the smart pointer would do the trick even without any changes inside member functions.

Right.  But I think smart pointer detects the fault too late because if layout happens inside render's method, we're already in danger by the time we come back to the method.  I bet we still hit many memory access violation or worse corrupt memory with smart pointer.

> > > 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”.
> > 
> > Something like ForbidLayout forbidLayout(this) might work better ?
> 
> I think the concept “forbid layout” is wrong. I think the concept is that we are holding on to a renderer pointer, and we need to forbid anything that could destroy the renderer out from under us.

Right.  I originally wanted to add a counter to each and every render object.  And just call the guard as RenderObjectGuard and we check in destroy().  But that wouldn't detect bad layouts that doesn't delete the render object in the call stack so it has a limited usage.

> > > 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).
> > 
> > However, it's hard to identify each function / portion of code that needs a guard. 
> 
> We need to explain where the guard is needed. Can you state the rule you think we are following?

I followed a rule of adding a guard to any non-inline function that calls non-inline function.  I might have missed some along the way though since there are so many functions in RenderObject.

> > Since guard adds very slight performance regression
> 
> The guard does *not* add a performance regression to non-debug builds.

I meant for the debug build.  And you're right, I made sure the guard doesn't add any performance regression to release builds.

[Eric Seidel (no email)]

(In reply to comment #24)
> (In reply to comment #22)
> > By "layout()" in the above, I really mean layout() and resolveStyle() since resolveStyle() is where renderers get created/destroyed if I remember correctly.
> 
> Eric, from what you’re saying it seems you do not understand my RendererPtr idea.

This is correct.

> The point of RendererPtr is not to use it when creating renderers. It’s to use it when holding a pointer to a renderer, to assert that an operation that can destroy renderers is not begun while you are still holding that pointer.

This makes much more sense.  This would be an additional Debug-only bool on the renderer which is set by the RenderPtr, it sounds like.  A hash could also work of course.

> I don’t think there’s any reason to restrict the guard to layout. Any operation that can destroy renderers should trigger the assertion. That would include layout and document destruction.

This would turn possible Release crashers in to Debug ASSERTs, which is good, and definitely a step forward.  It won't catch as many possible bugs as solutions like a layout guard (which may be too imprecise as mentioned above!) or making it impossible to include certain headers from rendering/.

I believe I now understand your proposal, and support it.  Whether instead-of, or in-addition-to a layout guard I don't know.  It certainly is more precise and will catch an overlapping but slightly different (and smaller?) set of bugs.

[James Robinson]

(In reply to comment #25)
> (In reply to comment #23)
> > I would really like to see some of the bugs this guard can catch and try to figure out what checks will help us find bugs and what checks are less likely to.  I think you've mentioned finding some tests that fail with this on, Ryosuke, could you link those bugs here or post some stack traces?
> 
> James, I think it’s pretty clear that all of these guards will help us find the bugs. We really don’t have a wide variety of options here. Ryosuke’s first effort immediately found problems, so I think we have few doubts this will be effective.
> 
> What we’re debating is how to do this with a clear easy to follow rule.

Right, but the earlier we can catch an error the better.  If this guard ends up catching mostly bugs that are concentrated in certain areas of the codebase, then that might point to a more targeted mechanism that could get us as much ore more bang for our proverbial buck.

Ideally our design would be such that it would be immediately obvious when writing new code what functions are safe to call and what functions are not.

[Darin Adler]

(In reply to comment #26)
> (In reply to comment #19)
> > At first I thought the smart pointer was an alternative to the guard, clearly a smart pointer doesn’t do any good for the “this” pointer; I think we need a proposal that covers both. RefPtr has the same problem with “this” pointers.
> 
> > > The smart pointer helps but I don't think it covers the case where caller just uses renderer() once and layout happens within the renderer's function.
> > 
> > Actually, it *almost* does. If renderer() returns a smart pointer, that smart pointer isn’t destroyed until after the function returns. If we put the assertion into the smart pointer destructor, so that having a smart pointer around causes the assertion even if you don’t use the pointer, then the smart pointer would do the trick even without any changes inside member functions.
> 
> Right.  But I think smart pointer detects the fault too late because if layout happens inside render's method, we're already in danger by the time we come back to the method.  I bet we still hit many memory access violation or worse corrupt memory with smart pointer.

The smart pointer turns on the “forbid” flag for its entire lifetime. We hit the assertion deep inside the bad code, not in the smart pointer destructor.

> > I think the concept “forbid layout” is wrong. I think the concept is that we are holding on to a renderer pointer, and we need to forbid anything that could destroy the renderer out from under us.
> 
> Right.  I originally wanted to add a counter to each and every render object.  And just call the guard as RenderObjectGuard and we check in destroy().  But that wouldn't detect bad layouts that doesn't delete the render object in the call stack so it has a limited usage.

Separate counters for each render object is the wrong concept for the reason you mention here. It’s not just code that actually destroys the renderers, but code that potentially would in other cases that we want to detect. We need to forbid doing things that *can* destroy renderers.

> I followed a rule of adding a guard to any non-inline function that calls non-inline function.  I might have missed some along the way though since there are so many functions in RenderObject.

Why are inlines any different? Why are RenderObject member functions the right set.

> > > Since guard adds very slight performance regression
> > 
> > The guard does *not* add a performance regression to non-debug builds.
> 
> I meant for the debug build.  And you're right, I made sure the guard doesn't add any performance regression to release builds.

I don’t care about slight performance regressions in debug builds. We don’t even inline functions in debug builds!

[Darin Adler]

(In reply to comment #28)
> Right, but the earlier we can catch an error the better.  If this guard ends up catching mostly bugs that are concentrated in certain areas of the codebase, then that might point to a more targeted mechanism that could get us as much ore more bang for our proverbial buck.
> 
> Ideally our design would be such that it would be immediately obvious when writing new code what functions are safe to call and what functions are not.

I agree with your goals.

Dan Bernstein and Simon Fraser and I talked about this for a few minutes, but didn’t come to any quick conclusions.

[Simon Fraser (smfr)]

Here's one idea for what the smart pointer would look like (it would be templatized, of course)

class CheckedRenderObject
{
public:
    CheckedRenderObject(RenderObject* object)
        : m_renderer(object)
    {
        ...document()->incrementOutstandingRenderObjects();
    }
    
    ~CheckedRenderObject()
    {
        ...document()->decrementOutstandingRenderObjects();
    }
    
    RenderObject* operator*() const { return m_renderer; }
    
private:
    RenderObject* m_renderer;
}

void Document::recalcStyle(StyleChange change)
{
    ...
    ASSERT(!m_outstandingRenderObjectCount);
    ...
}

[Eric Seidel (no email)]

(In reply to comment #31)
> Here's one idea for what the smart pointer would look like (it would be templatized, of course)
> 
> class CheckedRenderObject

OK, we're all saying the same thing here.  There are two proposals.

1.  Simon's proposal is exactly the same as Ryosuke's, its just correcting my error of saying layout() to Ryosuke when I really should have been saying recalcStyle() since it's recalcStyle which does the creation/deletion of render objects.

2.  Darin's proposal is also useful, but will catch a possibly different set of bugs.  I should also point out that Darin's proposal will require an int on every RenderObject (or a hash of ptr->int), not just a flag.  It's basically a separate ref count used only for ASSERTs and not keeping the object alive.

I like both proposals.  Why not do both?  Simon/Ryosuke's is harder to deploy, but might catch more.  Darin's is more memory-expensive, but easier to deploy.

[Ryosuke Niwa]

Comment on attachment 70820 [details]
removed guards that caused crashes on the existing layout tests

Making my patch obsolete for now since there has been a lot of discussions as to how to implement this since darin r+ed.

[Eric Seidel (no email)]

There is one subtle difference between simon's and ryosuke's that Simon's is a smart pointer, and ryosuke's is just a scoping object.  Either works.  Ryosuke's can be deployed more places which don't need a local.  Simon's is easier to not forget about, but may end up incrementing/decrementing more than necessary.

[Darin Adler]

(In reply to comment #32)
> 2. I should also point out that Darin's proposal will require an int on every RenderObject (or a hash of ptr->int), not just a flag.  It's basically a separate ref count used only for ASSERTs and not keeping the object alive.

That’s not my proposal. My proposal does not involve adding any data members to RenderObject.

My proposal is to make a RenderPtr template class that does what Simon posted.

[Darin Adler]

(In reply to comment #34)
> There is one subtle difference between simon's and ryosuke's that Simon's is a smart pointer, and ryosuke's is just a scoping object.  Either works.  Ryosuke's can be deployed more places which don't need a local.  Simon's is easier to not forget about, but may end up incrementing/decrementing more than necessary.

Simon’s proposal is actually mine. As I said in my original suggestion, it doesn’t help for “this” pointers. I think there are important questions here you are not asking Eric.

These are the questions I see:

    1) What is forbidden? Ryosuke’s patch forbids layout. We should forbid operations that can destroy renderers, which includes calculating style, layout, and manipulating the DOM in a way that can destroy renderers. I suggest function naming and design should be about renderer destruction rather than layout.

    2) What is the mechanism for forbidding things? I suggest three layers, each built on the one below. The first layer would be a forbid/allow function pair. The second layer would be a C++ helper class to make it easy to match up forbid/allow. And the third layer would be a RenderPtr template that uses the helper class. Ryosuke’s patch had the first two of these.

    3) Where should the mechanism be deployed? Ryosuke’s patch deployed this in all non-inline RenderObject member functions that called non-inline functions. I don’t have any good ideas, but generally speaking it would be helpful to have this in any functions that have pointers to renderers that they use after calling non-trivial functions. It seems exciting to make Node::renderer() return one of these smart pointers, but that might require a lot of code changes.

I still like Ryosuke’s original patch.

Like James, I am worried about getting maximum bang for the buck. I don’t want to make a bigger change than is needed to uncover the bugs.

[Ryosuke Niwa]

(In reply to comment #23)
> I would really like to see some of the bugs this guard can catch and try to figure out what checks will help us find bugs and what checks are less likely to.  I think you've mentioned finding some tests that fail with this on, Ryosuke, could you link those bugs here or post some stack traces?

Hi James.  Sorry, I had been busy working on the other stuff.  

plugins/createScriptableObject-before-start.html's stack trace is the following:

#0	0x10189c6bc in WebCore::RenderObjectLayoutGuard::RenderObjectLayoutGuard at RenderObject.h:1034
#1	0x1018915d5 in WebCore::RenderObject::enclosingLayer at RenderObject.cpp:534
#2	0x10124a518 in WebCore::FrameView::windowClipRect at FrameView.cpp:1720
#3	0x10191726f in WebCore::RenderWidget::windowClipRect at RenderWidget.cpp:369
#4	0x1008d15b0 in -[WebBaseNetscapePluginView _windowClipRect] at WebBaseNetscapePluginView.mm:345
#5	0x1008d1471 in -[WebBaseNetscapePluginView visibleRect] at WebBaseNetscapePluginView.mm:352
#6	0x101b118ae in WebCore::Widget::setFrameRect at WidgetMac.mm:177
#7	0x101917fae in WebCore::RenderWidget::setWidgetGeometry at RenderWidget.cpp:177
#8	0x101918a8a in WebCore::RenderWidget::setWidget at RenderWidget.cpp:205
#9	0x10189e9c7 in WebCore::RenderPart::setWidget at RenderPart.cpp:50
#10	0x1019da729 in WebCore::SubframeLoader::loadPlugin at SubframeLoader.cpp:367
#11	0x1019db590 in WebCore::SubframeLoader::requestObject at SubframeLoader.cpp:138
#12	0x101330459 in WebCore::HTMLObjectElement::updateWidget at HTMLObjectElement.cpp:292
#13	0x10124aac1 in WebCore::FrameView::updateWidget at FrameView.cpp:1575
#14	0x10124ac87 in WebCore::FrameView::updateWidgets at FrameView.cpp:1607
#15	0x10124af76 in WebCore::FrameView::performPostLayoutTasks at FrameView.cpp:1637
#16	0x10124d940 in WebCore::FrameView::layout at FrameView.cpp:819
#17	0x10109c96b in WebCore::Document::updateLayout at Document.cpp:1566
#18	0x10109e7ce in WebCore::Document::updateLayoutIgnorePendingStylesheets at Document.cpp:1597
#19	0x101330b47 in WebCore::HTMLObjectElement::renderWidgetForJSBindings at HTMLObjectElement.cpp:64
#20	0x101336faa in WebCore::HTMLPlugInElement::pluginWidget at HTMLPlugInElement.cpp:103
#21	0x1015b8e65 in WebCore::pluginScriptObjectFromPluginViewBase at JSPluginElementFunctions.cpp:60
#22	0x1015b8f22 in WebCore::pluginScriptObject at JSPluginElementFunctions.cpp:90

It's hitting ASSERT(m_object) i.e. ASSERT(this) in the guard's constructor.

The stack trace for plugins/createScriptableObject-before-start.html is:

#0	0x10124ccff in WebCore::FrameView::layout at FrameView.cpp:620
#1	0x10124dc5a in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive at FrameView.cpp:2042
#2	0x10092f785 in -[WebHTMLView(WebInternal) _web_updateLayoutAndStyleIfNeededRecursive] at WebHTMLView.mm:5517
#3	0x10092af51 in -[WebHTMLView(WebPrivate) viewWillDraw] at WebHTMLView.mm:1304
#4	0x7fff87c09251 in -[NSView viewWillDraw]
#5	0x7fff87c09251 in -[NSView viewWillDraw]
#6	0x7fff87c09251 in -[NSView viewWillDraw]
#7	0x7fff87c09251 in -[NSView viewWillDraw]
#8	0x1009ace71 in -[WebView(WebPrivate) viewWillDraw] at WebView.mm:869
#9	0x7fff87c09251 in -[NSView viewWillDraw]
#10	0x7fff87c09251 in -[NSView viewWillDraw]
#11	0x7fff87c08802 in -[NSView _sendViewWillDrawInRect:clipRootView:suppressRecursion:]
#12	0x7fff87b83fb9 in -[NSView displayIfNeeded]
#13	0x7fff87b397e4 in -[NSNextStepFrame displayIfNeeded]
#14	0x1019735e9 in WebCore::ScrollView::platformRepaintContentRectangle at ScrollViewMac.mm:168
#15	0x10196eda3 in WebCore::ScrollView::repaintContentRectangle at ScrollView.cpp:756
#16	0x10124bde7 in WebCore::FrameView::repaintContentRectangle at FrameView.cpp:1202
#17	0x10190f596 in WebCore::RenderView::repaintViewRectangle at RenderView.cpp:249
#18	0x101895129 in WebCore::RenderObject::repaintUsingContainer at RenderObject.cpp:1336
#19	0x101896202 in WebCore::RenderObject::repaint at RenderObject.cpp:1363
#20	0x100f67049 in WebCore::ContainerNode::setActive at ContainerNode.cpp:951
#21	0x1013163d7 in WebCore::HTMLInputElement::defaultEventHandler at HTMLInputElement.cpp:1589
#22	0x101765469 in WebCore::Node::dispatchGenericEvent at Node.cpp:2678
#23	0x101765695 in WebCore::Node::dispatchEvent at Node.cpp:2587
#24	0x1011e0afe in WebCore::EventTarget::dispatchEvent at EventTarget.cpp:282
#25	0x1011cde5f in WebCore::EventHandler::keyEvent at EventHandler.cpp:2310
#26	0x1011db675 in WebCore::EventHandler::keyEvent at EventHandlerMac.mm:148
#27	0x100931727 in -[WebHTMLView keyDown:] at WebHTMLView.mm:4128
#28	0x100017cbe in -[EventSendingController keyDown:withModifiers:withLocation:] at EventSendingController.mm:700

This one is interesting because it never calls Document::recalcStyle.  It's either that adding assertion into layout causes false positives or that we need to add the assertion to both Document::recalcStyle and FrameView::layout.

[Ryosuke Niwa]

There are also ones that instantiate VisiblePosition but I don't post stack trace here 'cause they're self-evidently wrong.

[Ryosuke Niwa]

Created attachment 70982 [details]
new patch

[Darin Adler]

(In reply to comment #37)
> plugins/createScriptableObject-before-start.html's stack trace is the following:
> 
> #0    0x10189c6bc in WebCore::RenderObjectLayoutGuard::RenderObjectLayoutGuard at RenderObject.h:1034
> #1    0x1018915d5 in WebCore::RenderObject::enclosingLayer at RenderObject.cpp:534
> #2    0x10124a518 in WebCore::FrameView::windowClipRect at FrameView.cpp:1720
> #3    0x10191726f in WebCore::RenderWidget::windowClipRect at RenderWidget.cpp:369
> #4    0x1008d15b0 in -[WebBaseNetscapePluginView _windowClipRect] at WebBaseNetscapePluginView.mm:345
> #5    0x1008d1471 in -[WebBaseNetscapePluginView visibleRect] at WebBaseNetscapePluginView.mm:352
> #6    0x101b118ae in WebCore::Widget::setFrameRect at WidgetMac.mm:177
> #7    0x101917fae in WebCore::RenderWidget::setWidgetGeometry at RenderWidget.cpp:177
> #8    0x101918a8a in WebCore::RenderWidget::setWidget at RenderWidget.cpp:205
> #9    0x10189e9c7 in WebCore::RenderPart::setWidget at RenderPart.cpp:50
> #10    0x1019da729 in WebCore::SubframeLoader::loadPlugin at SubframeLoader.cpp:367
> #11    0x1019db590 in WebCore::SubframeLoader::requestObject at SubframeLoader.cpp:138
> #12    0x101330459 in WebCore::HTMLObjectElement::updateWidget at HTMLObjectElement.cpp:292
> #13    0x10124aac1 in WebCore::FrameView::updateWidget at FrameView.cpp:1575
> #14    0x10124ac87 in WebCore::FrameView::updateWidgets at FrameView.cpp:1607
> #15    0x10124af76 in WebCore::FrameView::performPostLayoutTasks at FrameView.cpp:1637
> #16    0x10124d940 in WebCore::FrameView::layout at FrameView.cpp:819
> #17    0x10109c96b in WebCore::Document::updateLayout at Document.cpp:1566
> #18    0x10109e7ce in WebCore::Document::updateLayoutIgnorePendingStylesheets at Document.cpp:1597
> #19    0x101330b47 in WebCore::HTMLObjectElement::renderWidgetForJSBindings at HTMLObjectElement.cpp:64
> #20    0x101336faa in WebCore::HTMLPlugInElement::pluginWidget at HTMLPlugInElement.cpp:103
> #21    0x1015b8e65 in WebCore::pluginScriptObjectFromPluginViewBase at JSPluginElementFunctions.cpp:60
> #22    0x1015b8f22 in WebCore::pluginScriptObject at JSPluginElementFunctions.cpp:90
> 
> It's hitting ASSERT(m_object) i.e. ASSERT(this) in the guard's constructor.

This is catching something completely unrelated to destruction of render tree objects. This is a case of a RenderObject member function being called with a null pointer. Needs its own separate bug report. A a bug in FrameView::windowClipRect, which needs an additional null check.

> The stack trace for plugins/createScriptableObject-before-start.html is:
> 
> #0    0x10124ccff in WebCore::FrameView::layout at FrameView.cpp:620
> #1    0x10124dc5a in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive at FrameView.cpp:2042
> #2    0x10092f785 in -[WebHTMLView(WebInternal) _web_updateLayoutAndStyleIfNeededRecursive] at WebHTMLView.mm:5517
> #3    0x10092af51 in -[WebHTMLView(WebPrivate) viewWillDraw] at WebHTMLView.mm:1304
> #4    0x7fff87c09251 in -[NSView viewWillDraw]
> #5    0x7fff87c09251 in -[NSView viewWillDraw]
> #6    0x7fff87c09251 in -[NSView viewWillDraw]
> #7    0x7fff87c09251 in -[NSView viewWillDraw]
> #8    0x1009ace71 in -[WebView(WebPrivate) viewWillDraw] at WebView.mm:869
> #9    0x7fff87c09251 in -[NSView viewWillDraw]
> #10    0x7fff87c09251 in -[NSView viewWillDraw]
> #11    0x7fff87c08802 in -[NSView _sendViewWillDrawInRect:clipRootView:suppressRecursion:]
> #12    0x7fff87b83fb9 in -[NSView displayIfNeeded]
> #13    0x7fff87b397e4 in -[NSNextStepFrame displayIfNeeded]
> #14    0x1019735e9 in WebCore::ScrollView::platformRepaintContentRectangle at ScrollViewMac.mm:168
> #15    0x10196eda3 in WebCore::ScrollView::repaintContentRectangle at ScrollView.cpp:756
> #16    0x10124bde7 in WebCore::FrameView::repaintContentRectangle at FrameView.cpp:1202
> #17    0x10190f596 in WebCore::RenderView::repaintViewRectangle at RenderView.cpp:249
> #18    0x101895129 in WebCore::RenderObject::repaintUsingContainer at RenderObject.cpp:1336
> #19    0x101896202 in WebCore::RenderObject::repaint at RenderObject.cpp:1363
> #20    0x100f67049 in WebCore::ContainerNode::setActive at ContainerNode.cpp:951
> #21    0x1013163d7 in WebCore::HTMLInputElement::defaultEventHandler at HTMLInputElement.cpp:1589
> #22    0x101765469 in WebCore::Node::dispatchGenericEvent at Node.cpp:2678
> #23    0x101765695 in WebCore::Node::dispatchEvent at Node.cpp:2587
> #24    0x1011e0afe in WebCore::EventTarget::dispatchEvent at EventTarget.cpp:282
> #25    0x1011cde5f in WebCore::EventHandler::keyEvent at EventHandler.cpp:2310
> #26    0x1011db675 in WebCore::EventHandler::keyEvent at EventHandlerMac.mm:148
> #27    0x100931727 in -[WebHTMLView keyDown:] at WebHTMLView.mm:4128
> #28    0x100017cbe in -[EventSendingController keyDown:withModifiers:withLocation:] at EventSendingController.mm:700
> 
> This one is interesting because it never calls Document::recalcStyle.  It's either that adding assertion into layout causes false positives or that we need to add the assertion to both Document::recalcStyle and FrameView::layout.

I don’t think the lack of recalcStyle is deeply interesting. This is not a false positive.

This issue is fundamental to the “repaint immediately” feature in various render tree repaint functions. There’s no way to repaint immediately without possibly reentering the style and layout calculation machinery.

The best way I can think of to fix this is to change the API for immediate repaint so that callers don’t tell the renderer to do it. Instead the mechanism to trigger an immediate repaint can be a separate call made on FrameView afterward. Thus the code in ContainerNode::setActive, for example, would call a function directly on the FrameView to trigger the immediate repainting rather than using a boolean to get the renderer to do it indirectly.

In fact, I think the existing code forces extra painting since it does a repaint directly on the renderer in addition to the style change, but the style change should already trigger sufficient repainting indirectly.

[Darin Adler]

Each of these issues needs its own bug report. While I understand James’s desire to hear about them to help develop the assertion, I also think the discussion gets really confusing when we are talking about multiple problems all in a single report.

[Ryosuke Niwa]

(In reply to comment #40)
> This is catching something completely unrelated to destruction of render tree objects. This is a case of a RenderObject member function being called with a null pointer. Needs its own separate bug report. A a bug in FrameView::windowClipRect, which needs an additional null check.

Yes.

> I don’t think the lack of recalcStyle is deeply interesting. This is not a false positive.

Good to know this is not a false positive.

> This issue is fundamental to the “repaint immediately” feature in various render tree repaint functions. There’s no way to repaint immediately without possibly reentering the style and layout calculation machinery.
> 
> The best way I can think of to fix this is to change the API for immediate repaint so that callers don’t tell the renderer to do it. Instead the mechanism to trigger an immediate repaint can be a separate call made on FrameView afterward. Thus the code in ContainerNode::setActive, for example, would call a function directly on the FrameView to trigger the immediate repainting rather than using a boolean to get the renderer to do it indirectly.
> 
> In fact, I think the existing code forces extra painting since it does a repaint directly on the renderer in addition to the style change, but the style change should already trigger sufficient repainting indirectly.

Do you think we should add the assertion to both FrameView::layout and Document::recalcStyle since this cannot be caught without the assertion in FrameView::layout?  Of course, we could do that in a follow-up patch.

[Darin Adler]

(In reply to comment #42)
> Do you think we should add the assertion to both FrameView::layout and Document::recalcStyle since this cannot be caught without the assertion in FrameView::layout? Of course, we could do that in a follow-up patch.

We definitely will need the assertion in more places than just Document::recalcStyle, but I don’t think those two functions are the right places.

I would want an assertion in updateStyleIfNeeded and in updateStyleForAllDocuments, for example. Sometimes calls to one or both of those would call recalcStyle or layout, but other times they would not, and any time those were called we would want to detect that style computation could have occurred.

Finding just the right places to put the assertion will take a little research and thought.

[Eric Seidel (no email)]

Comment on attachment 70820 [details]
removed guards that caused crashes on the existing layout tests

Cleared Darin Adler's review+ from obsolete attachment 70820 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Ryosuke Niwa]

(In reply to comment #43)
> We definitely will need the assertion in more places than just Document::recalcStyle, but I don’t think those two functions are the right places.
> 
> I would want an assertion in updateStyleIfNeeded and in updateStyleForAllDocuments, for example. Sometimes calls to one or both of those would call recalcStyle or layout, but other times they would not, and any time those were called we would want to detect that style computation could have occurred.

Yes, we definitely need check there because I instantly found these:

#0	0x1010a75ca in WebCore::Document::updateStyleIfNeeded at Document.cpp:1550
#1	0x1010a73b9 in WebCore::Document::updateLayout at Document.cpp:1584
#2	0x10187ae2d in WebCore::RenderLayer::hitTest at RenderLayer.cpp:2655
#3	0x1011cef34 in WebCore::EventHandler::hitTestResultAtPoint at EventHandler.cpp:874
#4	0x10119f096 in WebCore::DragController::mayStartDragAtEventLocation at DragController.cpp:570
#5	0x1011ca6be in WebCore::EventHandler::shouldDragAutoNode at EventHandler.cpp:2454
#6	0x1018b22a5 in WebCore::RenderObject::draggableNode at RenderObject.cpp:1673
#7	0x1011cf863 in WebCore::EventHandler::handleDrag at EventHandler.cpp:2507
#8	0x1011d161c in WebCore::EventHandler::handleMouseDraggedEvent at EventHandler.cpp:508
#9	0x1011d2073 in WebCore::EventHandler::handleMouseMoveEvent at EventHandler.cpp:1517
#10	0x1011d9182 in WebCore::EventHandler::mouseDragged at EventHandlerMac.mm:519

#0	0x1010a75ca in WebCore::Document::updateStyleIfNeeded at Document.cpp:1550
#1	0x10124cbcb in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive at FrameView.cpp:2111
#2	0x100937eff in -[WebHTMLView(WebInternal) _web_updateLayoutAndStyleIfNeededRecursive] at WebHTMLView.mm:5521
#3	0x1009335bf in -[WebHTMLView(WebPrivate) viewWillDraw] at WebHTMLView.mm:1305
#4	0x7fff87c09251 in -[NSView viewWillDraw]
#5	0x7fff87c09251 in -[NSView viewWillDraw]
#6	0x7fff87c09251 in -[NSView viewWillDraw]
#7	0x7fff87c09251 in -[NSView viewWillDraw]
#8	0x1009b48ee in -[WebView(WebPrivate) viewWillDraw] at WebView.mm:886
#9	0x7fff87c09251 in -[NSView viewWillDraw]
#10	0x7fff87c09251 in -[NSView viewWillDraw]
#11	0x7fff87c08802 in -[NSView _sendViewWillDrawInRect:clipRootView:suppressRecursion:]
#12	0x7fff87b83fb9 in -[NSView displayIfNeeded]
#13	0x7fff87b397e4 in -[NSNextStepFrame displayIfNeeded]
#14	0x1019966cd in WebCore::ScrollView::platformRepaintContentRectangle at ScrollViewMac.mm:168
#15	0x101991e9f in WebCore::ScrollView::repaintContentRectangle at ScrollView.cpp:770
#16	0x10124a7d1 in WebCore::FrameView::repaintContentRectangle at FrameView.cpp:1276
#17	0x1019328fc in WebCore::RenderView::repaintViewRectangle at RenderView.cpp:257
#18	0x1018b29f1 in WebCore::RenderObject::repaintUsingContainer at RenderObject.cpp:1341
#19	0x1018b3aca in WebCore::RenderObject::repaint at RenderObject.cpp:1368
#20	0x100f6dc69 in WebCore::ContainerNode::setActive at ContainerNode.cpp:949

On the other hand, it also finds something in a gray zone.  The following one is actually harmless now
because creating VisiblePosition is the last thing we do in each of these functions on the stack.
However, this is very fragile design too.  IMHO, we should NEVER create VisiblePosition in RenderObject.
In fact, WebCore/rendering shouldn't even know what VisiblePosition is.

#0	0x1010a75ca in WebCore::Document::updateStyleIfNeeded at Document.cpp:1550
#1	0x1010a73b9 in WebCore::Document::updateLayout at Document.cpp:1584
#2	0x1010a92b8 in WebCore::Document::updateLayoutIgnorePendingStylesheets at Document.cpp:1620
#3	0x101afdd0a in WebCore::VisiblePosition::canonicalPosition at VisiblePosition.cpp:459
#4	0x101aff288 in WebCore::VisiblePosition::init at VisiblePosition.cpp:61
#5	0x101aff397 in WebCore::VisiblePosition::VisiblePosition at VisiblePosition.cpp:54
#6	0x1018afadc in WebCore::RenderObject::createVisiblePosition at RenderObject.cpp:2716
#7	0x1017ff1bf in WebCore::RenderBlock::positionForPointWithInlineChildren at RenderBlock.cpp:4286
#8	0x1017ff65f in WebCore::RenderBlock::positionForPoint at RenderBlock.cpp:4369

[Ryosuke Niwa]

Let me define few properties to talk about the rules more precisely.

P(X): X is an inline single-line getting or setter, which only accesses a POD value
Q(X): X is a function that does not cause layout, and there is infinitesimally small or zero chance for us to modify X to cause a layout (e.g. most of stuff in wtf).
R(X): X is an inline single-line function that only calls functions with properties P, Q, or R.
S(X): X is not a member function of RenderObject or its derived classes and it does not need to guard any render object.

And I claim that we don't need to add a guard to {X in all functions of WebKit | P(X) or Q(X) or R(X) or S(X) } although we probably need to refine the definition of Q.

[Ryosuke Niwa]

Created attachment 73031 [details]
added assertion to updateStyleIfNeeded and updateStyleForAllDocuments

[Darin Adler]

(In reply to comment #46)
> Let me define few properties to talk about the rules more precisely.
> 
> P(X): X is an inline single-line getting or setter, which only accesses a POD value
> Q(X): X is a function that does not cause layout, and there is infinitesimally small or zero chance for us to modify X to cause a layout (e.g. most of stuff in wtf).
> R(X): X is an inline single-line function that only calls functions with properties P, Q, or R.
> S(X): X is not a member function of RenderObject or its derived classes and it does not need to guard any render object.
> 
> And I claim that we don't need to add a guard to {X in all functions of WebKit | P(X) or Q(X) or R(X) or S(X) } although we probably need to refine the definition of Q.

This analysis seems OK, but the issue is not restricted to member functions so I’m not sure that S(X) is a helpful distinction.

The potential problem comes any time code holds a pointer to a render tree object. You can do that in any function. The only thing special about member functions of RenderObject and classes derived is that they have such a in "this". If we have code holding such a pointer that plans to reuse it, then we want an assertion to fire if the functions that can destroy renderers are called.

[Ryosuke Niwa]

(In reply to comment #48)
> This analysis seems OK, but the issue is not restricted to member functions so I’m not sure that S(X) is a helpful distinction.
> 
> The potential problem comes any time code holds a pointer to a render tree object. You can do that in any function. The only thing special about member functions of RenderObject and classes derived is that they have such a in "this". If we have code holding such a pointer that plans to reuse it, then we want an assertion to fire if the functions that can destroy renderers are called.

Right.  But I think this problem can be addressed by your smart pointer idea.  I like that solution better for components that use render objects since it's less invasive and hides the implementation details of the rendering engine.  IMHO, it'll be really hard to enforce the use of ForbitRenderObjectDestruction outside of WebCore/rendering.

[Darin Adler]

Comment on attachment 73031 [details]
added assertion to updateStyleIfNeeded and updateStyleForAllDocuments

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.

[Darin Adler]

Comment on attachment 73031 [details]
added assertion to updateStyleIfNeeded and updateStyleForAllDocuments

View in context: https://bugs.webkit.org/attachment.cgi?id=73031&action=review

> WebCore/rendering/RenderObject.cpp:2770
>  void RenderObject::setNeedsBoundariesUpdate()
>  {
> +    ForbidRenderObjectDestruction forbidDestructionOf(this);
>      if (RenderObject* renderer = parent())
>          renderer->setNeedsBoundariesUpdate();
>  }

This is a good example of a function that doesn’t need to forbid anything. It calls only one function, parent, so it’s not really holding a pointer. I think having too many assertions may be a problem.

[Darin Adler]

(In reply to comment #49)
> But I think this problem can be addressed by your smart pointer idea.  I like that solution better for components that use render objects since it's less invasive and hides the implementation details of the rendering engine.

If we are going to create the smart pointer, then I suggest we use that here the way we would use a RefPtr as a “protector” instead of having a separate “forbidder” class.

[Ryosuke Niwa]

I also caught:
#0	0x10109fe3f in WebCore::Document::updateStyleForAllDocuments at Document.cpp:1564
#1	0x10148acb1 in WebCore::JSCallbackData::invokeCallback at JSCallbackData.cpp:73
#2	0x1015e87b6 in WebCore::JSSQLTransactionSyncCallback::handleEvent at JSSQLTransactionSyncCallback.cpp:72
#3	0x1019e5520 in WebCore::SQLTransactionSync::execute at SQLTransactionSync.cpp:153
#4	0x101062abb in WebCore::DatabaseSync::runTransaction at DatabaseSync.cpp:152
#5	0x101062b9d in WebCore::DatabaseSync::transaction at DatabaseSync.cpp:134
#6	0x1014bdcf7 in WebCore::jsDatabaseSyncPrototypeFunctionTransaction at JSDatabaseSync.cpp:182
#7	0x3f93858041b8 in ??
#8	0x1001b65bd in JSC::JITCode::execute at JITCode.h:77
#9	0x1001b2648 in JSC::Interpreter::execute at Interpreter.cpp:759
#10	0x100181fa6 in JSC::evaluate at Completion.cpp:62
#11	0x101b43026 in WebCore::WorkerScriptController::evaluate at WorkerScriptController.cpp:128
#12	0x101b4320c in WebCore::WorkerScriptController::evaluate at WorkerScriptController.cpp:107
#13	0x101b44a48 in WebCore::WorkerThread::workerThread at WorkerThread.cpp:134
#14	0x101b44b41 in WebCore::WorkerThread::workerThreadStart at WorkerThread.cpp:117
#15	0x1002c974d in WTF::threadEntryPoint at Threading.cpp:65

in running fast/workers/storage/test-authorizer-sync.html.  Now I'm more than convinced that we need this guard.

[Darin Adler]

(In reply to comment #53)
> I also caught:
> #0    0x10109fe3f in WebCore::Document::updateStyleForAllDocuments at Document.cpp:1564
> #1    0x10148acb1 in WebCore::JSCallbackData::invokeCallback at JSCallbackData.cpp:73
> #2    0x1015e87b6 in WebCore::JSSQLTransactionSyncCallback::handleEvent at JSSQLTransactionSyncCallback.cpp:72
> #3    0x1019e5520 in WebCore::SQLTransactionSync::execute at SQLTransactionSync.cpp:153
> #4    0x101062abb in WebCore::DatabaseSync::runTransaction at DatabaseSync.cpp:152
> #5    0x101062b9d in WebCore::DatabaseSync::transaction at DatabaseSync.cpp:134
> #6    0x1014bdcf7 in WebCore::jsDatabaseSyncPrototypeFunctionTransaction at JSDatabaseSync.cpp:182
> #7    0x3f93858041b8 in ??
> #8    0x1001b65bd in JSC::JITCode::execute at JITCode.h:77
> #9    0x1001b2648 in JSC::Interpreter::execute at Interpreter.cpp:759
> #10    0x100181fa6 in JSC::evaluate at Completion.cpp:62
> #11    0x101b43026 in WebCore::WorkerScriptController::evaluate at WorkerScriptController.cpp:128
> #12    0x101b4320c in WebCore::WorkerScriptController::evaluate at WorkerScriptController.cpp:107
> #13    0x101b44a48 in WebCore::WorkerThread::workerThread at WorkerThread.cpp:134
> #14    0x101b44b41 in WebCore::WorkerThread::workerThreadStart at WorkerThread.cpp:117
> #15    0x1002c974d in WTF::threadEntryPoint at Threading.cpp:65
> 
> in running fast/workers/storage/test-authorizer-sync.html.  Now I'm more than convinced that we need this guard.

This one says that Document::updateStyleForAllDocuments needs an isMainThread guard.

[Ryosuke Niwa]

(In reply to comment #50)
> > 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.

Sorry, I forgot about that again.  But I'd pursue your smart pointer / protector idea since it end up being almost identical implementation with a little more functionality.

> 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.

Done, made it global.  But my solution isn't ideal.  It pollutes WebCore namespace and referencing through RenderObjectPointer<RenderObject> looks weird.  Do you think I should just make rendererDestructionIsForbidden a global function?

> > WebCore/rendering/RenderObject.h:117
> > +private:
> > +#ifndef NDEBUG
> > +    const RenderObject* m_object;
> > +#endif
> 
> I would put the private: inside the #ifndef myself.

It turned out that we need m_object even in release build for operator*().  I'll check assembly code & make sure gcc still optimize all of this away on release build.

(In reply to comment #51)
> (From update of attachment 73031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73031&action=review
> 
> > WebCore/rendering/RenderObject.cpp:2770
> >  void RenderObject::setNeedsBoundariesUpdate()
> >  {
> > +    ForbidRenderObjectDestruction forbidDestructionOf(this);
> >      if (RenderObject* renderer = parent())
> >          renderer->setNeedsBoundariesUpdate();
> >  }
> 
> This is a good example of a function that doesn’t need to forbid anything. It calls only one function, parent, so it’s not really holding a pointer. I think having too many assertions may be a problem.

Thanks for noticing that.  The guard removed.

(In reply to comment #52)
> If we are going to create the smart pointer, then I suggest we use that here the way we would use a RefPtr as a “protector” instead of having a separate “forbidder” class.

Done.

[Ryosuke Niwa]

Created attachment 73136 [details]
implements smart pointer

[Ryosuke Niwa]

(In reply to comment #55)
> It turned out that we need m_object even in release build for operator*().  I'll check assembly code & make sure gcc still optimize all of this away on release build.

Checked.

[Ryosuke Niwa]

Darin, could you take a look at the new patch?  I implemented the smart pointer and deployed it as a protector in several places.  I talked with Simon on IRC, and he suggested I ask you to take a look at it.

[Ryosuke Niwa]

Comment on attachment 73136 [details]
implements smart pointer

Oops, I just realized I didn't implement copy constructor & assignment operator in this patch.  Will do and resubmit.

[Ryosuke Niwa]

Created attachment 73842 [details]
added copy constructor and removed erroneous const

[Darin Adler]

Comment on attachment 73842 [details]
added copy constructor and removed erroneous const

View in context: https://bugs.webkit.org/attachment.cgi?id=73842&action=review

While this is not great, it’s good as is, so r=me.

I am not comfortable with the rules about where to use this smart pointer class. It’s going to be tricky to teach people how to use it.

> WebCore/dom/Document.cpp:1530
> +    ASSERT(!RenderObjectPointer<RenderObject>::rendererDestructionIsForbidden());

It’s strange to be calling this function on one particular class expanded from the template rather than on the entire template. I think because of that, it should be a non-member function.

> WebCore/rendering/RenderObject.cpp:235
> +    RenderObjectPointer<const RenderObject> forbidDestructionOf(this);

I do not think the name forbidDestructionOf is all that cute. Could we come up with a less cute way to do this.

I still don’t think it makes sense to add this in all the places we add it in this patch. For example, there is definitely no need for it in this one-line function!

> WebCore/rendering/RenderObject.h:864
> +class RenderObjectPointer : public FastAllocBase {

We traditionally use the abbreviation Ptr in all our smart pointer classes. I think RenderPtr would be a better name for this class template than RenderObjectPointer.

This class template should have its own header file.

It would be better to say:

    template<typename T>

rather than:
    template<class T>

> WebCore/rendering/RenderObject.h:867
> +    : m_renderer(renderer)

We indent these.

> WebCore/rendering/RenderObject.h:875
> +    : m_renderer(o.m_renderer)

We indent these.

> WebCore/rendering/RenderObject.h:889
> +    T* operator*() const { return m_renderer; }

We also need to override operator-> so we can use these as normal pointers.

We’ll also need either a get() function or implicit conversion to T*.

[Ryosuke Niwa]

Comment on attachment 73842 [details]
added copy constructor and removed erroneous const

View in context: https://bugs.webkit.org/attachment.cgi?id=73842&action=review

Because these are major changes, I'll re-submit patch for another review.

>> WebCore/dom/Document.cpp:1530
>> +    ASSERT(!RenderObjectPointer<RenderObject>::rendererDestructionIsForbidden());
> 
> It’s strange to be calling this function on one particular class expanded from the template rather than on the entire template. I think because of that, it should be a non-member function.

Will do.

>> WebCore/rendering/RenderObject.cpp:235
>> +    RenderObjectPointer<const RenderObject> forbidDestructionOf(this);
> 
> I do not think the name forbidDestructionOf is all that cute. Could we come up with a less cute way to do this.
> 
> I still don’t think it makes sense to add this in all the places we add it in this patch. For example, there is definitely no need for it in this one-line function!

Protector?  Right, this and the one below are safe.  Will remove.

>> WebCore/rendering/RenderObject.h:864
>> +class RenderObjectPointer : public FastAllocBase {
> 
> We traditionally use the abbreviation Ptr in all our smart pointer classes. I think RenderPtr would be a better name for this class template than RenderObjectPointer.
> 
> This class template should have its own header file.
> 
> It would be better to say:
> 
>     template<typename T>
> 
> rather than:
>     template<class T>

Will do.

>> WebCore/rendering/RenderObject.h:867
>> +    : m_renderer(renderer)
> 
> We indent these.

Will do.

>> WebCore/rendering/RenderObject.h:875
>> +    : m_renderer(o.m_renderer)
> 
> We indent these.

Ditto.

>> WebCore/rendering/RenderObject.h:889
>> +    T* operator*() const { return m_renderer; }
> 
> We also need to override operator-> so we can use these as normal pointers.
> 
> We’ll also need either a get() function or implicit conversion to T*.

Oops, I meant to say "operator T*()" instead of "T* operator*()" there.  Obviously, operator*() should return the reference...
I'm adding implicit conversion to T*.

[Ryosuke Niwa]

Created attachment 73950 [details]
added RefPtr.h

[Simon Fraser (smfr)]

Comment on attachment 73950 [details]
added RefPtr.h

I'm not sure the 'protector' term is correct here. We normally use 'protector' for ref-counted things that we need to keep alive for some operation. Here, protector is not "protecting" the RenderObject, it's just making things assert if something bad happens. This could lead to misunderstanding and incorrect code. It's more like a canary, but I can't think of a good name.

[Ryosuke Niwa]

(In reply to comment #64)
> (From update of attachment 73950 [details])
> I'm not sure the 'protector' term is correct here. We normally use 'protector' for ref-counted things that we need to keep alive for some operation. Here, protector is not "protecting" the RenderObject, it's just making things assert if something bad happens. This could lead to misunderstanding and incorrect code. It's more like a canary, but I can't think of a good name.

That's a very good point.

I also have a concern for RenderPtr.  I'm not 100% convinced that using it for argument list and return value will have the same effect as using Render*.  I think it should given that RenderPtr has exactly one member variable which is Render* but I'm not sure if the entire object is put into a register and treated as a POD.  Does anyone know?

[Darin Adler]

(In reply to comment #65)
> I also have a concern for RenderPtr. I'm not 100% convinced that using it for argument list and return value will have the same effect as using Render*. I think it should given that RenderPtr has exactly one member variable which is Render* but I'm not sure if the entire object is put into a register and treated as a POD.  Does anyone know?

We did this testing when working on PassRefPtr, and it was just as efficient as passing the scalar.

[Ryosuke Niwa]

(In reply to comment #64)
> (From update of attachment 73950 [details])
> I'm not sure the 'protector' term is correct here. We normally use 'protector' for ref-counted things that we need to keep alive for some operation. Here, protector is not "protecting" the RenderObject, it's just making things assert if something bad happens. This could lead to misunderstanding and incorrect code. It's more like a canary, but I can't think of a good name.

How about "forbidRendererDestruction" or "assertNoRendererDestruction"?

(In reply to comment #66)
> (In reply to comment #65)
> > I also have a concern for RenderPtr. I'm not 100% convinced that using it for argument list and return value will have the same effect as using Render*. I think it should given that RenderPtr has exactly one member variable which is Render* but I'm not sure if the entire object is put into a register and treated as a POD.  Does anyone know?
> 
> We did this testing when working on PassRefPtr, and it was just as efficient as passing the scalar.

Thanks for the information.  Good to know.

[Darin Adler]

(In reply to comment #67)
> (In reply to comment #64)
> > I'm not sure the 'protector' term is correct here. We normally use 'protector' for ref-counted things that we need to keep alive for some operation. Here, protector is not "protecting" the RenderObject, it's just making things assert if something bad happens. This could lead to misunderstanding and incorrect code. It's more like a canary, but I can't think of a good name.
> 
> How about "forbidRendererDestruction" or "assertNoRendererDestruction"?

These seem OK, but objects should have names that are noun phrases, not verb phrases.

[Ryosuke Niwa]

(In reply to comment #68)
> (In reply to comment #67)
> > How about "forbidRendererDestruction" or "assertNoRendererDestruction"?
> 
> These seem OK, but objects should have names that are noun phrases, not verb phrases.

Right, that'll be ideal.  But I can't come up with a good noun.  Maybe renderDestructionIsForbidden?  Or renderDestructionDetector?

[Darin Adler]

(In reply to comment #69)
> (In reply to comment #68)
> > (In reply to comment #67)
> > > How about "forbidRendererDestruction" or "assertNoRendererDestruction"?
> > 
> > These seem OK, but objects should have names that are noun phrases, not verb phrases.
> 
> Right, that'll be ideal.  But I can't come up with a good noun.  Maybe renderDestructionIsForbidden?  Or renderDestructionDetector?

I think destructionDetector is the best one I’ve heard so far.

[Ryosuke Niwa]

Created attachment 74616 [details]
renamed protector to destructionDetector

[Ryosuke Niwa]

Could someone review this patch?

[Adam Barth]

Comment on attachment 74616 [details]
renamed protector to destructionDetector

I think we need some more high-level agreement that this is a good approach to solving this problem.  There's been some discussion about how to solve this problem at the contributor meeting.  I think you and some rendertree folks should figure out the best approach here and then execute it.

[Adam Barth]

I'd also recommend using a new bug because this one has gone epic.