Bug 90414

Summary: Make HTMLCollection RefCounted
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, cmarcelo, darin, dglazkov, fpizlo, haraken, japhet, jochen, kling, koivisto, mifenton, sam, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89919    
Attachments:
Description Flags
Cleanup
none
Fixed builds sam: review+, buildbot: commit-queue-

Ryosuke Niwa
Reported 2012-07-02 16:52:52 PDT
Right now HTMLCollection forwards ref/deref to its owner but this model makes tricky to merge it with DynamicNodeList. Also, we never delete HTMLCollection as long as its owner is alive because no one clears entries in m_cachedCollections, end up consuming a lot of memory over time when the node is left in the tree.
Attachments
Cleanup (46.00 KB, patch)
2012-07-02 17:58 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixed builds (51.72 KB, patch)
2012-07-02 18:43 PDT, Ryosuke Niwa 		sam: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2012-07-02 17:58:30 PDT
Created attachment 150507 [details] Cleanup
WebKit Review Bot
Comment 2 2012-07-02 18:24:33 PDT
Comment on attachment 150507 [details] Cleanup Attachment 150507 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13125324
Build Bot
Comment 3 2012-07-02 18:30:03 PDT
Comment on attachment 150507 [details] Cleanup Attachment 150507 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13138187
Ryosuke Niwa
Comment 4 2012-07-02 18:43:44 PDT
Created attachment 150514 [details] Fixed builds
Build Bot
Comment 5 2012-07-02 19:28:56 PDT
Comment on attachment 150514 [details] Fixed builds Attachment 150514 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13126334
Ryosuke Niwa
Comment 6 2012-07-02 21:12:47 PDT
Comment on attachment 150514 [details] Fixed builds View in context: https://bugs.webkit.org/attachment.cgi?id=150514&action=review > Source/WebKit/win/DOMHTMLClasses.cpp:713 > + RefPtr<HTMLCollection> options = selectElement->options(); Oops, I need to change this to HTMLOptionsCollection.
Ryosuke Niwa
Comment 7 2012-07-06 22:55:19 PDT
Ping reviewers.
Sam Weinig
Comment 8 2012-07-07 12:21:54 PDT
Comment on attachment 150514 [details] Fixed builds View in context: https://bugs.webkit.org/attachment.cgi?id=150514&action=review > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:122 > + return toJS(exec, thisObj, WTF::getPtr(collection)); Could you use release here? > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:80 > + return toJS(exec, thisObj->globalObject(), WTF::getPtr(collection)); Could you use release here?
Ryosuke Niwa
Comment 9 2012-07-08 21:33:06 PDT
(In reply to comment #8) > (From update of attachment 150514 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150514&action=review > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:122 > > + return toJS(exec, thisObj, WTF::getPtr(collection)); > > Could you use release here? > > > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:80 > > + return toJS(exec, thisObj->globalObject(), WTF::getPtr(collection)); > > Could you use release here? I don't think so. At least I don't see such a pattern anywhere else in the binding code.
Ryosuke Niwa
Comment 10 2012-07-09 10:40:14 PDT
Committed r122115: <http://trac.webkit.org/changeset/122115>
Ryosuke Niwa
Comment 11 2012-07-09 11:32:49 PDT
EFL build fix landed in http://trac.webkit.org/changeset/122118.
Ryosuke Niwa
Comment 12 2012-07-09 13:53:26 PDT
Another EFL build fix (microdata tests): http://trac.webkit.org/changeset/122155
Filip Pizlo
Comment 13 2012-07-11 13:23:27 PDT
I suspect this may have caused the WK1 Mac release bot to explode into flames: http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r122115%20(900)/results.html
Ryosuke Niwa
Comment 14 2012-07-11 13:26:34 PDT
(In reply to comment #13) > I suspect this may have caused the WK1 Mac release bot to explode into flames: > > http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r122115%20(900)/results.html That seems unlikely. I use WK1 Mac debug for development, and didn't see any failures like that when I ran the test locally. Is it possible that the bot is sick (some stale file lock, process, etc...) ?
Filip Pizlo
Comment 15 2012-07-11 13:27:39 PDT
(In reply to comment #14) > (In reply to comment #13) > > I suspect this may have caused the WK1 Mac release bot to explode into flames: > > > > http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r122115%20(900)/results.html > > That seems unlikely. I use WK1 Mac debug for development, and didn't see any failures like that when I ran the test locally. > > Is it possible that the bot is sick (some stale file lock, process, etc...) ? Absolutely! I'm checking this now. Don't worry, I'm not hovering over the rollout trigger. ;-)
Note You need to log in before you can comment on or make changes to this bug.