Bug 115398 – Simplify ContainerNode::removeChildren

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryosuke Niwa]

Simplify ContainerNode::removeChildren

[Ryosuke Niwa]

Created attachment 200087 [details]
Cleanup

[Andreas Kling]

Comment on attachment 200087 [details]
Cleanup

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

> Source/WebCore/dom/ContainerNode.cpp:610
> -    Vector<RefPtr<Node>, 10> removedChildren;
> +    NodeVector removedChildren;

I think we should get rid of the NodeVector typedef, or at least give it a less silly default capacity.
Since it's always on the stack, we can just as well go with 16, 32 or 64.
The goal is presumably to avoid having to allocate a heap buffer, so why not try a little harder?

[Ryosuke Niwa]

Comment on attachment 200087 [details]
Cleanup

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

>> Source/WebCore/dom/ContainerNode.cpp:610
>> +    NodeVector removedChildren;
> 
> I think we should get rid of the NodeVector typedef, or at least give it a less silly default capacity.
> Since it's always on the stack, we can just as well go with 16, 32 or 64.
> The goal is presumably to avoid having to allocate a heap buffer, so why not try a little harder?

The last time I checked, 11 was the magic number.

[Andreas Kling]

Comment on attachment 200087 [details]
Cleanup

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

>>> Source/WebCore/dom/ContainerNode.cpp:610
>>> +    NodeVector removedChildren;
>> 
>> I think we should get rid of the NodeVector typedef, or at least give it a less silly default capacity.
>> Since it's always on the stack, we can just as well go with 16, 32 or 64.
>> The goal is presumably to avoid having to allocate a heap buffer, so why not try a little harder?
> 
> The last time I checked, 11 was the magic number.

Right, but what is the harm in using a higher number?

[Build Bot]

Comment on attachment 200087 [details]
Cleanup

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

New failing tests:
fast/frames/crash-remove-iframe-during-object-beforeload.html

[Build Bot]

Created attachment 200092 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Ryosuke Niwa]

Stack trace:
0   com.apple.WebCore             	0x0000000106747b24 WTF::KeyValuePair<WebCore::RenderObject*, unsigned int>* WTF::HashTable<WebCore::RenderObject*, WTF::KeyValuePair<WebCore::RenderObject*, unsigned int>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::RenderObject*, unsigned int> >, WTF::PtrHash<WebCore::RenderObject*>, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::RenderObject*>, WTF::HashTraits<unsigned int> >, WTF::HashTraits<WebCore::RenderObject*> >::lookup<WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::RenderObject*> >, WebCore::RenderObject*>(WebCore::RenderObject* const&) + 4 (HashTable.h:609)
1   com.apple.WebCore             	0x00000001067227c4 WebCore::AXObjectCache::get(WebCore::RenderObject*) + 36 (HashTable.h:422)
2   com.apple.WebCore             	0x0000000106722cd7 WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*) + 55 (AXObjectCache.cpp:394)
3   com.apple.WebCore             	0x00000001068ecbe3 WebCore::Document::implicitClose() + 819 (Document.cpp:2469)
4   com.apple.WebCore             	0x0000000106a2d0b1 WebCore::FrameLoader::checkCompleted() + 337 (FrameLoader.cpp:771)
5   com.apple.WebCore             	0x0000000106a2bfcc WebCore::FrameLoader::finishedParsing() + 92 (FrameLoader.cpp:704)
6   com.apple.WebCore             	0x00000001068f56c9 WebCore::Document::finishedParsing() + 313 (Frame.h:324)
7   com.apple.WebCore             	0x0000000106aba198 WebCore::HTMLDocumentParser::prepareToStopParsing() + 168 (HTMLDocumentParser.cpp:756)
8   com.apple.WebCore             	0x00000001069128ed WebCore::DocumentWriter::end() + 61 (RefPtr.h:133)
9   com.apple.WebCore             	0x00000001069015d3 WebCore::DocumentLoader::finishedLoading(double) + 355 (ResourceErrorBase.h:42)
10  com.apple.WebCore             	0x00000001067b488c WebCore::CachedResource::checkNotify() + 76 (CachedResource.cpp:366)
11  com.apple.WebCore             	0x00000001067b1954 WebCore::CachedRawResource::data(WTF::PassRefPtr<WebCore::ResourceBuffer>, bool) + 436 (PassRefPtr.h:68)
12  com.apple.WebCore             	0x00000001072c2640 WebCore::SubresourceLoader::didFinishLoading(double) + 128 (PassRefPtr.h:68)

I don't think this is related to the patch.

[Andrei Bucur]

(In reply to comment #7)
> Stack trace:
> 0   com.apple.WebCore                 0x0000000106747b24 WTF::KeyValuePair<WebCore::RenderObject*, unsigned int>* WTF::HashTable<WebCore::RenderObject*, WTF::KeyValuePair<WebCore::RenderObject*, unsigned int>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::RenderObject*, unsigned int> >, WTF::PtrHash<WebCore::RenderObject*>, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::RenderObject*>, WTF::HashTraits<unsigned int> >, WTF::HashTraits<WebCore::RenderObject*> >::lookup<WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::RenderObject*> >, WebCore::RenderObject*>(WebCore::RenderObject* const&) + 4 (HashTable.h:609)
> 1   com.apple.WebCore                 0x00000001067227c4 WebCore::AXObjectCache::get(WebCore::RenderObject*) + 36 (HashTable.h:422)
> 2   com.apple.WebCore                 0x0000000106722cd7 WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*) + 55 (AXObjectCache.cpp:394)
> 3   com.apple.WebCore                 0x00000001068ecbe3 WebCore::Document::implicitClose() + 819 (Document.cpp:2469)
> 4   com.apple.WebCore                 0x0000000106a2d0b1 WebCore::FrameLoader::checkCompleted() + 337 (FrameLoader.cpp:771)
> 5   com.apple.WebCore                 0x0000000106a2bfcc WebCore::FrameLoader::finishedParsing() + 92 (FrameLoader.cpp:704)
> 6   com.apple.WebCore                 0x00000001068f56c9 WebCore::Document::finishedParsing() + 313 (Frame.h:324)
> 7   com.apple.WebCore                 0x0000000106aba198 WebCore::HTMLDocumentParser::prepareToStopParsing() + 168 (HTMLDocumentParser.cpp:756)
> 8   com.apple.WebCore                 0x00000001069128ed WebCore::DocumentWriter::end() + 61 (RefPtr.h:133)
> 9   com.apple.WebCore                 0x00000001069015d3 WebCore::DocumentLoader::finishedLoading(double) + 355 (ResourceErrorBase.h:42)
> 10  com.apple.WebCore                 0x00000001067b488c WebCore::CachedResource::checkNotify() + 76 (CachedResource.cpp:366)
> 11  com.apple.WebCore                 0x00000001067b1954 WebCore::CachedRawResource::data(WTF::PassRefPtr<WebCore::ResourceBuffer>, bool) + 436 (PassRefPtr.h:68)
> 12  com.apple.WebCore                 0x00000001072c2640 WebCore::SubresourceLoader::didFinishLoading(double) + 128 (PassRefPtr.h:68)
> 
> I don't think this is related to the patch.

FYI, I wasn't able to repro the crash on my machine.

[WebKit Commit Bot]

Comment on attachment 200087 [details]
Cleanup

Clearing flags on attachment: 200087

Committed r149386: <http://trac.webkit.org/changeset/149386>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.