| Summary: | Add a release assertion that Functions can only be constructed from non-null CompletionHandlers | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, dbates, ews-watchlist, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Alex Christensen
2019-05-06 17:58:42 PDT
Created attachment 369213 [details]
Patch
Comment on attachment 369213 [details] Patch Attachment 369213 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12122984 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html Created attachment 369280 [details]
Archive of layout-test-results from ews210 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369213&action=review > Source/WTF/wtf/Function.h:33 > +namespace Detail { Why is this better than a private class in the Function scope? I preferred the old way as it was hiding the implementation detail. Now it's public and anyone could use it directly in theory. Private classes inside template classes seem to be unspecializable. I only get these: error: cannot specialize a member of an unspecialized template This is what Detail namespaces are for. Comment on attachment 369213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369213&action=review > Source/WTF/wtf/CompletionHandler.h:68 > +template<typename Out, typename... In> Should we add a FIXME to indicate this is temporary? I don't this this would stay a RELEASE_ASSERT() forever and it seems like a lot of extra code for a debug ASSERT(). I'm not convinced this is temporary. We should always be unable to make a non-null Function that when called will always dereference null. Also, we could use this same technique to make a CallableWrapperStorage class that is usually a unique_ptr but is just a CompletionHandler for the Function-wrapping-CompletionHandler case. We should also make Function more const correct by making operator() const if the lambda is not mutable and non-const for mutable lambdas. There's work to be done here. |