Bug 139491

Created attachment 243039 [details] Patch It's still disabled by default because I'm getting a lot of test failures, mainly due to what I think it's a bug in the compiler, but I'm not sure. When completing an async task, UniqueIDBDatabase is calling AsyncRequest::completeRequest() with an errorCode and an errorMessage. For some reason, when the errorCode is 0, something very different is received in the completion handler callback, I guess someting goes wrong when unpacking the Arguments, however all other parameters are correctly unpacked. This makes the WebProcess think that many of the methods failed when they didn't. I manged to workaround it by using WTF::move(errorCode), but I don't have an explanation. With that, and skipping the same layout tests than mac, I managed to get only 25 failures (with similar error, so I suspect is actually the same bug in most of them).

Attachment 243039 [details] did not pass style-queue: ERROR: Source/WebKit2/CMakeLists.txt:469: Alphabetical sorting problem. "Shared/Network/CustomProtocols/CustomProtocolManager.messages.in" should be before "Shared/Plugins/NPObjectMessageReceiver.messages.in". [list/order] [5] ERROR: Source/WebKit2/DatabaseProcess/gtk/DatabaseProcessMainGtk.cpp:32: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 243042 [details] Updated patch I forgot to git add a file.

Attachment 243042 [details] did not pass style-queue: ERROR: Source/WebKit2/CMakeLists.txt:469: Alphabetical sorting problem. "Shared/Network/CustomProtocols/CustomProtocolManager.messages.in" should be before "Shared/Plugins/NPObjectMessageReceiver.messages.in". [list/order] [5] ERROR: Source/WebKit2/DatabaseProcess/gtk/DatabaseProcessMainGtk.cpp:32: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 243042 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=243042&action=review Looks reasonable to me, though maybe someone with more GVariant experience should double-check the type strings. > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:2 > + * Copyright (C) 2014 Apple Inc. All rights reserved. I think we also want an Igalia copyright here too. > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:55 > + while (g_variant_iter_loop (&iter, "{&sv}", &key, &value)) Extra space here after g_variant_iter_loop. > Source/WebKit2/Shared/gtk/KeyedEncoder.cpp:2 > + * Copyright (C) 2013 Apple Inc. All rights reserved. I think we want an Igalia copyright here too.

(In reply to comment #5) > Comment on attachment 243042 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243042&action=review > > Looks reasonable to me, though maybe someone with more GVariant experience > should double-check the type strings. > > > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:2 > > + * Copyright (C) 2014 Apple Inc. All rights reserved. > > I think we also want an Igalia copyright here too. Right, I forgot to update that after copy-pasting :-P > > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:55 > > + while (g_variant_iter_loop (&iter, "{&sv}", &key, &value)) > > Extra space here after g_variant_iter_loop. Good catch! > > Source/WebKit2/Shared/gtk/KeyedEncoder.cpp:2 > > + * Copyright (C) 2013 Apple Inc. All rights reserved. > > I think we want an Igalia copyright here too. Right

Maybe sergio can look at the gvariant code.

Created attachment 243110 [details] Updated patch

Attachment 243110 [details] did not pass style-queue: ERROR: Source/WebKit2/CMakeLists.txt:470: Alphabetical sorting problem. "Shared/Network/CustomProtocols/CustomProtocolManager.messages.in" should be before "Shared/Plugins/NPObjectMessageReceiver.messages.in". [list/order] [5] ERROR: Source/WebKit2/DatabaseProcess/gtk/DatabaseProcessMainGtk.cpp:32: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #1) > Created attachment 243039 [details] > Patch > > It's still disabled by default because I'm getting a lot of test failures, > mainly due to what I think it's a bug in the compiler, but I'm not sure. > When completing an async task, UniqueIDBDatabase is calling > AsyncRequest::completeRequest() with an errorCode and an errorMessage. For > some reason, when the errorCode is 0, something very different is received > in the completion handler callback, I guess someting goes wrong when > unpacking the Arguments, however all other parameters are correctly > unpacked. This makes the WebProcess think that many of the methods failed > when they didn't. I manged to workaround it by using WTF::move(errorCode), > but I don't have an explanation. With that, and skipping the same layout > tests than mac, I managed to get only 25 failures (with similar error, so I > suspect is actually the same bug in most of them). It's definitely a gcc issue and also the other 25 tests failing, I built with clang and I'm getting only one test failure.

Created attachment 243291 [details] Test case for the compiler issue Ok, I've isolated the problem into a simpler test case. I'm not so sure this is a gcc issue, since the test case also fails here with clang, I don't understand why WebKit worked when built with clang, though. The problem seems to be that we are using the parent class, casting to the derived class, and using a lambda function (if the print method in the test case, calls a member function it works too). I'm not a C++11 expert, but the fact that when using the derived class directly, the build fails when calling print without using std::move for the arguments, makes me thing whether we should actually passing WTF::move for all the arguments passed to AsyncRequest::completeRequest(). I wonder if by using the parent class method and casting we are confusing the compiler that should fail like when using the derived class directly, but it doesn't.

(In reply to comment #11) > I'm not a C++11 expert, but the fact > that when using the derived class directly, the build fails when calling > print without using std::move for the arguments So you can only call the function with rvalues. I think you found a bug in AsyncRequestImpl<typename... Arguments>::completeRequest. Brady, can you look at this please? This function in the derived class in the attached test case is not right: template <typename... Arguments> class Printer : public PrinterBase { void print(Arguments&&... arguments) { m_printerFunction(std::forward<Arguments>(arguments)...); } } Argument&&... is not a pack of forwarding/universal references (which can bind to any type) as was intended, or you would be able to call it with an lvalue. Therefore it must be a pack of rvalue references, since if it was a pack of forwarding references, Carlos wouldn't have needed the std::move to call it. So the std::forward used inside the function is useless, as it can only ever be called on rvalue references. Now this here would be a pack of forwarding references: template <typename... Arguments> // <-- the difference is this line void print(Arguments&&... arguments) { m_printerFunction(std::forward<Arguments>(arguments)...); } In the function template version, type deduction is performed on the parameters that get passed. But when it's a normal function in a template class, there is no type deduction -- the types were already specified when the class template was instantiated -- so the only reason to write the && is to force the parameters to be rvalues, which is what (I guess, almost confidently) the syntax does. Now, at first I thought this all was unrelated to the data corruption problem, but now I think it's to blame. Here's why: AsyncRequestImpl<typename... Arguments>::completeRequest must "obviously" only be passed rvalue references and that's normally enforced at compile time as Carlos discovered, but if you call it via AsyncRequest::completeRequest, you are (possibly, and in this test case) perfect-forwarding lvalue references to it, so you wind up with lvalues being bound to rvalue references. This is the part I don't understand: how does that even compile? It seems like something that ought to be rejected at compile time, but since that's not happening, I guess it's a hole in the type system? (This is really hard to Google for.) P.S. I think I've reread that enough times and what I wrote makes sense, but look for mistakes in my reasoning please....

Zan already fixed the AsyncRequest issues in bug #139724, that is blocking this one.

Created attachment 245132 [details] Rebased patch

Attachment 245132 [details] did not pass style-queue: ERROR: Source/WebKit2/CMakeLists.txt:484: Alphabetical sorting problem. "Shared/Network/CustomProtocols/CustomProtocolManager.messages.in" should be before "Shared/Plugins/NPObjectMessageReceiver.messages.in". [list/order] [5] ERROR: Source/WebKit2/DatabaseProcess/gtk/DatabaseProcessMainGtk.cpp:32: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

I have no idea what the GTK+ EWS build failure is about, but looks unrelated to this patch.

Comment on attachment 245132 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=245132&action=review Great that the problems with the tests are solved. > Source/WebKit2/DatabaseProcess/EntryPoint/unix/DatabaseProcessMain.cpp:2 > + * Copyright (C) 2014 Igalia S.L. Nit: 2015 > Source/WebKit2/DatabaseProcess/gtk/DatabaseProcessMainGtk.cpp:2 > + * Copyright (C) 2014 Igalia S.L. Ditto. > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:2 > + * Copyright (C) 2014 Igalia S.L. Ditto. > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:30 > + Is this going/meant to be used for anything else apart from the DatabaseProcess? If not we should add the proper guards. > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:38 > + buildDictionaryFromGVariant(variant.get(), dictionary); As we do this trice, and knowing that in none of those cases we use the dictionary for anything else, I think we can create and return the dictionary from buildDictionaryFromGVariant (renamed to perhaps createDictionary... or convertGVariantToDictionary). That would allow us to remove those unneeded local variables. > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:47 > +} What about enclosing the destructor in a #if !ASSERT_DISABLED block? > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:79 > + This and the following 4 decodeXXX look almost the same, I think we can create a macro with this code, the unique difference is the g_variant_get_XXX call, which can be generated by the macro using a passed in argument. So the body of those methods would be simply something like DECODE(key, result, boolean) or DECODE(key, result, int32) ... > Source/WebKit2/Shared/gtk/KeyedDecoder.h:2 > + * Copyright (C) 2014 Igaia S.L. Nit: 2015 > Source/WebKit2/Shared/gtk/KeyedDecoder.h:35 > + Is this going/meant to be used for anything else apart from the DatabaseProcess? If not we should add the proper guards. > Source/WebKit2/Shared/gtk/KeyedEncoder.cpp:2 > + * Copyright (C) 2014 Igalia S.L. Ditto. > Source/WebKit2/Shared/gtk/KeyedEncoder.cpp:31 > + Is this going/meant to be used for anything else apart from the DatabaseProcess? If not we should add the proper guards. > Source/WebKit2/Shared/gtk/KeyedEncoder.cpp:48 > +} Same comment about the destructor full of ASSERTs. > Source/WebKit2/Shared/gtk/KeyedEncoder.h:2 > + * Copyright (C) 2014 Igalia S.L. Nit: 2015 > Source/WebKit2/Shared/gtk/KeyedEncoder.h:34 > + Is this going/meant to be used for anything else apart from the DatabaseProcess? If not we should add the proper guards.

Comment on attachment 245132 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=245132&action=review > Source/WTF/wtf/gobject/GRefPtr.cpp:102 > +template <> void derefGPtr(GVariantBuilder* ptr) > +{ > + g_variant_builder_unref(ptr); > +} Safe to call g_variant_builder_unref() without a null check on ptr, like in refGPtr() for GVariantBuilder? > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:196 > + RefPtr<DatabaseToWebProcessConnection> connection = DatabaseToWebProcessConnection::create(socketPair.server); > + m_databaseToWebProcessConnections.append(connection.release()); Can you inline the DatabaseToWebProcessConnection::create() call? > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:199 > + IPC::Attachment clientSocket(socketPair.client); > + parentProcessConnection()->send(Messages::DatabaseProcessProxy::DidCreateDatabaseToWebProcessConnection(clientSocket), 0); Can you inline the IPC::Attachment construction? > Source/WebKit2/DatabaseProcess/unix/DatabaseProcessMainUnix.h:43 > +#endif // DatabaseProcessMainUnix_h > + > +#endif // ENABLE(DATABASE_PROCESS) The comments for the build guards are reversed here. >> Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:79 >> + > > This and the following 4 decodeXXX look almost the same, I think we can create a macro with this code, the unique difference is the g_variant_get_XXX call, which can be generated by the macro using a passed in argument. So the body of those methods would be simply something like DECODE(key, result, boolean) or DECODE(key, result, int32) ... You get a golden r+ for using templates instead of macros. > Source/WebKit2/Shared/gtk/KeyedEncoder.h:52 > + virtual void encodeBytes(const String& key, const uint8_t*, size_t) override; > + virtual void encodeBool(const String& key, bool) override; > + virtual void encodeUInt32(const String& key, uint32_t) override; > + virtual void encodeInt32(const String& key, int32_t) override; > + virtual void encodeInt64(const String& key, int64_t) override; > + virtual void encodeFloat(const String& key, float) override; > + virtual void encodeDouble(const String& key, double) override; > + virtual void encodeString(const String& key, const String&) override; Same here, it might be possible to implement these through macros or templates.

(In reply to comment #17) > Comment on attachment 245132 [details] > Rebased patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245132&action=review > > Great that the problems with the tests are solved. Thanks for the review. > > Source/WebKit2/DatabaseProcess/EntryPoint/unix/DatabaseProcessMain.cpp:2 > > + * Copyright (C) 2014 Igalia S.L. > > Nit: 2015 Sure. > > > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:30 > > + > > Is this going/meant to be used for anything else apart from the > DatabaseProcess? If not we should add the proper guards. I don't know, but neither the WebCore base classes nor the mac implementation have DatabaseProcess guards. In any case these are GTK+ specific files and I plan to enable indexeddb by default in bug #98932, so it will be always built anyway. > > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:38 > > + buildDictionaryFromGVariant(variant.get(), dictionary); > > As we do this trice, and knowing that in none of those cases we use the > dictionary for anything else, I think we can create and return the > dictionary from buildDictionaryFromGVariant (renamed to perhaps > createDictionary... or convertGVariantToDictionary). That would allow us to > remove those unneeded local variables. Good idea. > > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:47 > > +} > > What about enclosing the destructor in a #if !ASSERT_DISABLED block? Isn't ASSERT noop in non debug builds? Or do you mean not defining the destructor at all? I think I need to define the destructor, since it's virtual in the parent class. > > Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:79 > > + > > This and the following 4 decodeXXX look almost the same, I think we can > create a macro with this code, the unique difference is the > g_variant_get_XXX call, which can be generated by the macro using a passed > in argument. So the body of those methods would be simply something like > DECODE(key, result, boolean) or DECODE(key, result, int32) ... I'll use a template as Zan suggested.

(In reply to comment #18) > Comment on attachment 245132 [details] > Rebased patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245132&action=review Thanks for the review. > > Source/WTF/wtf/gobject/GRefPtr.cpp:102 > > +template <> void derefGPtr(GVariantBuilder* ptr) > > +{ > > + g_variant_builder_unref(ptr); > > +} > > Safe to call g_variant_builder_unref() without a null check on ptr, like in > refGPtr() for GVariantBuilder? Good catch, it's indeed needed, and I made the mistake because I copy-pasted the GVariant implementation, which means it also needs to be fixed. I've noticed that GHashTable impl has the same issue so I'll fix both GVariant and GHashTable in a separate patch. > > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:196 > > + RefPtr<DatabaseToWebProcessConnection> connection = DatabaseToWebProcessConnection::create(socketPair.server); > > + m_databaseToWebProcessConnections.append(connection.release()); > > Can you inline the DatabaseToWebProcessConnection::create() call? Yes > > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:199 > > + IPC::Attachment clientSocket(socketPair.client); > > + parentProcessConnection()->send(Messages::DatabaseProcessProxy::DidCreateDatabaseToWebProcessConnection(clientSocket), 0); > > Can you inline the IPC::Attachment construction? Yes > > Source/WebKit2/DatabaseProcess/unix/DatabaseProcessMainUnix.h:43 > > +#endif // DatabaseProcessMainUnix_h > > + > > +#endif // ENABLE(DATABASE_PROCESS) > > The comments for the build guards are reversed here. Oops > >> Source/WebKit2/Shared/gtk/KeyedDecoder.cpp:79 > >> + > > > > This and the following 4 decodeXXX look almost the same, I think we can create a macro with this code, the unique difference is the g_variant_get_XXX call, which can be generated by the macro using a passed in argument. So the body of those methods would be simply something like DECODE(key, result, boolean) or DECODE(key, result, int32) ... > > You get a golden r+ for using templates instead of macros. Let's try > > Source/WebKit2/Shared/gtk/KeyedEncoder.h:52 > > + virtual void encodeBytes(const String& key, const uint8_t*, size_t) override; > > + virtual void encodeBool(const String& key, bool) override; > > + virtual void encodeUInt32(const String& key, uint32_t) override; > > + virtual void encodeInt32(const String& key, int32_t) override; > > + virtual void encodeInt64(const String& key, int64_t) override; > > + virtual void encodeFloat(const String& key, float) override; > > + virtual void encodeDouble(const String& key, double) override; > > + virtual void encodeString(const String& key, const String&) override; > > Same here, it might be possible to implement these through macros or > templates. In the case of the encoder, the implementation of those methods is already a single line, what would be the benefit of a template?

Created attachment 245219 [details] Updated patch Addressed review comments

Attachment 245219 [details] did not pass style-queue: ERROR: Source/WebKit2/CMakeLists.txt:484: Alphabetical sorting problem. "Shared/Network/CustomProtocols/CustomProtocolManager.messages.in" should be before "Shared/Plugins/NPObjectMessageReceiver.messages.in". [list/order] [5] ERROR: Source/WebKit2/DatabaseProcess/gtk/DatabaseProcessMainGtk.cpp:32: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 245219 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=245219&action=review Awesome job! > Source/WebCore/ChangeLog:10 > + WebCore::fileSystemRepresentation() for the databae filename, Typo: databae > Source/WebCore/ChangeLog:11 > + otherwise sqlite3_open() fails when the filename contains "%2E". Is there any test for this?

(In reply to comment #23) > Comment on attachment 245219 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245219&action=review > > Awesome job! Thanks > > Source/WebCore/ChangeLog:10 > > + WebCore::fileSystemRepresentation() for the databae filename, > > Typo: databae Oops > > Source/WebCore/ChangeLog:11 > > + otherwise sqlite3_open() fails when the filename contains "%2E". > > Is there any test for this? Yes, I noticed because some test failed, but I don't remember which ones.

Committed r179007: <http://trac.webkit.org/changeset/179007>