Bug 139491

Summary: [GTK] Add initial database process support
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, beidson, commit-queue, gustavo, mcatanzaro, ossy, pnormand, svillar
Priority: P2 Keywords: Gtk, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 139724    
Bug Blocks: 98932    
Attachments:
Description Flags
Patch
none
Updated patch
none
Updated patch
none
Test case for the compiler issue
none
Rebased patch
none
Updated patch svillar: review+

Description Carlos Garcia Campos 2014-12-10 09:35:06 PST
To bring back IndexedDB support to the GTK port.
Comment 1 Carlos Garcia Campos 2014-12-10 10:01:55 PST
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).
Comment 2 WebKit Commit Bot 2014-12-10 10:03:26 PST
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.
Comment 3 Carlos Garcia Campos 2014-12-10 10:15:12 PST
Created attachment 243042 [details]
Updated patch

I forgot to git add a file.
Comment 4 WebKit Commit Bot 2014-12-10 10:23:56 PST
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 5 Martin Robinson 2014-12-10 10:43:10 PST
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.
Comment 6 Carlos Garcia Campos 2014-12-10 10:48:52 PST
(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
Comment 7 Carlos Garcia Campos 2014-12-11 01:25:15 PST
Maybe sergio can look at the gvariant code.
Comment 8 Carlos Garcia Campos 2014-12-11 02:01:57 PST
Created attachment 243110 [details]
Updated patch
Comment 9 Csaba Osztrogonác 2014-12-11 04:35:18 PST
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.
Comment 10 Carlos Garcia Campos 2014-12-11 04:48:57 PST
(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.
Comment 11 Carlos Garcia Campos 2014-12-15 06:13:08 PST
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.
Comment 12 Michael Catanzaro 2015-01-04 16:03:55 PST
(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....
Comment 13 Carlos Garcia Campos 2015-01-04 23:17:18 PST
Zan already fixed the AsyncRequest issues in bug #139724, that is blocking this one.
Comment 14 Carlos Garcia Campos 2015-01-22 01:53:49 PST
Created attachment 245132 [details]
Rebased patch
Comment 15 WebKit Commit Bot 2015-01-22 01:56:08 PST
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.
Comment 16 Carlos Garcia Campos 2015-01-22 06:50:31 PST
I have no idea what the GTK+ EWS build failure is about, but looks unrelated to this patch.
Comment 17 Sergio Villar Senin 2015-01-22 07:09:17 PST
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 18 Zan Dobersek 2015-01-22 10:30:41 PST
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.
Comment 19 Carlos Garcia Campos 2015-01-23 02:44:16 PST
(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.
Comment 20 Carlos Garcia Campos 2015-01-23 02:47:09 PST
(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?
Comment 21 Carlos Garcia Campos 2015-01-23 02:53:20 PST
Created attachment 245219 [details]
Updated patch

Addressed review comments
Comment 22 WebKit Commit Bot 2015-01-23 02:56:14 PST
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 23 Sergio Villar Senin 2015-01-23 06:13:39 PST
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?
Comment 24 Carlos Garcia Campos 2015-01-23 06:22:38 PST
(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.
Comment 25 Carlos Garcia Campos 2015-01-23 06:30:27 PST
Committed r179007: <http://trac.webkit.org/changeset/179007>