Summary: | Should have Node::inDetach() for assertion purposes. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||
Component: | DOM | Assignee: | Hajime Morrita <morrita> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, koivisto, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Hajime Morrita
2012-05-10 16:47:19 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! Created attachment 141306 [details]
Patch
Created attachment 141307 [details]
Patch
Hi Darin, could you take a look at this small piece? 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. 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. Created attachment 141621 [details]
Patch
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 on attachment 141621 [details]
Patch
Thanks for quick review! landing...
Comment on attachment 141621 [details] Patch Clearing flags on attachment: 141621 Committed r116927: <http://trac.webkit.org/changeset/116927> All reviewed patches have been landed. Closing bug. |