Bug 139491 – [GTK] Add initial database process support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

To bring back IndexedDB support to the GTK port.

[Carlos Garcia Campos]

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).

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

Created attachment 243042 [details]
Updated patch

I forgot to git add a file.

[WebKit Commit Bot]

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.

[Martin Robinson]

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.

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

Maybe sergio can look at the gvariant code.

[Carlos Garcia Campos]

Created attachment 243110 [details]
Updated patch

[Csaba Osztrogonác]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

(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....

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 245132 [details]
Rebased patch

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

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

[Sergio Villar Senin]

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.

[Zan Dobersek]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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?

[Carlos Garcia Campos]

Created attachment 245219 [details]
Updated patch

Addressed review comments

[WebKit Commit Bot]

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.

[Sergio Villar Senin]

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?

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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