Bug 86157 - Should have Node::inDetach() for assertion purposes.
: Should have Node::inDetach() for assertion purposes.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Hajime Morrita
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 16:47 PDT by Hajime Morrita
Modified: 2012-05-14 00:49 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-05-10 16:47:19 PDT
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 Darin Adler 2012-05-10 17:26:11 PDT
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 Hajime Morrita 2012-05-10 18:24:55 PDT
Created attachment 141306 [details]
Patch
Comment 3 Hajime Morrita 2012-05-10 18:27:10 PDT
Created attachment 141307 [details]
Patch
Comment 4 Hajime Morrita 2012-05-10 18:27:31 PDT
Hi Darin, could you take a look at this small piece?
Comment 5 Darin Adler 2012-05-12 08:42:44 PDT
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.
Comment 6 Darin Adler 2012-05-12 08:43:31 PDT
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 Hajime Morrita 2012-05-13 17:47:25 PDT
Created attachment 141621 [details]
Patch
Comment 8 Hajime Morrita 2012-05-13 17:51:31 PDT
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 Hajime Morrita 2012-05-13 23:55:53 PDT
Comment on attachment 141621 [details]
Patch

Thanks for quick review! landing...
Comment 10 WebKit Review Bot 2012-05-14 00:49:15 PDT
Comment on attachment 141621 [details]
Patch

Clearing flags on attachment: 141621

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