Ensure that all the smart pointer types in WTF clear their pointer before deref
Created attachment 250828 [details] Patch
Created attachment 250829 [details] Patch
Comment on attachment 250829 [details] Patch Attachment 250829 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4594815194890240 Number of test failures exceeded the failure limit.
Created attachment 250838 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 250829 [details] Patch Attachment 250829 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4723881109618688 Number of test failures exceeded the failure limit.
Created attachment 250845 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 250829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250829&action=review Good idea. I almost said review+ but there are a few questions worth answering first. Since deref is a hot function, I presume this could have performance impact. What did we do to measure it? Looks like some tests are crashing. Should look into that. > Source/WTF/wtf/PassRefPtr.h:62 > + ALWAYS_INLINE ~PassRefPtr() { > + T* ptr = m_ptr; > + m_ptr = nullptr; > + derefIfNotNull(ptr); > + } If it’s going to become multi-line like this I prefer that this function’s body be moved outside of the class definition. > Source/WTF/wtf/Ref.h:77 > + T* ptr = std::exchange(m_ptr, nullptr); > m_ptr = &object; Why first set to nullptr, then to &object? Seems that we either should simply set it to &object in the first place, or leave it as nullptr during the deref and then set to &object only after the deref. > Source/WTF/wtf/RefPtr.h:62 > - ALWAYS_INLINE ~RefPtr() { derefIfNotNull(m_ptr); } > + ALWAYS_INLINE ~RefPtr() > + { > + derefIfNotNull(std::exchange(m_ptr, nullptr)); > + } If it’s going to become multi-line like this I prefer that this function’s body be moved outside of the class definition. > Source/WTF/wtf/RetainPtr.h:78 > + ~RetainPtr() > + { > + if (StorageType ptr = std::exchange(m_ptr, nullptr)) > + CFRelease(ptr); > + } If it’s going to become multi-line like this I prefer that this function’s body be moved outside of the class definition.
Another r- because it is somehow causing millions of tests to fail ;) I hadn't run tests yet when i pushed it up as i assumed it would Just Work (hahahahaha), and wanted to make sure everything built on other platforms. --Oliver (In reply to comment #7) > Comment on attachment 250829 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250829&action=review > > Good idea. I almost said review+ but there are a few questions worth > answering first. > > Since deref is a hot function, I presume this could have performance impact. > What did we do to measure it? > > Looks like some tests are crashing. Should look into that. > > > Source/WTF/wtf/PassRefPtr.h:62 > > + ALWAYS_INLINE ~PassRefPtr() { > > + T* ptr = m_ptr; > > + m_ptr = nullptr; > > + derefIfNotNull(ptr); > > + } > > If it’s going to become multi-line like this I prefer that this function’s > body be moved outside of the class definition. > > > Source/WTF/wtf/Ref.h:77 > > + T* ptr = std::exchange(m_ptr, nullptr); > > m_ptr = &object; > > Why first set to nullptr, then to &object? Seems that we either should > simply set it to &object in the first place, or leave it as nullptr during > the deref and then set to &object only after the deref. > > > Source/WTF/wtf/RefPtr.h:62 > > - ALWAYS_INLINE ~RefPtr() { derefIfNotNull(m_ptr); } > > + ALWAYS_INLINE ~RefPtr() > > + { > > + derefIfNotNull(std::exchange(m_ptr, nullptr)); > > + } > > If it’s going to become multi-line like this I prefer that this function’s > body be moved outside of the class definition. > > > Source/WTF/wtf/RetainPtr.h:78 > > + ~RetainPtr() > > + { > > + if (StorageType ptr = std::exchange(m_ptr, nullptr)) > > + CFRelease(ptr); > > + } > > If it’s going to become multi-line like this I prefer that this function’s > body be moved outside of the class definition.
Created attachment 252101 [details] Patch
Comment on attachment 252101 [details] Patch Attachment 252101 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6709304367251456 Number of test failures exceeded the failure limit.
Created attachment 252116 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 252101 [details] Patch Attachment 252101 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6428580472946688 Number of test failures exceeded the failure limit.
Created attachment 252117 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 252180 [details] Patch
Comment on attachment 252180 [details] Patch Attachment 252180 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5818393932333056 Number of test failures exceeded the failure limit.
Created attachment 252194 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 252180 [details] Patch Can you do one pointer type at a time? Doing them all at once will make it harder for us to regress later if any of these changes causes a bug.
Comment on attachment 252180 [details] Patch There are 25 or 30 crashes caused by this in run-webkit-tests test cases. Should land these one at a time and investigate and solve problems caused by code that depends on the old behavior.
Created attachment 252876 [details] just a test to run against other bots
Comment on attachment 252876 [details] just a test to run against other bots Hopefully no perf regressions...
Comment on attachment 252876 [details] just a test to run against other bots Clearing flags on attachment: 252876 Committed r184316: <http://trac.webkit.org/changeset/184316>
All reviewed patches have been landed. Closing bug.