Bug 90414 - Make HTMLCollection RefCounted
Summary: Make HTMLCollection RefCounted
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: 89919
  Show dependency treegraph
 
Reported: 2012-07-02 16:52 PDT by Ryosuke Niwa
Modified: 2012-07-11 13:27 PDT (History)
15 users (show)

See Also:


Attachments
Cleanup (46.00 KB, patch)
2012-07-02 17:58 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed builds (51.72 KB, patch)
2012-07-02 18:43 PDT, Ryosuke Niwa
sam: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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.
Comment 1 Ryosuke Niwa 2012-07-02 17:58:30 PDT
Created attachment 150507 [details]
Cleanup
Comment 2 WebKit Review Bot 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
Comment 3 Build Bot 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
Comment 4 Ryosuke Niwa 2012-07-02 18:43:44 PDT
Created attachment 150514 [details]
Fixed builds
Comment 5 Build Bot 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
Comment 6 Ryosuke Niwa 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.
Comment 7 Ryosuke Niwa 2012-07-06 22:55:19 PDT
Ping reviewers.
Comment 8 Sam Weinig 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?
Comment 9 Ryosuke Niwa 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.
Comment 10 Ryosuke Niwa 2012-07-09 10:40:14 PDT
Committed r122115: <http://trac.webkit.org/changeset/122115>
Comment 11 Ryosuke Niwa 2012-07-09 11:32:49 PDT
EFL build fix landed in http://trac.webkit.org/changeset/122118.
Comment 12 Ryosuke Niwa 2012-07-09 13:53:26 PDT
Another EFL build fix (microdata tests): http://trac.webkit.org/changeset/122155
Comment 13 Filip Pizlo 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
Comment 14 Ryosuke Niwa 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...) ?
Comment 15 Filip Pizlo 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. ;-)