Bug 119070 – Introduce RecursionGuard to prevent recursive calls to a function

• Status: NEW
• Component: Web Template Framework

[Kwang Yul Seo]

A common pattern to prevent recursion calls to a function is

void Foo::bar()
{
    ASSERT(!m_inBar);
    m_inBar = true;

    ...

    m_inBar = false;
}

We can abstract this pattern into RecursionGuard, which is more convenient because it automatically asserts if the flag is false before setting it to true, and resets the flag to false in the destructor (RAAI).

void Foo::bar()
{
    RecursionGuard guard(m_inBar);

    ...
}

[Kwang Yul Seo]

Created attachment 207427 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #1)
> Created an attachment (id=207427) [details]
> Patch

I applied RecursionGuard to where there is no behavior change. 

There are many more sites where we can use RecursionGuard, but RecursionGuard introduces an assertion which hasn't been there before, so we need to check if it is okay. I will work on them later one by one.

[Alexey Proskuryakov]

Thank you for following up with this!

I suggest e-mailing webkit-dev. First, it's good to spread knowledge, and second, a small discussion about naming might be useful.

[Benjamin Poulain]

What I would expect from something named RecursionGuard is an early return if we have recursion (RecursionGuard = guard that prevents recursion).

To prevent recursion, we would not even need an attribute, we could just walk up the stack frames until we find the current address.

It is also not preventing recursion, but re-entry. If you have two methods with the "RecursionGuard", entering the first then the second would cause an assertion.

Naming this thing is gonna be hard… ScopedAssignment? e.g.:

    ASSERT(!m_inBar);
    ScopedAssignment guard(m_inBar, true);

[Adam Barth]

Comment on attachment 207427 [details]
Patch

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

> Source/WTF/wtf/RecursionGuard.h:44
> +        m_guard = true;

Have you considered building this class using WTF's TemporaryChange?

> Source/WTF/wtf/RecursionGuard.h:49
> +        m_guard = false;

Should we ASSERT that m_guard == true here?

[Adam Barth]

> Naming this thing is gonna be hard… ScopedAssignment? e.g.:
> 
>     ASSERT(!m_inBar);
>     ScopedAssignment guard(m_inBar, true);

Is't that just WTF::TemporaryChange?

[Kwang Yul Seo]

(In reply to comment #5)
> Have you considered building this class using WTF's TemporaryChange?

Yes, but I thought that this class is very small and readable without using TemporaryChange.

> > Source/WTF/wtf/RecursionGuard.h:49
> > +        m_guard = false;
> 
> Should we ASSERT that m_guard == true here?

It's a good idea. I will add ASSERT(m_guard).

[Kwang Yul Seo]

(In reply to comment #6)
> > Naming this thing is gonna be hard… ScopedAssignment? e.g.:
> > 
> >     ASSERT(!m_inBar);
> >     ScopedAssignment guard(m_inBar, true);
> 
> Is't that just WTF::TemporaryChange?

Yeah, it's disguised WTF::TemporaryChange.

[zalan]

(In reply to comment #4)
> What I would expect from something named RecursionGuard is an early return if we have recursion (RecursionGuard = guard that prevents recursion).
> 
> To prevent recursion, we would not even need an attribute, we could just walk up the stack frames until we find the current address.
> 
> It is also not preventing recursion, but re-entry. If you have two methods with the "RecursionGuard", entering the first then the second would cause an assertion.
> 
> Naming this thing is gonna be hard… ScopedAssignment? e.g.:
> 
>     ASSERT(!m_inBar);
>     ScopedAssignment guard(m_inBar, true);

I'd also expect an early return from something named RecursionGuard and would mistakenly replace code this like:
void FrameView::layout(bool allowSubtree)
{
    if (m_inLayout)
        return;

[Build Bot]

Comment on attachment 207427 [details]
Patch

Attachment 207427 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1255345

New failing tests:
editing/selection/leak-document-with-selection-inside.html

[Build Bot]

Created attachment 207576 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Mikhail Pozdnyakov]

Generic WTF::TemporaryChange might look like an overhead here.. maybe something like this would make sense? 

template<>
class TemporaryChange<bool> {
    WTF_MAKE_NONCOPYABLE(TemporaryChange);
public:
    TemporaryChange(bool& scopedVariable)
        : m_scopedVariable(scopedVariable)
    {
        m_scopedVariable = !m_scopedVariable;
    }

    ~TemporaryChange()
    {
        m_scopedVariable = !m_scopedVariable;
    }

private:
    bool& m_scopedVariable;
};

[Alexey Proskuryakov]

Comment on attachment 207427 [details]
Patch

Looks like there is consensus that this shouldn't be landed as is, marking r- to clean up review queue.