Bug 115398 - Simplify ContainerNode::removeChildren
Summary: Simplify ContainerNode::removeChildren
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-30 00:46 PDT by Ryosuke Niwa
Modified: 2013-04-30 11:43 PDT (History)
7 users (show)

See Also:


Attachments
Cleanup (2.19 KB, patch)
2013-04-30 00:47 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (530.54 KB, application/zip)
2013-04-30 01:54 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-04-30 00:46:10 PDT
Simplify ContainerNode::removeChildren
Comment 1 Ryosuke Niwa 2013-04-30 00:47:32 PDT
Created attachment 200087 [details]
Cleanup
Comment 2 Andreas Kling 2013-04-30 00:52:46 PDT
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?
Comment 3 Ryosuke Niwa 2013-04-30 00:53:42 PDT
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.
Comment 4 Andreas Kling 2013-04-30 00:59:18 PDT
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?
Comment 5 Build Bot 2013-04-30 01:54:00 PDT
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
Comment 6 Build Bot 2013-04-30 01:54:02 PDT
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
Comment 7 Ryosuke Niwa 2013-04-30 11:12:56 PDT
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.
Comment 8 Andrei Bucur 2013-04-30 11:21:18 PDT
(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.
Comment 9 WebKit Commit Bot 2013-04-30 11:43:45 PDT
Comment on attachment 200087 [details]
Cleanup

Clearing flags on attachment: 200087

Committed r149386: <http://trac.webkit.org/changeset/149386>
Comment 10 WebKit Commit Bot 2013-04-30 11:43:48 PDT
All reviewed patches have been landed.  Closing bug.