Bug 86157 – Should have Node::inDetach() for assertion purposes.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

This is followup on Bug 85963 where we removed Node::inDetach().
The rationale of the removal is that inDetach() should never be true for its caller.
But it's safe to assert() it instead of just completely remove it for a while.

[Darin Adler]

I don’t think we have to switch to an assertion. If we can prove to ourselves somehow that Node::detach does not call out to anything that can run “arbitrary” code, then we’re OK.

But <http://trac.webkit.org/changeset/116644> seems to have assumed this without investigating and proving it!

[Hajime Morrita]

Created attachment 141306 [details]
Patch

[Hajime Morrita]

Created attachment 141307 [details]
Patch

[Hajime Morrita]

Hi Darin, could you take a look at this small piece?

[Darin Adler]

Comment on attachment 141307 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:1343
> +static Node* detachingNode;
> +
> +bool Node::inDetach() const
> +{
> +    return detachingNode == this;
> +}

This global variable and the body of the inDetach function should be NDEBUG-only. We don’t want to pay the price for setting a global used only for assertions in a build without assertions.

[Darin Adler]

While an assertion is OK, what I’m really interested in is studying the code thinking through whether this problem is guaranteed not to happen rather than trying to prove that it doesn’t happen by testing with an assertion in place.

[Hajime Morrita]

Created attachment 141621 [details]
Patch

[Hajime Morrita]

Hi darin, thanks for the comment. I updated the patch.

(In reply to comment #6)
> While an assertion is OK, what I’m really interested in is studying the code thinking through whether this problem is guaranteed not to happen rather than trying to prove that it doesn’t happen by testing with an assertion in place.

Here is an investigation:
There are three non-trivial function called from Node::detach()

- Document::hoveredNodeDetached();
- Document::activeChainNodeDetached();
- RnderObject::destroyAndCleanupAnonymousWrappers();

First two Document methods are clearly innocent.
They just start a timer or mutate its internal member variables.

- http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp#L3603
- http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp#L3615

Although  destroyAndCleanupAnonymousWrappers() isn't so obviously innocent, in fact it is:
This is because any focus change won't be initiated by rendering side.
Especially not by anonymous ROs.
It looks there is clear responsibility boundary between dom/ and rendering/ on this focus concept,
and it is DOM side responsibility to maintain the focus, in my understanding.

[Hajime Morrita]

Comment on attachment 141621 [details]
Patch

Thanks for quick review! landing...

[WebKit Review Bot]

Comment on attachment 141621 [details]
Patch

Clearing flags on attachment: 141621

Committed r116927: <http://trac.webkit.org/changeset/116927>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.