Bug 180762 – Enhance Ref and RefPtr to be able to work with smart pointers.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This is so we can use RefPtr with ConstExprPoisoned pointers.

[Radar WebKit Bug Importer]

<rdar://problem/36027122>

[Mark Lam]

Created attachment 329240 [details]
proposed patch.

[JF Bastien]

Comment on attachment 329240 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=329240&action=review

> Source/WTF/wtf/PoisonedUniquePtr.h:34
> +public:

?

[EWS Watchlist]

Comment on attachment 329240 [details]
proposed patch.

Attachment 329240 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5648342

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329248 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Mark Lam]

Created attachment 329251 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 329251 [details]
proposed patch.

Attachment 329251 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5649367

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329261 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Mark Lam]

(In reply to Build Bot from comment #8)
> Created attachment 329261 [details]
> Archive of layout-test-results from ews125 for ios-simulator-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> ios-sim-ews.
> Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

This is very strange.  I am not able to replicate these failures at all on my local runs.

[Mark Lam]

Created attachment 329325 [details]
proposed patch.

I've rebased the patch.  Let's do another EWS run to see if the issue earlier was just with the ios-sim bot, or if the test crashes on the bot are still manifesting.

[EWS Watchlist]

Comment on attachment 329325 [details]
proposed patch.

Attachment 329325 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5655478

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329329 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Mark Lam]

Comment on attachment 329325 [details]
proposed patch.

Looks like the ios-sim EWS failure is not caused by this patch.  Let's get a review.

[Alexey Proskuryakov]

> Looks like the ios-sim EWS failure is not caused by this patch.  Let's get a review.

I'm not sure if that's the case. Tests always pass without the change, but always fail with it, so the change can't be landed without destroying testing.

[JF Bastien]

Comment on attachment 329325 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=329325&action=review

Added Darin and Chris, just because it's hard to see mistakes in code like this sometimes, despite having shoulder-coded with you on this code for a bit, so even deferred review would be useful :-)
As I like to say, C++ templates are their own reward! Not sure that's a good thing, but I like how the patch turned out.

Should we also update http://webkit.org/coding/RefPtr.html ? It would be useful to document when to poison such pointers, and which keys should / shouldn't be shared.

Would be good to figure out the crashes.

Otherwise r=me

> Source/WTF/wtf/DumbPtrTraits.h:43
> +using WTF::DumbPtrTraits;

I don't think you need this, since it's only used in WTF? The other traits aren't injected in the global namespace.

> Source/WTF/wtf/Forward.h:24
> +#include <wtf/RefForward.h>

I'm not sure I understand why this is a useful addition. We can forward templates.

> Source/WTF/wtf/RefForward.h:28
> +#include <wtf/DumbPtrTraits.h>

I think you can forward-declare the traits as well, and use them below in the forward declaration? Not that the header is big right now, but I'm wary of include-creep in Forward.h.

[Darin Adler]

Comment on attachment 329325 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=329325&action=review

Looks like good work to me.

The most important part of this patch is the tests. They look pretty good! I don’t see a check that makes sure the pointer values are actually poisoned. We add at least one test of that, with a reintepret_cast or bitwise_cast or whatever.

I’d really like to keep the added header to a minimum here. I think templates give us a good opportunity here, because they can usually be compiled without the dependencies all resolved, unless the template is actually used and instantiated. So I think we can do this without adding so many headers.

> Source/WTF/wtf/DumbPtrTraits.h:36
> +    template<class U>
> +    static ALWAYS_INLINE T* exchange(StorageType& ptr, U&& newValue) { return std::exchange(ptr, newValue); }

I know everyone else formats it like this, but I find template functions like this hard to read when they are split like this. The second line looks like a standalone function to me and I don’t automatically understand that it combines with the above like. That’s why in my new code I have been putting template on the same line as the thing it’s a template for.

I also prefer typename to class for things like "U" here.

> Source/WTF/wtf/PoisonedRef.h:34
> +template<uint32_t key, typename T>
> +using PoisonedRef = Ref<T, ConstExprPoisonedPtrTraits<key, T>>;

It‘s sad to have an entire header just for this one line. I’d love to avoid adding a header just for this. Can this just go into Ref.h without adding any includes, using forward declarations?

These two lines seem like they should be one longer line, in the coding style I prefer.

> Source/WTF/wtf/PoisonedRefPtr.h:34
> +template<uint32_t key, typename T>
> +using PoisonedRefPtr = RefPtr<T, ConstExprPoisonedPtrTraits<key, T>>;

Ditto.

>> Source/WTF/wtf/RefForward.h:28
>> +#include <wtf/DumbPtrTraits.h>
> 
> I think you can forward-declare the traits as well, and use them below in the forward declaration? Not that the header is big right now, but I'm wary of include-creep in Forward.h.

And I think this should be merged back in with Forward.h.

[Mark Lam]

Comment on attachment 329325 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=329325&action=review

Thanks for the reviews.  I'll add some tests for confirming that expected poisoned values are indeed poisoned.

Regarding  the ios-sim EWS test failure, I'm still investigating.  The issue appears to be triggered just because I have a second template parameter in RefPtr even if I don't use the parameter.  I'm still investigating.

>> Source/WTF/wtf/DumbPtrTraits.h:36
>> +    static ALWAYS_INLINE T* exchange(StorageType& ptr, U&& newValue) { return std::exchange(ptr, newValue); }
> 
> I know everyone else formats it like this, but I find template functions like this hard to read when they are split like this. The second line looks like a standalone function to me and I don’t automatically understand that it combines with the above like. That’s why in my new code I have been putting template on the same line as the thing it’s a template for.
> 
> I also prefer typename to class for things like "U" here.

OK, will change.

>> Source/WTF/wtf/DumbPtrTraits.h:43
>> +using WTF::DumbPtrTraits;
> 
> I don't think you need this, since it's only used in WTF? The other traits aren't injected in the global namespace.

Will remove.

>> Source/WTF/wtf/Forward.h:24
>> +#include <wtf/RefForward.h>
> 
> I'm not sure I understand why this is a useful addition. We can forward templates.

Did you mean why I didn't forward declare DumbPtrTraits here instead of #include'ing its definition?  The reason is that I'd have to sprinkle #include <wtf/DumbPtrTraits.h> in every file that #include <wtf/Forward.h> for forward declaring a RefPtr<something>.  It was not sufficient to forward declare DumbPtrTraits.  I'll try again before landing just to confirm that I wasn't just tired and making the wrong assessment of the compilation error message the last time I tried this.

>> Source/WTF/wtf/PoisonedRef.h:34
>> +using PoisonedRef = Ref<T, ConstExprPoisonedPtrTraits<key, T>>;
> 
> It‘s sad to have an entire header just for this one line. I’d love to avoid adding a header just for this. Can this just go into Ref.h without adding any includes, using forward declarations?
> 
> These two lines seem like they should be one longer line, in the coding style I prefer.

I can give it (forward declaring ConstExprPoisonedPtrTraits) a try.  If it works out, I'll fold this into Ref.h.  If not, I'll keep this file separate.

>>> Source/WTF/wtf/RefForward.h:28
>>> +#include <wtf/DumbPtrTraits.h>
>> 
>> I think you can forward-declare the traits as well, and use them below in the forward declaration? Not that the header is big right now, but I'm wary of include-creep in Forward.h.
> 
> And I think this should be merged back in with Forward.h.

Forward declaring DumbPtrTraits results in needing to #include DumbPtrTraits.h almost everywhere that Forward.h is #included.  I'll re-test.

I can merge this file back into Forward.h.  But that would mean that I'd have to #include Forward.h in Ref.h.  Otherwise, the compiler will complain of duplicate definitions of the default traits argument for Ref and RefPtr in files that both #include Ref/RefPtr.h and Forward.h (which is every .h file that #include Forward.h).  Anyway, I'll go with merging these files.

[JF Bastien]

> >>> Source/WTF/wtf/RefForward.h:28
> >>> +#include <wtf/DumbPtrTraits.h>
> >> 
> >> I think you can forward-declare the traits as well, and use them below in the forward declaration? Not that the header is big right now, but I'm wary of include-creep in Forward.h.
> > 
> > And I think this should be merged back in with Forward.h.
> 
> Forward declaring DumbPtrTraits results in needing to #include
> DumbPtrTraits.h almost everywhere that Forward.h is #included.  I'll re-test.
> 
> I can merge this file back into Forward.h.  But that would mean that I'd
> have to #include Forward.h in Ref.h.  Otherwise, the compiler will complain
> of duplicate definitions of the default traits argument for Ref and RefPtr
> in files that both #include Ref/RefPtr.h and Forward.h (which is every .h
> file that #include Forward.h).  Anyway, I'll go with merging these files.

The right way to resolve this is to only declare default template parameters in Forward.h, and nowhere else. Otherwise any file that includes both will get that error.

[Keith Miller]

Comment on attachment 329325 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=329325&action=review

>>> Source/WTF/wtf/DumbPtrTraits.h:36
>>> +    static ALWAYS_INLINE T* exchange(StorageType& ptr, U&& newValue) { return std::exchange(ptr, newValue); }
>> 
>> I know everyone else formats it like this, but I find template functions like this hard to read when they are split like this. The second line looks like a standalone function to me and I don’t automatically understand that it combines with the above like. That’s why in my new code I have been putting template on the same line as the thing it’s a template for.
>> 
>> I also prefer typename to class for things like "U" here.
> 
> OK, will change.

I disagree, I find it hard to read functions that have the template on the same line as the rest of the signature. I prefer to have a whitespace between the template and the line above. In this case it would be:

using StorageType = T*;

template<class U>
static ALWAYS_INLINE T* exchange(StorageType& ptr, U&& newValue) { return std::exchange(ptr, newValue); }
...

[JF Bastien]

> I disagree, I find it hard to read functions that have the template on the
> same line as the rest of the signature. I prefer to have a whitespace
> between the template and the line above.

I ❤️ one-liner templates, as well as piling on to style 🚲sheds. 😁

[Keith Miller]

(In reply to Keith Miller from comment #19)
> Comment on attachment 329325 [details]
> proposed patch.
> 
> I disagree, I find it hard to read functions that have the template on the
> same line as the rest of the signature. I prefer to have a whitespace
> between the template and the line above. In this case it would be:
> 
> using StorageType = T*;
> 
> template<class U>
> static ALWAYS_INLINE T* exchange(StorageType& ptr, U&& newValue) { return
> std::exchange(ptr, newValue); }
> ...

To clarify, I find that the whitespace signals to me that something interesting is happening and I should look a little more closely at the signature.

[Mark Lam]

(In reply to Keith Miller from comment #21)
> (In reply to Keith Miller from comment #19)
> > Comment on attachment 329325 [details]
> > proposed patch.
> > 
> > I disagree, I find it hard to read functions that have the template on the
> > same line as the rest of the signature. I prefer to have a whitespace
> > between the template and the line above. In this case it would be:
> > 
> > using StorageType = T*;
> > 
> > template<class U>
> > static ALWAYS_INLINE T* exchange(StorageType& ptr, U&& newValue) { return
> > std::exchange(ptr, newValue); }
> > ...
> 
> To clarify, I find that the whitespace signals to me that something
> interesting is happening and I should look a little more closely at the
> signature.

I think there's a place for both (if our goal is readable code, and not just personal preferences).

For example, the single line form makes sense when there are a set of templated functions that go together (see the template<U> operator==, operator!=, etc. in Poisoned.h).  Having them in single lines makes it so much easier to contrast between each of those functions, and catch if there was a typo bug in the implementation.

On the other hand, the template on a separate line form works better when the number of template arguments are huge (making the "template<...> line very long).  For example, see ConstExprPoisonedPtrTraits::swap in Poisoned.h.  Having the template parameters on a separate line makes it easier to mentally filter out the template parameters, and allows us to see and focus on the declaration of the swap function itself.  And I agree that in these cases, it makes sense to have whitespace between each template declaration.

For DumbPtrTraits itself which only takes 1 template argument, I could go both ways.  I'll just go with 2 lines and add white space around it (to be consistent with how I'll format ConstExprPoisonedPtrTraits.

[Mark Lam]

Looks like the ios-sim EWS failure is due to a WebInspector ABI compatibility issue: system WebKit on the simulator was expecting to link against a function that takes as RefPtr as an argument.  Since the RefPtr now takes 2 template arguments, the C++ mangling for the function has also changed.  This resulted in a dlopen failure which caused most of the tests to fail to run.

I'll fix the WebInspector ABI compatibility issue in https://bugs.webkit.org/show_bug.cgi?id=180901, but I'll upload a composite patch here first to test if the fix works.

[Mark Lam]

Created attachment 329560 [details]
test for ews.

[EWS Watchlist]

Attachment 329560 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Ref.h:266:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WTF/wtf/RefPtr.h:286:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

If Ref and RefPtr are part of our ABI, you could keep a compatible wrapper in WTF namespace, use it explicitly in ABI context, and not have the global injection at the end of the header. Then what you actually global inject as “Ref” is the new version instead, so all non-ABI code get the new one.

[Mark Lam]

(In reply to JF Bastien from comment #26)
> If Ref and RefPtr are part of our ABI, you could keep a compatible wrapper
> in WTF namespace, use it explicitly in ABI context, and not have the global
> injection at the end of the header. Then what you actually global inject as
> “Ref” is the new version instead, so all non-ABI code get the new one.

I shouldn't have used the term ABI.  It's just some deprecated code that we need to keep around so that old Safari builds can work with ToT WebKit build.  We only need it for linkage.  It is never actually called.  I already have the fix implemented and will post it in https://bugs.webkit.org/show_bug.cgi?id=180901 later.

[Mark Lam]

Created attachment 329620 [details]
patch for landing + adding a RefPtr.cpp to hold some static_asserts.

[Mark Lam]

(In reply to Mark Lam from comment #17)
> >>> Source/WTF/wtf/RefForward.h:28
> >>> +#include <wtf/DumbPtrTraits.h>
> >> 
> >> I think you can forward-declare the traits as well, and use them below in the forward declaration? Not that the header is big right now, but I'm wary of include-creep in Forward.h.
> > 
> > And I think this should be merged back in with Forward.h.
> 
> Forward declaring DumbPtrTraits results in needing to #include
> DumbPtrTraits.h almost everywhere that Forward.h is #included.  I'll re-test.

I was wrong.  Forward declaring DumbPtrTraits worked out.  I'll be landing with the forward declaration in Forward.h.

[Mark Lam]

Landed in r226015: <http://trac.webkit.org/r226015>.