Bug 170270

Created attachment 305837 [details] Patch

Depends on the Utilities.h header that's being added in bug #170269.

Created attachment 305947 [details] Patch

*** Bug 169528 has been marked as a duplicate of this bug. ***

Comment on attachment 305947 [details] Patch Thanks for adding WebCrypto API support for GTK+. Looks good:)

Comment on attachment 305947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305947&action=review > Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:37 > +#include <pal/crypto/gcrypt/Utilities.h> Can you not #include <pal/Utilities.h>? Do we not have any forwarding headers for PAL yet? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:76 > + context.ref(); It'd be cleaner to use Ref<ScriptExecutionContext> for this, so you don't have to remember to deref it in two difference places below, but now I see that ScriptExecutionContext does not inherit from RefCounted even though it has ref and deref methods. Huh. So not great, but I guess this is best.... > Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:85 > + [callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) { What, why are you capturing callback here, when you don't use it? There's no particular need to move the value. Just ignore it. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:93 > + [output = WTFMove(*output), callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) { And here there is no need to capture exceptionCallback.

Comment on attachment 305947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305947&action=review Actually, I'd like to look at this one again. > Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:64 > + Vector<uint8_t> result(length / 8); Can you explain why the length of the result is only 1/8th the size of the length of the input? Seems a bit arbitrary? > Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:71 > + return WTFMove(result); Hm, no this is not right, you can't use WTFMove when returning a local variable... it's stack-allocated and has been destroyed, right...? Isn't this undefined behavior or something otherwise bad? Evens if I'm wrong about that, you're guaranteed move semantics if you just return result directly anyway, so you should do that instead, no need for WTFMove here.

(In reply to Michael Catanzaro from comment #7) > Can you explain why the length of the result is only 1/8th the size of the > length of the input? Seems a bit arbitrary? I guess in a function named "deriveBits" it's understandable that the length parameter is assumed to refer to bits, then you store it in a vector holding bytes... so OK, that checks out.

(In reply to Michael Catanzaro from comment #7) > Hm, no this is not right, you can't use WTFMove when returning a local > variable... it's stack-allocated and has been destroyed, right...? Isn't > this undefined behavior or something otherwise bad? Evens if I'm wrong about > that, you're guaranteed move semantics if you just return result directly > anyway, so you should do that instead, no need for WTFMove here. This stupid complicated Stack Overflow answer says (a) I'm wrong and (b) you still shouldn't do it: http://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value So don't do it!

Comment on attachment 305947 [details] Patch (Changed my mind again; these are minor comments. ;)

Comment on attachment 305947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305947&action=review >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:37 >> +#include <pal/crypto/gcrypt/Utilities.h> > > Can you not #include <pal/Utilities.h>? Do we not have any forwarding headers for PAL yet? No, there's no forwarding headers for PAL. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:64 >> + Vector<uint8_t> result(length / 8); > > Can you explain why the length of the result is only 1/8th the size of the length of the input? Seems a bit arbitrary? The length is the number of bits to be derived, but the API works with bytes. CryptoAlgorithmPBKDF2::deriveBits() ensures that the length is a multiple of 8. An assert with a comment can be put here though. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:71 >> + return WTFMove(result); > > Hm, no this is not right, you can't use WTFMove when returning a local variable... it's stack-allocated and has been destroyed, right...? Isn't this undefined behavior or something otherwise bad? Evens if I'm wrong about that, you're guaranteed move semantics if you just return result directly anyway, so you should do that instead, no need for WTFMove here. This constructs the std::optional<Vector<uint8_t>> by moving the non-nullopt value into the std::optional<> constructor, which will then just move the resources of the stack-allocated Vector into the Vector that's wrapped in std::optional. Move semantics would be guaranteed if the return type of this function was Vector<uint8_t>, but since it's std::optional<>, we have to construct that object upon return. >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:85 >> + [callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback)](ScriptExecutionContext& context) { > > What, why are you capturing callback here, when you don't use it? There's no particular need to move the value. Just ignore it. This follows the Mac implementation. Per comments there, the callbacks should be dereferenced only when finally back on the Document or Worker thread.I'll add the same comment here, and in the other patches.

(In reply to Zan Dobersek from comment #11) > Comment on attachment 305947 [details] > >> Source/WebCore/crypto/gcrypt/CryptoAlgorithmPBKDF2GCrypt.cpp:71 > >> + return WTFMove(result); > > > > Hm, no this is not right, you can't use WTFMove when returning a local variable... it's stack-allocated and has been destroyed, right...? Isn't this undefined behavior or something otherwise bad? Evens if I'm wrong about that, you're guaranteed move semantics if you just return result directly anyway, so you should do that instead, no need for WTFMove here. > > This constructs the std::optional<Vector<uint8_t>> by moving the non-nullopt > value into the std::optional<> constructor, which will then just move the > resources of the stack-allocated Vector into the Vector that's wrapped in > std::optional. > > Move semantics would be guaranteed if the return type of this function was > Vector<uint8_t>, but since it's std::optional<>, we have to construct that > object upon return. > Testing this on a function that returns std::optional<std::unique_ptr<T>>, a simple return appears to work, constructing the std::optional<> object with an rvalue reference of the returned std::unique_ptr<> object without using move semantics. I'll check at some point later whether the result Vector<> object is still copied when constructing the std::optional<> value, but even copies are probably not that expensive in these cases as the vectors are small. So WTFMove() can be dropped.

Created attachment 306064 [details] Patch for landing

(In reply to Zan Dobersek from comment #11) > This follows the Mac implementation. Per comments there, the callbacks > should be dereferenced only when finally back on the Document or Worker > thread.I'll add the same comment here, and in the other patches. Wow, that's a serious footgun. Definitely requires a comment everywhere.

Comment on attachment 306064 [details] Patch for landing Clearing flags on attachment: 306064 Committed r214821: <http://trac.webkit.org/changeset/214821>

All reviewed patches have been landed. Closing bug.