RESOLVED FIXED 145753
DeferredWrapper should clear its JS strong references once its promise is resolved/rejected
https://bugs.webkit.org/show_bug.cgi?id=145753
Summary DeferredWrapper should clear its JS strong references once its promise is res...
youenn fablet
Reported 2015-06-08 03:04:22 PDT
Clearing strong references would lower the chances to get stuck in a retain cycle.
Attachments
Patch (21.08 KB, patch)
2015-06-09 06:58 PDT, youenn fablet
no flags
Fixed according review (21.74 KB, patch)
2015-06-09 12:33 PDT, youenn fablet
no flags
Removed redundant assert (21.70 KB, patch)
2015-06-09 12:57 PDT, youenn fablet
no flags
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (579.84 KB, application/zip)
2015-06-09 15:16 PDT, Build Bot
no flags
youenn fablet
Comment 1 2015-06-09 06:58:36 PDT
Darin Adler
Comment 2 2015-06-09 09:42:08 PDT
Comment on attachment 254566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254566&action=review > Source/WebCore/ChangeLog:12 > + Removed DeferredWrapper constructor that creates internally its promise. > + Reason is DeferredWrapper should be moved and we would loose access to its promise. I don’t understand this. I don’t see DeferredWrapper being moved! It seems to be captured by lambda, which makes copies and doesn’t move. Is this a statement of future intent? > Source/WebCore/bindings/js/JSDOMPromise.cpp:43 > + // FIXME: Remove this accessor once ReadableStreamReader custom binding do not need it. Grammar mistake: should be "does not need it". > Source/WebCore/bindings/js/JSDOMPromise.h:62 > + ASSERT(m_deferred && m_globalObject); A rule of thumb in WebKit is that we never use && in the top level expression in an ASSERT, because we’d like to know which clause failed. So this should be written in two lines: ASSERT(m_deferred); ASSERT(m_globalObject);
youenn fablet
Comment 3 2015-06-09 10:07:56 PDT
Comment on attachment 254566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254566&action=review >> Source/WebCore/ChangeLog:12 >> + Reason is DeferredWrapper should be moved and we would loose access to its promise. > > I don’t understand this. I don’t see DeferredWrapper being moved! It seems to be captured by lambda, which makes copies and doesn’t move. Is this a statement of future intent? The log states 'DeferredWrapper should be moved', not that it is actually moved. This migration is done in bug 145223 patch for AudioPromise, Once all promise binding code is moved to direct use of DOMPromise/DeferredWrapper, promise() accessor and copy constructors of DeferredWrapper could be also removed. I will clarify this in a new version of the patch. >> Source/WebCore/bindings/js/JSDOMPromise.cpp:43 >> + // FIXME: Remove this accessor once ReadableStreamReader custom binding do not need it. > > Grammar mistake: should be "does not need it". I thought I fixed it... Will do in next patch. >> Source/WebCore/bindings/js/JSDOMPromise.h:62 >> + ASSERT(m_deferred && m_globalObject); > > A rule of thumb in WebKit is that we never use && in the top level expression in an ASSERT, because we’d like to know which clause failed. So this should be written in two lines: > > ASSERT(m_deferred); > ASSERT(m_globalObject); OK
youenn fablet
Comment 4 2015-06-09 12:33:17 PDT
Created attachment 254589 [details] Fixed according review
Darin Adler
Comment 5 2015-06-09 12:34:23 PDT
Comment on attachment 254589 [details] Fixed according review View in context: https://bugs.webkit.org/attachment.cgi?id=254589&action=review > Source/WebCore/bindings/js/JSDOMPromise.h:84 > + ASSERT(m_deferred && m_globalObject); Oops, left this one in by accident.
youenn fablet
Comment 6 2015-06-09 12:57:55 PDT
Created attachment 254594 [details] Removed redundant assert
Build Bot
Comment 7 2015-06-09 15:16:31 PDT
Comment on attachment 254594 [details] Removed redundant assert Attachment 254594 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5766373422661632 New failing tests: platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-vertical.html
Build Bot
Comment 8 2015-06-09 15:16:35 PDT
Created attachment 254607 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
youenn fablet
Comment 9 2015-06-09 22:37:25 PDT
Comment on attachment 254594 [details] Removed redundant assert Let's try cq+. The failing test might be unrelated.
WebKit Commit Bot
Comment 10 2015-06-09 23:27:30 PDT
Comment on attachment 254594 [details] Removed redundant assert Clearing flags on attachment: 254594 Committed r185404: <http://trac.webkit.org/changeset/185404>
WebKit Commit Bot
Comment 11 2015-06-09 23:27:36 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.