Bug 180762 - Enhance Ref and RefPtr to be able to work with smart pointers.
Summary: Enhance Ref and RefPtr to be able to work with smart pointers.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 180901
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-13 11:38 PST by Mark Lam
Modified: 2017-12-17 15:23 PST (History)
12 users (show)

See Also:


Attachments
proposed patch. (67.45 KB, patch)
2017-12-13 11:48 PST, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.31 MB, application/zip)
2017-12-13 13:12 PST, EWS Watchlist
no flags Details
proposed patch. (65.63 KB, patch)
2017-12-13 13:20 PST, Mark Lam
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.31 MB, application/zip)
2017-12-13 14:42 PST, EWS Watchlist
no flags Details
proposed patch. (65.60 KB, patch)
2017-12-13 21:42 PST, Mark Lam
jfbastien: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.33 MB, application/zip)
2017-12-13 23:04 PST, EWS Watchlist
no flags Details
test for ews. (111.58 KB, patch)
2017-12-15 22:05 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing + adding a RefPtr.cpp to hold some static_asserts. (65.60 KB, patch)
2017-12-17 13:12 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-12-13 11:38:01 PST
This is so we can use RefPtr with ConstExprPoisoned pointers.
Comment 1 Radar WebKit Bug Importer 2017-12-13 11:38:47 PST
<rdar://problem/36027122>
Comment 2 Mark Lam 2017-12-13 11:48:49 PST
Created attachment 329240 [details]
proposed patch.
Comment 3 JF Bastien 2017-12-13 12:59:14 PST
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:

?
Comment 4 EWS Watchlist 2017-12-13 13:12:11 PST
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.
Comment 5 EWS Watchlist 2017-12-13 13:12:13 PST
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
Comment 6 Mark Lam 2017-12-13 13:20:09 PST
Created attachment 329251 [details]
proposed patch.
Comment 7 EWS Watchlist 2017-12-13 14:42:28 PST
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.
Comment 8 EWS Watchlist 2017-12-13 14:42:29 PST
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
Comment 9 Mark Lam 2017-12-13 21:09:43 PST
(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.
Comment 10 Mark Lam 2017-12-13 21:42:00 PST
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.
Comment 11 EWS Watchlist 2017-12-13 23:04:03 PST
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.
Comment 12 EWS Watchlist 2017-12-13 23:04:04 PST
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
Comment 13 Mark Lam 2017-12-14 17:07:32 PST
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.
Comment 14 Alexey Proskuryakov 2017-12-14 17:35:00 PST
> 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.
Comment 15 JF Bastien 2017-12-15 09:10:47 PST
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.
Comment 16 Darin Adler 2017-12-15 09:36:37 PST
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.
Comment 17 Mark Lam 2017-12-15 10:08:36 PST
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.
Comment 18 JF Bastien 2017-12-15 10:14:46 PST
> >>> 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.
Comment 19 Keith Miller 2017-12-15 10:17:26 PST
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); }
...
Comment 20 JF Bastien 2017-12-15 10:19:15 PST
> 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. 😁
Comment 21 Keith Miller 2017-12-15 10:20:09 PST
(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.
Comment 22 Mark Lam 2017-12-15 10:32:46 PST
(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.
Comment 23 Mark Lam 2017-12-15 21:30:49 PST
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.
Comment 24 Mark Lam 2017-12-15 22:05:46 PST
Created attachment 329560 [details]
test for ews.
Comment 25 EWS Watchlist 2017-12-15 22:08:35 PST
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.
Comment 26 JF Bastien 2017-12-15 22:15:35 PST
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.
Comment 27 Mark Lam 2017-12-15 22:41:53 PST
(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.
Comment 28 Mark Lam 2017-12-17 13:12:24 PST
Created attachment 329620 [details]
patch for landing + adding a RefPtr.cpp to hold some static_asserts.
Comment 29 Mark Lam 2017-12-17 15:13:32 PST
(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.
Comment 30 Mark Lam 2017-12-17 15:23:04 PST
Landed in r226015: <http://trac.webkit.org/r226015>.