Bug 129727 - appendChild shouldn't invalidate LiveNodeLists and HTMLCollections if they don't have valid caches
Summary: appendChild shouldn't invalidate LiveNodeLists and HTMLCollections if they do...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-04 21:43 PST by Ryosuke Niwa
Modified: 2014-03-05 02:36 PST (History)
8 users (show)

See Also:


Attachments
Fixes the bug (23.49 KB, patch)
2014-03-04 22:03 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (467.91 KB, application/zip)
2014-03-04 23:23 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (482.31 KB, application/zip)
2014-03-04 23:57 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (539.46 KB, application/zip)
2014-03-05 00:41 PST, Build Bot
no flags Details
Reverted the erronous change (24.59 KB, patch)
2014-03-05 00:41 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (23.45 KB, patch)
2014-03-05 01:56 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2014-03-04 21:43:45 PST
Right now, invalidateNodeListAndCollectionCachesInAncestors invalidates node lists and HTML collections
on all ancestors whenever we're inserting or removing a node.

We don't have to do this when there are no node lists and HTML collections with valid caches.
Comment 1 Ryosuke Niwa 2014-03-04 22:03:48 PST
Created attachment 225856 [details]
Fixes the bug
Comment 2 Ryosuke Niwa 2014-03-04 22:04:37 PST
A/B test result: http://dromaeo.com/?id=218031,218032
Looks like a 1% progression.
Comment 3 Build Bot 2014-03-04 23:23:37 PST
Comment on attachment 225856 [details]
Fixes the bug

Attachment 225856 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5980174520680448

New failing tests:
dom/html/level2/html/HTMLIsIndexElement02.html
Comment 4 Build Bot 2014-03-04 23:23:39 PST
Created attachment 225862 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2014-03-04 23:57:42 PST
Comment on attachment 225856 [details]
Fixes the bug

Attachment 225856 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5656540950298624

New failing tests:
dom/html/level2/html/HTMLIsIndexElement02.html
Comment 6 Build Bot 2014-03-04 23:57:44 PST
Created attachment 225864 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Build Bot 2014-03-05 00:41:13 PST
Comment on attachment 225856 [details]
Fixes the bug

Attachment 225856 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5973414678364160

New failing tests:
dom/html/level2/html/HTMLIsIndexElement02.html
Comment 8 Build Bot 2014-03-05 00:41:15 PST
Created attachment 225867 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Ryosuke Niwa 2014-03-05 00:41:22 PST
Comment on attachment 225856 [details]
Fixes the bug

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

> Source/WebCore/dom/CollectionIndexCache.h:199
> -    if (index && m_currentNode) {
> +    if (index) {

Oops, this erroneous change caused a crash in dom/html/level2/html/HTMLIsIndexElement02.html
Comment 10 Ryosuke Niwa 2014-03-05 00:41:48 PST
Created attachment 225868 [details]
Reverted the erronous change
Comment 11 Andreas Kling 2014-03-05 01:27:31 PST
Comment on attachment 225868 [details]
Reverted the erronous change

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

Good idea. r=me

> Source/WebCore/dom/Node.cpp:728
> +    HashSet<LiveNodeList*> liveNodeLists;
> +    liveNodeLists.swap(m_listsInvalidatedAtDocument);

I'd write this with move semantics:
HashSet<LiveNodeList*> liveNodeLists = std::move(m_listsInvalidatedAtDocument);

> Source/WebCore/dom/Node.cpp:733
> +    HashSet<HTMLCollection*> collectionLists;
> +    collectionLists.swap(m_collectionsInvalidatedAtDocument);

Same here.
Comment 12 Ryosuke Niwa 2014-03-05 01:56:18 PST
Created attachment 225871 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2014-03-05 02:36:07 PST
Comment on attachment 225871 [details]
Patch for landing

Clearing flags on attachment: 225871

Committed r165103: <http://trac.webkit.org/changeset/165103>
Comment 14 WebKit Commit Bot 2014-03-05 02:36:10 PST
All reviewed patches have been landed.  Closing bug.