Bug 86157 - Should have Node::inDetach() for assertion purposes.
: Should have Node::inDetach() for assertion purposes.
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-10 16:47 PST by
Modified: 2012-05-14 00:49 PST (History)


Attachments
Patch (2.89 KB, patch)
2012-05-10 18:24 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (2.90 KB, patch)
2012-05-10 18:27 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (2.98 KB, patch)
2012-05-13 17:47 PST, Hajime Morrita
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-10 16:47:19 PST
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.
------- Comment #1 From 2012-05-10 17:26:11 PST -------
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!
------- Comment #2 From 2012-05-10 18:24:55 PST -------
Created an attachment (id=141306) [details]
Patch
------- Comment #3 From 2012-05-10 18:27:10 PST -------
Created an attachment (id=141307) [details]
Patch
------- Comment #4 From 2012-05-10 18:27:31 PST -------
Hi Darin, could you take a look at this small piece?
------- Comment #5 From 2012-05-12 08:42:44 PST -------
(From update of attachment 141307 [details])
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.
------- Comment #6 From 2012-05-12 08:43:31 PST -------
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.
------- Comment #7 From 2012-05-13 17:47:25 PST -------
Created an attachment (id=141621) [details]
Patch
------- Comment #8 From 2012-05-13 17:51:31 PST -------
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.
------- Comment #9 From 2012-05-13 23:55:53 PST -------
(From update of attachment 141621 [details])
Thanks for quick review! landing...
------- Comment #10 From 2012-05-14 00:49:15 PST -------
(From update of attachment 141621 [details])
Clearing flags on attachment: 141621

Committed r116927: <http://trac.webkit.org/changeset/116927>
------- Comment #11 From 2012-05-14 00:49:20 PST -------
All reviewed patches have been landed.  Closing bug.