Bug 143789 – Ensure that all the smart pointer types in WTF clear their pointer before deref

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Ensure that all the smart pointer types in WTF clear their pointer before deref

[Oliver Hunt]

Created attachment 250828 [details]
Patch

[Oliver Hunt]

Created attachment 250829 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

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.

[Oliver Hunt]

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.

[Oliver Hunt]

Created attachment 252101 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Oliver Hunt]

Created attachment 252180 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Oliver Hunt]

Created attachment 252876 [details]
just a test to run against other bots

[Ryosuke Niwa]

Comment on attachment 252876 [details]
just a test to run against other bots

Hopefully no perf regressions...

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.