Bug 59816

Summary: [Refactoring] Replace Node's Document pointer with a TreeScope pointer
Product: WebKit Reporter: Roland Steiner <rolandsteiner>
Component: DOMAssignee: Elliott Sprehn <esprehn>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, barraclough, cmarcelo, darin, dglazkov, dominicc, esprehn, ggaren, gns, mifenton, morrita, ojan.autocc, philn, rakuco, rniwa, rolandsteiner, simon.fraser, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 73800, 74067, 75240, 75241, 75290, 92048, 92049, 104859    
Bug Blocks: 72352, 87034, 90913    
Attachments:
Description Flags
Patch
none
Patch
none
Just for examining ews
none
Patch
none
Patch
none
Updated to ToT
none
WIP: Naive version
none
WIP: Naive version
none
WIP: with a few peephole optimizations
none
WIP: Almost working
none
Yet another trial with a tag bit
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
WIP
none
WIP
none
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch
none
Patch none

Description Roland Steiner 2011-04-29 11:33:31 PDT
Replace the Document* in Node with a TreeScope*. This removes the need for NodeRareData in all shadow tree nodes, while access to the Document is still O(1) via each node's TreeScope. 

This does, however, require that nodes that are associated with a document but not in the tree are put in a special TreeScope. Likewise DocumentFragments would need to become TreeScopes.
Comment 1 Dominic Cooney 2011-11-20 18:27:16 PST
This is not specific to the content element.
Comment 2 Hajime Morrita 2011-12-04 22:49:10 PST
Starting to work on this.
Comment 3 Hajime Morrita 2012-01-10 01:20:33 PST
Created attachment 121810 [details]
Patch
Comment 4 Early Warning System Bot 2012-01-10 01:40:58 PST
Comment on attachment 121810 [details]
Patch

Attachment 121810 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11166919
Comment 5 Gyuyoung Kim 2012-01-10 01:47:00 PST
Comment on attachment 121810 [details]
Patch

Attachment 121810 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11172381
Comment 6 Roland Steiner 2012-01-10 01:53:50 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=121810&action=review

I like this patch very much - two big thumbs up! Just some minor nits inline.

> Source/WebCore/dom/Document.h:1490
> +inline bool TreeScope::isDocumentScope() const

Can't this be moved to TreeScope.h?

> Source/WebCore/dom/TreeScope.h:42
> +    Document* rootTreeScope() const { return m_rootTreeScope; }

Why not simply name it 'document()' and 'm_document', respectively?
Comment 7 Gustavo Noronha (kov) 2012-01-10 02:06:44 PST
Comment on attachment 121810 [details]
Patch

Attachment 121810 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11183917
Comment 8 Hajime Morrita 2012-01-10 02:52:28 PST
Created attachment 121818 [details]
Patch
Comment 9 Hajime Morrita 2012-01-10 02:54:54 PST
Hi Roland, thanks for taking a look and supporting this!
I just updated the patch to address your points.

> > Source/WebCore/dom/Document.h:1490
> > +inline bool TreeScope::isDocumentScope() const
> 
> Can't this be moved to TreeScope.h?
Sure. Done.

> 
> > Source/WebCore/dom/TreeScope.h:42
> > +    Document* rootTreeScope() const { return m_rootTreeScope; }
> 
> Why not simply name it 'document()' and 'm_document', respectively?
Sounds reasonable. Initially I thought it hide Node::document()
but actually there is no harm in practice.
Comment 10 Early Warning System Bot 2012-01-10 03:18:05 PST
Comment on attachment 121818 [details]
Patch

Attachment 121818 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11186933
Comment 11 Gyuyoung Kim 2012-01-10 03:30:22 PST
Comment on attachment 121818 [details]
Patch

Attachment 121818 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11186937
Comment 12 Gustavo Noronha (kov) 2012-01-10 03:47:41 PST
Comment on attachment 121818 [details]
Patch

Attachment 121818 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11197001
Comment 13 WebKit Review Bot 2012-01-10 03:53:41 PST
Comment on attachment 121818 [details]
Patch

Attachment 121818 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11184994
Comment 14 Dimitri Glazkov (Google) 2012-01-10 08:50:41 PST
Comment on attachment 121818 [details]
Patch

This patch may have performance implications, right? If so, it needs to be tested for perf regressions.
Comment 15 Hajime Morrita 2012-01-10 17:29:09 PST
(In reply to comment #14)
> (From update of attachment 121818 [details])
> This patch may have performance implications, right? If so, it needs to be tested for perf regressions.
Fair enough. I'll try - and I should update the patch to make the bots happy anyway...
Comment 16 Hajime Morrita 2012-01-10 21:03:23 PST
Created attachment 121969 [details]
Just for examining  ews
Comment 17 Darin Adler 2012-01-11 11:01:33 PST
Comment on attachment 121969 [details]
Just for examining  ews

Since Morrita says this is for EWS only, clearing review flag.
Comment 18 Adam Barth 2012-01-11 11:04:19 PST
@morita: You can send a patch to the EWS without marking it for review.  If you don't mark the patch for review, there's a button you can click (where the green and red bubbles usually are) that sends it to the EWS.
Comment 19 Hajime Morrita 2012-01-11 17:08:17 PST
@darin, @abarth.
Ah, sure. I should have done so. Thanks for your advice!
Comment 20 Hajime Morrita 2012-02-12 20:39:54 PST
Created attachment 126707 [details]
Patch
Comment 21 Hajime Morrita 2012-03-29 04:46:02 PDT
Created attachment 134547 [details]
Patch
Comment 22 Hajime Morrita 2012-03-29 04:47:43 PDT
I couldn't resolve performance regression and give this up.
Will try another idea.
Comment 23 Hajime Morrita 2012-06-13 00:24:20 PDT
Reopening to attach new patch.
Comment 24 Hajime Morrita 2012-06-13 00:24:24 PDT
Created attachment 147249 [details]
Updated to ToT
Comment 25 Hajime Morrita 2012-06-27 01:57:01 PDT
Created attachment 149707 [details]
WIP: Naive version
Comment 26 Hajime Morrita 2012-06-27 02:12:35 PDT
Created attachment 149709 [details]
WIP: Naive version
Comment 27 Hajime Morrita 2012-06-29 00:46:48 PDT
Created attachment 150106 [details]
WIP: with a few peephole optimizations
Comment 28 Hajime Morrita 2012-06-29 04:34:05 PDT
Created attachment 150138 [details]
WIP: Almost working
Comment 29 Hajime Morrita 2012-07-04 21:54:22 PDT
Created attachment 150872 [details]
Yet another trial with a tag bit
Comment 30 Hajime Morrita 2012-07-04 23:14:42 PDT
Created attachment 150879 [details]
Patch
Comment 31 Build Bot 2012-07-04 23:45:54 PDT
Comment on attachment 150879 [details]
Patch

Attachment 150879 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13139736
Comment 32 Gustavo Noronha (kov) 2012-07-05 00:39:40 PDT
Comment on attachment 150879 [details]
Patch

Attachment 150879 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13144040
Comment 33 Build Bot 2012-07-05 03:00:09 PDT
Comment on attachment 150879 [details]
Patch

Attachment 150879 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13141113
Comment 34 Build Bot 2012-07-05 03:04:55 PDT
Comment on attachment 150879 [details]
Patch

Attachment 150879 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13151004
Comment 35 Hajime Morrita 2012-07-09 19:08:19 PDT
Created attachment 151386 [details]
Patch
Comment 36 Roland Steiner 2012-07-09 20:24:49 PDT
Comment on attachment 151386 [details]
Patch

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

Nice patch! :) Added some general comments on TaggedPtr.

> Source/WTF/wtf/TaggedPtr.h:26
> +

Given this is in a general library, I think this needs introductory documentation on what it does and why it's useful.

> Source/WTF/wtf/TaggedPtr.h:39
> +    explicit TaggedPtr(T* ptr, bool tag = false) { set(ptr, tag); }

With Tag = 1, this pointer cannot point to odd addresses - that means, it cannot be used for chars at all, nor for other stuff that is byte-aligned (e.g., #pragma pack) Now, I don't think this is necessarily a fatal issue, but it should be documented and/or - even better - prevented by template magic.

Additionally, this needs to answer the question whether NULL can be tagged. If not, it should be documented and prevented. If yes, then IMHO this class should really provide operator! and conversion to UnspecifiedBoolType (see RefPtr.h) that takes the tag into consideration. (Personally I'd add the latter methods in any case, since it's generally useful.)

> Source/WTF/wtf/TaggedPtr.h:68
> +

Adding setTagged/setUntagged versions might be nice, to remove the check when the tag state is known a-priori.
Comment 37 Hajime Morrita 2012-07-09 20:42:34 PDT
Comment on attachment 151386 [details]
Patch

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

Thanks for the feedback, Roland!

>> Source/WTF/wtf/TaggedPtr.h:26
>> +
> 
> Given this is in a general library, I think this needs introductory documentation on what it does and why it's useful.

Well, I don't want people to use this more. 
This is necessary evil without which we cannot avoid the performance regression in this specific change.
It prevent debugger to inspect the pointer.
So this unfriendliness is kinda intentional.

>> Source/WTF/wtf/TaggedPtr.h:39
>> +    explicit TaggedPtr(T* ptr, bool tag = false) { set(ptr, tag); }
> 
> With Tag = 1, this pointer cannot point to odd addresses - that means, it cannot be used for chars at all, nor for other stuff that is byte-aligned (e.g., #pragma pack) Now, I don't think this is necessarily a fatal issue, but it should be documented and/or - even better - prevented by template magic.
> 
> Additionally, this needs to answer the question whether NULL can be tagged. If not, it should be documented and prevented. If yes, then IMHO this class should really provide operator! and conversion to UnspecifiedBoolType (see RefPtr.h) that takes the tag into consideration. (Personally I'd add the latter methods in any case, since it's generally useful.)

The alignment of the address is checked in ASSERT() in set(). I have no idea how to catch it in the compilation time.
The value allows to be NULL. Since client cannot refer the raw value, additional operator seems a bit overkill. We can just use get().
Again, I don't want this to be used broadly. And helping easier use doesn't match that intention.

>> Source/WTF/wtf/TaggedPtr.h:68
>> +
> 
> Adding setTagged/setUntagged versions might be nice, to remove the check when the tag state is known a-priori.

Currently no one needs it. So it can be done later.
Maybe I should remove the default parameter. It's misleading.
Comment 38 Simon Fraser (smfr) 2012-07-10 18:14:26 PDT
Comment on attachment 151386 [details]
Patch

r-. We don't want bits stuffed into pointers. This breaks lots of tools.
Comment 39 Ryosuke Niwa 2012-07-10 18:27:01 PDT
Comment on attachment 151386 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        - In shadow tree, Each node has its tree scope in ElementRareData,

Nit: Each should not be capitalized.

> Source/WebCore/ChangeLog:15
> +        This change get rid of ElementRareData::m_treeScope by replacing

Nit: gets rid of.

> Source/WebCore/ChangeLog:16
> +        Node::m_document with Node::m_treeScope. And retrieves the

Nit: And what retrieves the document?

> Source/WebCore/ChangeLog:28
> +        - It makes Node::m_treeScope a tagged pointer, whose tag implies
> +          that m_treeScope refers ShadowRoot, not Document. If the pointer
> +          isn't tagged, we can assume that m_treeScope is actually a
> +          Document and can return it as a Document rather than invoking
> +          TreeScope::roootDocument(). This eliminates an extra dereference
> +          in many case.

Per IRC discussion, we should try node flag first.

> Source/WebCore/dom/Document.cpp:496
> +    setRootDocument(this);

Why do we need to set the document again here? Isn't the call to TreeScope's constructor sufficient?
In fact, I don't think we need setRootDocument.
Comment 40 Hajime Morrita 2012-07-10 20:31:15 PDT
Created attachment 151583 [details]
Patch
Comment 41 Ryosuke Niwa 2012-07-10 20:44:44 PDT
Comment on attachment 151583 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +          that means m_treeScope is actually a document an can be returned as the result.

Nit: s/an /and /.

> Source/WebCore/ChangeLog:25
> +          this eliminates an extract dereference.

What are you referring to by "an extract dereference"?

> Source/WebCore/ChangeLog:36
> +        * dom/Document.cpp: Took care of connectio betwen TreeScope.

Nit: s/connectio/connection/

> Source/WebCore/dom/Document.h:1537
> +inline Document* Node::documentInternal() const

"Internal" is rather a vague adjective. I'd prefer using a more descriptive name like documentWithoutNodeTypeCheck.

> Source/WebCore/dom/Document.h:1565
> +    , m_treeScope(0)

Do we need to initialize it if we're calling setTreeScope? It seems redundant.

> Source/WebCore/dom/Node.h:731
> +    Document* documentInternal() const;

This should certainly be private, no?
Comment 42 Early Warning System Bot 2012-07-10 20:53:54 PDT
Comment on attachment 151583 [details]
Patch

Attachment 151583 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13215010
Comment 43 Hajime Morrita 2012-07-10 21:00:20 PDT
Created attachment 151585 [details]
Patch
Comment 44 WebKit Review Bot 2012-07-10 21:01:59 PDT
Attachment 151585 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/inspector/PageConsoleAgent.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 45 Hajime Morrita 2012-07-10 21:52:24 PDT
Created attachment 151593 [details]
Patch
Comment 46 Hajime Morrita 2012-07-17 01:26:42 PDT
Created attachment 152715 [details]
Patch
Comment 47 Roland Steiner 2012-07-17 02:24:55 PDT
Comment on attachment 152715 [details]
Patch

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

Just a few drive-by remarks. :)

> Source/WebCore/dom/Document.h:1544
> +    return m_treeScope == TreeScope::nullInstance() ? 0 : m_treeScope;

I'm wondering if it wouldn't be OK to just return the nullInstance (?). This would remove yet another condition here as well as below. (Perhaps have a TreeScopePointer wrapper class that implements operator! et al)

> Source/WebCore/dom/Document.h:1548
> +{

If the nullInstance is returned and used throughout, one could just ASSERT(scope) here and take the scope without further condition (?).

> Source/WebCore/dom/Document.h:1576
> +    return treeScope() != document();

Couldn't you just 'return getFlag(InShadowTree);'?

> Source/WebCore/dom/TreeScope.cpp:58
> +    , m_parentTreeScope(rootNode == rootDocument ? 0 : rootDocument)

Given the above, could use nullInstance instead of 0 here?

> Source/WebCore/dom/TreeScope.cpp:67
> +    , m_parentTreeScope(0)

Given the above, could initialize m_parentTreeScope to nullInstance here?
Comment 48 Hajime Morrita 2012-07-17 03:31:21 PDT
Comment on attachment 152715 [details]
Patch

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

Hi Roland, thanks for your feedback! I'll update the patch shortly.

>> Source/WebCore/dom/Document.h:1544
>> +    return m_treeScope == TreeScope::nullInstance() ? 0 : m_treeScope;
> 
> I'm wondering if it wouldn't be OK to just return the nullInstance (?). This would remove yet another condition here as well as below. (Perhaps have a TreeScopePointer wrapper class that implements operator! et al)

Let callers take care of nullInstance() doen't look good idea for me.
I'd like to keep it an implementation detail considering that
- the instance is mutable.
- the instance violates some invariant like the m_rootDocument never be null.

>> Source/WebCore/dom/Document.h:1548
>> +{
> 
> If the nullInstance is returned and used throughout, one could just ASSERT(scope) here and take the scope without further condition (?).

As I mentioned above, I don't want use nullInstance() widely.

>> Source/WebCore/dom/Document.h:1576
>> +    return treeScope() != document();
> 
> Couldn't you just 'return getFlag(InShadowTree);'?

Good catch! Will fix.

>> Source/WebCore/dom/TreeScope.cpp:67
>> +    , m_parentTreeScope(0)
> 
> Given the above, could initialize m_parentTreeScope to nullInstance here?

We cannot do that because this constructor is exactly for nullInstance()
Comment 49 Hajime Morrita 2012-07-17 03:58:04 PDT
Created attachment 152727 [details]
Patch
Comment 50 Ryosuke Niwa 2012-07-18 17:50:51 PDT
Comment on attachment 152727 [details]
Patch

This looks reasonable to me. Hopefully, I'm not failing on my review again here...
Comment 51 Hajime Morrita 2012-07-18 17:54:38 PDT
> (From update of attachment 152727 [details])
> This looks reasonable to me. Hopefully, I'm not failing on my review again here...
I hope so too.
Will wait one more day before landing and have a time to hear voices from veterans like Darin.
Comment 52 Hajime Morrita 2012-07-19 22:10:44 PDT
Created attachment 153407 [details]
Patch for landing
Comment 53 WebKit Review Bot 2012-07-19 23:34:07 PDT
Comment on attachment 153407 [details]
Patch for landing

Clearing flags on attachment: 153407

Committed r123184: <http://trac.webkit.org/changeset/123184>
Comment 54 WebKit Review Bot 2012-07-19 23:34:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 55 WebKit Review Bot 2012-07-23 17:39:09 PDT
Re-opened since this is blocked by 92048
Comment 56 Hajime Morrita 2012-08-08 04:06:20 PDT
Created attachment 157171 [details]
WIP
Comment 57 Hajime Morrita 2012-08-08 04:12:14 PDT
Created attachment 157174 [details]
WIP
Comment 58 Hajime Morrita 2012-08-10 02:33:11 PDT
A profiler told me that Node::document() is hit not only from DOM side but also from rendering side
through RenderObject::document(). This is why Dromaeo couln't catch the slow down of the page cycler.
Comment 59 Hajime Morrita 2012-09-20 18:06:46 PDT
I have no idea how we can do it. Closing for now.
Feel free to reopen and attack this if you come up with any possible ideas.
Comment 60 Elliott Sprehn 2012-12-06 18:47:32 PST
Per rniwa's request lets try this again.
Comment 61 Elliott Sprehn 2012-12-06 19:12:10 PST
Created attachment 178136 [details]
Patch
Comment 62 Elliott Sprehn 2012-12-06 19:26:47 PST
There's no justification for not doing a pointer deref inside document in the original patch so I didn't do that as it makes this simpler. Morrita do you have a benchmark where you saw the pointer deref being a perf regression? Also what page cycler did you see the regression on?
Comment 63 Ryosuke Niwa 2012-12-06 19:45:36 PST
Comment on attachment 178136 [details]
Patch

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

This looks right. I’m hoping that you can give us some perf. test results before landing it so that we have some confidence in this new patch.
I’ll wait for other reviewers (particularly morrita & darin) to comment.

> Source/WebCore/dom/Document.h:1564
> +        m_treeScope = TreeScope::noDocumentInstance();

We might want to consider inlining this function butI guess it’s rare to instantiate a node without a document that it doesn’t matter?

> Source/WebCore/dom/TreeScope.h:113
> +    TreeScope();

Why do we need this?

> Source/WebCore/dom/TreeScopeAdopter.cpp:-100
>  
> -    node->setDocument(newDocument);
> -

I think it deserves a change log comment saying that this node can’t be tree scope here.
Comment 64 Ryosuke Niwa 2012-12-06 19:46:26 PST
(In reply to comment #63)
> > Source/WebCore/dom/TreeScope.h:113
> > +    TreeScope();
> 
> Why do we need this?

Forget about this comment. It was added before I saw noDocumentInstance().
Comment 65 Hajime Morrita 2012-12-06 20:13:43 PST
Here is a bug which reported the perf regression https://crbug.com/138410
Comment 66 Elliott Sprehn 2012-12-06 20:23:01 PST
(In reply to comment #63)
> (From update of attachment 178136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review
> ...
> 
> > Source/WebCore/dom/TreeScopeAdopter.cpp:-100
> >  
> > -    node->setDocument(newDocument);
> > -
> 
> I think it deserves a change log comment saying that this node can’t be tree scope here.

I don't understand what you mean, what do you want me to explain?
Comment 67 Ryosuke Niwa 2012-12-06 20:23:22 PST
(In reply to comment #65)
> Here is a bug which reported the perf regression https://crbug.com/138410

Hm… the regression is 2%. That might be too small of a difference to detect on a non-bot environment.
Comment 68 Ryosuke Niwa 2012-12-06 20:24:14 PST
(In reply to comment #66)
> (In reply to comment #63)
> > (From update of attachment 178136 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review
> > ...
> > 
> > > Source/WebCore/dom/TreeScopeAdopter.cpp:-100
> > >  
> > > -    node->setDocument(newDocument);
> > > -
> > 
> > I think it deserves a change log comment saying that this node can’t be tree scope here.
> 
> I don't understand what you mean, what do you want me to explain?

We don’t have to call setDocument here since node can’t be a TreeScope but that isn’t obvious from the code change. It’ll be good to point that out in the change log.
Comment 69 Elliott Sprehn 2012-12-06 20:35:36 PST
(In reply to comment #68)
>
> 
> We don’t have to call setDocument here since node can’t be a TreeScope but that isn’t obvious from the code change. It’ll be good to point that out in the change log.

node can be a TreeScope here, we call this method for every ShadowRoot as well. The reason for not needing to call setDocument is because doing setDocumentScope implicitly changes the document for every descendant in the same scope.
Comment 70 Ryosuke Niwa 2012-12-06 20:41:51 PST
(In reply to comment #69)
> (In reply to comment #68)
> >
> > We don’t have to call setDocument here since node can’t be a TreeScope but that isn’t obvious from the code change. It’ll be good to point that out in the change log.
> 
> node can be a TreeScope here, we call this method for every ShadowRoot as well. The reason for not needing to call setDocument is because doing setDocumentScope implicitly changes the document for every descendant in the same scope.

I’m talking about the case when moveNodeToNewDocument is called directly from moveTreeToNewScope at line 60-61. On my second thought, this could be a bug.
Comment 71 Elliott Sprehn 2012-12-06 20:45:24 PST
(In reply to comment #70)
> (In reply to comment #69)
> > (In reply to comment #68)
> > >
> > > We don’t have to call setDocument here since node can’t be a TreeScope but that isn’t obvious from the code change. It’ll be good to point that out in the change log.
> > 
> > node can be a TreeScope here, we call this method for every ShadowRoot as well. The reason for not needing to call setDocument is because doing setDocumentScope implicitly changes the document for every descendant in the same scope.
> 
> I’m talking about the case when moveNodeToNewDocument is called directly from moveTreeToNewScope at line 60-61. On my second thought, this could be a bug.

We have to call it to maintain the guard refs and update the node lists after setting the tree scope and then to notify the node with didMoveToNewDocument. If the document changed we need to call moveNodeToNewDocument on every node in the top level tree, and then on every node all the way down through every shadow root.
Comment 72 Ryosuke Niwa 2012-12-06 20:48:18 PST
(In reply to comment #71)
> We have to call it to maintain the guard refs and update the node lists after setting the tree scope and then to notify the node with didMoveToNewDocument. If the document changed we need to call moveNodeToNewDocument on every node in the top level tree, and then on every node all the way down through every shadow root.

I know we have to call moveNodeToNewDocument there. What’s not obvious is why we don’t have to call either setDocument or setDocumentScope.
Comment 73 Elliott Sprehn 2012-12-06 21:04:41 PST
(In reply to comment #72)
> (In reply to comment #71)
> > We have to call it to maintain the guard refs and update the node lists after setting the tree scope and then to notify the node with didMoveToNewDocument. If the document changed we need to call moveNodeToNewDocument on every node in the top level tree, and then on every node all the way down through every shadow root.
> 
> I know we have to call moveNodeToNewDocument there. What’s not obvious is why we don’t have to call either setDocument or setDocumentScope.

Hmm, it seems we do attempt to adopt ShadowRoot's during removeAllShadowRoots and we notify of their removal which is somewhat concerning because it means during that notification shadowRoot->treeScope() is not shadowRoot, but actually the document. This seems wrong since normally shadowRoot->treeScope() == shadowRoot but it's always been this way. That might be a bug, but I don't know how you observe the behavior.

That's the only time node can ever be a TreeScope.
Comment 74 Elliott Sprehn 2012-12-06 21:16:33 PST
Created attachment 178154 [details]
Patch

Never end up in a situation where a TreeScope is detached. Also addressed rniwa's other comments.
Comment 75 Ojan Vafai 2012-12-06 21:29:49 PST
Comment on attachment 178136 [details]
Patch

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

I like it!

> Source/WebCore/dom/TreeScope.h:63
> +    Document* documentScope() const { return m_documentScope; }

Can we s/documentScope/document here? I think that would read a lot better in the calling locations.
Comment 76 Ryosuke Niwa 2012-12-06 21:32:06 PST
Comment on attachment 178136 [details]
Patch

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

>> Source/WebCore/dom/TreeScope.h:63
>> +    Document* documentScope() const { return m_documentScope; }
> 
> Can we s/documentScope/document here? I think that would read a lot better in the calling locations.

I don’t think we want do that given Document & ShdowRoot each of which inherits from TreeScope also has document().
Comment 77 Ryosuke Niwa 2012-12-10 21:30:12 PST
Comment on attachment 178154 [details]
Patch

Alright, why don't we try this one since it looks promising.
Comment 78 Elliott Sprehn 2012-12-12 11:53:55 PST
(In reply to comment #76)
> (From update of attachment 178136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178136&action=review
> 
> ...
> > Source/WebCore/dom/TreeScope.h:63
> > +    Document* documentScope() const { return m_documentScope; }
> 
> Can we s/documentScope/document here? I think that would read a lot better in the calling locations.

Would you prefer rootScope? Like rniwa said, I didn't do that since we already have a document method on Node.
Comment 79 WebKit Review Bot 2012-12-12 12:06:33 PST
Comment on attachment 178154 [details]
Patch

Rejecting attachment 178154 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 #1 succeeded at 55 (offset 1 line).
Hunk #2 succeeded at 112 (offset 1 line).
patching file Source/WebCore/dom/TreeScope.h
patching file Source/WebCore/dom/TreeScopeAdopter.cpp
Hunk #1 FAILED at 42.
Hunk #2 succeeded at 97 (offset 1 line).
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/dom/TreeScopeAdopter.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Ryosuke Ni..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/15282622
Comment 80 Elliott Sprehn 2012-12-12 12:24:31 PST
Created attachment 179102 [details]
Patch for landing
Comment 81 WebKit Review Bot 2012-12-12 14:27:53 PST
Comment on attachment 179102 [details]
Patch for landing

Rejecting attachment 179102 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ataBase(WebCore::Document*&)'
Source/WebCore/dom/Node.h:120: note: candidates are: WebCore::NodeRareDataBase::NodeRareDataBase()
Source/WebCore/dom/Node.h:113: note:                 WebCore::NodeRareDataBase::NodeRareDataBase(const WebCore::NodeRareDataBase&)
make: *** [out/Release/obj.target/webcore_dom/Source/WebCore/dom/ClassNodeList.o] Error 1

Failed to run "['Tools/Scripts/build-webkit', '--release', '--chromium', '--update-chromium']" exit_code: 2
rce/WebCore/dom/ClassNodeList.o] Error 1

Full output: http://queues.webkit.org/results/15281669
Comment 82 Elliott Sprehn 2012-12-12 14:36:20 PST
Created attachment 179129 [details]
Patch

Fix the merge for real this time. It seems gits automerge put the Node::setDocument method back in Node.cpp when I synced last time
Comment 83 WebKit Review Bot 2012-12-12 15:21:23 PST
Comment on attachment 179129 [details]
Patch

Clearing flags on attachment: 179129

Committed r137524: <http://trac.webkit.org/changeset/137524>
Comment 84 WebKit Review Bot 2012-12-12 15:21:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 85 WebKit Review Bot 2012-12-12 16:16:00 PST
Re-opened since this is blocked by bug 104859
Comment 86 Elliott Sprehn 2012-12-12 18:20:18 PST
Created attachment 179174 [details]
Patch
Comment 87 Simon Fraser (smfr) 2012-12-12 18:21:04 PST
Did you run all the tests in Debug mode?
Comment 88 Elliott Sprehn 2012-12-12 18:27:26 PST
(In reply to comment #87)
> Did you run all the tests in Debug mode?

I didn't request a commit because I haven't run all the tests yet, I did run fast/mutation and fast/dom though and this fixes it. Right now I can't get it to build because r137539 broke the chromium build, so I'll run them later tonight/tomorrow and update.
Comment 89 Elliott Sprehn 2012-12-12 18:48:45 PST
(In reply to comment #88)
> (In reply to comment #87)
> > Did you run all the tests in Debug mode?
> 
> I didn't request a commit because I haven't run all the tests yet, I did run fast/mutation and fast/dom though and this fixes it. Right now I can't get it to build because r137539 broke the chromium build, so I'll run them later tonight/tomorrow and update.

run-webkit-tests --chromium --debug --no-pixel-tests  LayoutTests/fast/

9502 tests executed and I didn't see any crashes.
Comment 90 Ryosuke Niwa 2012-12-12 20:04:14 PST
Comment on attachment 179174 [details]
Patch

(In reply to comment #89)
> (In reply to comment #88)
> > (In reply to comment #87)
> > > Did you run all the tests in Debug mode?
> > 
> > I didn't request a commit because I haven't run all the tests yet, I did run fast/mutation and fast/dom though and this fixes it. Right now I can't get it to build because r137539 broke the chromium build, so I'll run them later tonight/tomorrow and update.
> 
> run-webkit-tests --chromium --debug --no-pixel-tests  LayoutTests/fast/
> 
> 9502 tests executed and I didn't see any crashes.

For the change of this nature that could affect literally everything in DOM, it’s not acceptable to run just a subset of tests.

WebKit contribution policy that contributors run all layout tests prior to landing the patch: http://www.webkit.org/quality/testing.html
"Before patches can land in any of the frameworks in the repository, the layout regression tests must pass. To run these tests, execute the run-webkit-tests script."

In practice, you may get away with it by running a subset if you know that the patch only affects a subset of tests in advance but we can’t necessarily do that all the time.

Please run the entire layout test suite and report the result (I typically start run-webkit-tests before I go to lunch or go home).
Comment 91 Elliott Sprehn 2013-01-02 18:30:18 PST
Created attachment 181128 [details]
Patch
Comment 92 Elliott Sprehn 2013-01-02 18:31:25 PST
(In reply to comment #91)
> Created an attachment (id=181128) [details]
> Patch

I ran all 27k tests in debug and there were no crashes.
Comment 93 WebKit Review Bot 2013-01-03 13:40:30 PST
Comment on attachment 181128 [details]
Patch

Clearing flags on attachment: 181128

Committed r138735: <http://trac.webkit.org/changeset/138735>
Comment 94 WebKit Review Bot 2013-01-03 13:40:42 PST
All reviewed patches have been landed.  Closing bug.