Bug 54109 – [Qt][WK2]Unbreak InjectedBundle on Qt

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Balazs Kelemen]

The KURL dependency introduced in http://trac.webkit.org/changeset/77794 brakes non-mac platforms:
symbol lookup error: /home/balazs/WebKitGit/WebKitBuild/Release/lib/libWTRInjectedBundle.so: undefined symbol: _ZN7WebCore4KURLC1ENS_18ParsedURLStringTagEPKc
#CRASHED - WebProcess

To fix this we should either export those symbols everywhere or do not use WebCore internally.
Since the second is already a general concept of WTR I believe that would be the right solution.

[Csaba Osztrogonác]

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

[Balazs Kelemen]

Created attachment 81822 [details]
Patch

[WebKit Review Bot]

Attachment 81822 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:627:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

I removed a comment accidentally, this should be fixed before commit.
The style issue is better to be ignored in my opinion, it is pretty
confusing to write |!urlString.find("http")| to say that "if http is
prefix of the string".

[Balazs Kelemen]

Comment on attachment 81822 [details]
Patch

The patch makes two tests failing.
Need to find a better approach.

[Balazs Kelemen]

Created attachment 82453 [details]
Patch

[WebKit Review Bot]

Attachment 82453 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:636:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:637:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:638:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:639:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:644:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:647:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Tests are OK on my Mac (there are a few error but not related to this).

[Balazs Kelemen]

Comment on attachment 82453 [details]
Patch

As discussed on IRC this should rather be fixed by adding API to WKURL.

[Balazs Kelemen]

Created attachment 82975 [details]
Patch

[Kimmo Kinnunen]

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

[Kenneth Rohde Christiansen]

Darin can you have a look at the new C API?

[Adam Roben (:aroben)]

Comment on attachment 82975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82975&action=review

> Source/WebCore/WebCore.exp.in:-632
> -__ZN7WebCore4KURLC1ENS_18ParsedURLStringTagEPKc

You should also revert r77816.

> Source/WebKit2/Shared/API/c/WKString.h:50
> -WK_EXPORT WKStringRef WKStringCreateWithUTF8CString(const char* string);
> +WK_EXPORT WKStringRef WKStringCreateWithUTF8CString(const char*);
>  
> -WK_EXPORT bool WKStringIsEmpty(WKStringRef string);
> +WK_EXPORT bool WKStringIsEmpty(WKStringRef);
>  
> -WK_EXPORT size_t WKStringGetMaximumUTF8CStringSize(WKStringRef string);
> +WK_EXPORT size_t WKStringGetMaximumUTF8CStringSize(WKStringRef);
>  WK_EXPORT size_t WKStringGetUTF8CString(WKStringRef string, char* buffer, size_t bufferSize);
>  
> -WK_EXPORT bool WKStringIsEqual(WKStringRef a, WKStringRef b);
> -WK_EXPORT bool WKStringIsEqualToUTF8CString(WKStringRef a, const char* b);
> +WK_EXPORT bool WKStringIsEqual(WKStringRef, WKStringRef);
> +WK_EXPORT bool WKStringIsEqualToUTF8CString(WKStringRef, const char*);
> +WK_EXPORT bool WKStringIsEqualToUTF8CStringIgnoringCase(WKStringRef, const char*);

So far all of our C API headers do not omit argument names. I'd suggest undoing those changes. We can always discuss that separately from this patch.

> Source/WebKit2/Shared/API/c/WKURL.h:41
> -WK_EXPORT WKURLRef WKURLCreateWithUTF8CString(const char* string);
> +WK_EXPORT WKURLRef WKURLCreateWithUTF8CString(const char*);
>  
> -WK_EXPORT WKStringRef WKURLCopyString(WKURLRef URL);
> +WK_EXPORT WKStringRef WKURLCopyString(WKURLRef);
>  
> -WK_EXPORT bool WKURLIsEqual(WKURLRef a, WKURLRef b);
> +WK_EXPORT bool WKURLIsEqual(WKURLRef, WKURLRef);

Same comment here about argument names.

> Source/WebKit2/Shared/API/c/WKURL.h:45
> +WK_EXPORT WKStringRef WKURLCopyProtocol(WKURLRef);

The correct term is "scheme", not "protocol".

> Source/WebKit2/Shared/WebURL.h:62
> +    String host() const
> +    {
> +        WebCore::KURL url(WebCore::KURL(), m_string);
> +        return url.isValid() ? url.host() : String();
> +    }
> +    
> +    String protocol() const
> +    {
> +        WebCore::KURL url(WebCore::KURL(), m_string);
> +        return url.isValid() ? url.protocol() : String();
> +    }

It seems unfortunate to create and throw away a KURL each time these functions are called.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:627
> +    WKRetainPtr<WKStringRef> host = WKURLCopyHostName(url.get());
> +    WKRetainPtr<WKStringRef> protocol = WKURLCopyProtocol(url.get());

These two lines will leak. You need to adopt the result of Copy-style functions.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:628
> +    if (host.get()

No need for .get() here.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:632
> +        && (!WKStringIsEqualToUTF8CString(host.get(), "127.0.0.1"))
> +        && (!WKStringIsEqualToUTF8CString(host.get(), "255.255.255.255")) // Used in some tests that expect to get back an error.
> +        && (!WKStringIsEqualToUTF8CStringIgnoringCase(host.get(), "localhost"))) {

I don't think the extra parentheses are helping here.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:633
> +        InjectedBundle::shared().os() << "Blocked access to external URL " << toSTD(adoptWK(WKURLCopyString(url.get()))) << "\n";

We should add an operator<< that takes a WKURLRef.

[Adam Roben (:aroben)]

Comment on attachment 82975 [details]
Patch

I guess my previous comments were enough to warrant an r- on this version.

[Balazs Kelemen]

Created attachment 83899 [details]
Patch

Incorporated review comments with the exception of still using temporary KURL's.

[WebKit Review Bot]

Attachment 83899 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/Shared/API/c/WKURL.h:39:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/API/c/WKURL.h:43:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/API/c/WKURL.h:45:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Ping. Sam, it has been sad that you are the most appropriate man to review this,
but I don't find you on IRC. Could you review the patch? Thanks.

[Adam Roben (:aroben)]

Comment on attachment 83899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83899&action=review

I think this is really close! Just a few comments.

>> Source/WebKit2/Shared/API/c/WKURL.h:43
>> +WK_EXPORT WKStringRef WKURLCopyHostName(WKURLRef url);
> 
> The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]

I think you should put these new functions just beneath WKURLCopyString, all in the same paragraph.

> Source/WebKit2/Shared/WebURL.h:62
> +    String host() const
> +    {
> +        WebCore::KURL url(WebCore::KURL(), m_string);
> +        return url.isValid() ? url.host() : String();
> +    }
> +    
> +    String protocol() const
> +    {
> +        WebCore::KURL url(WebCore::KURL(), m_string);
> +        return url.isValid() ? url.protocol() : String();
> +    }

Maybe it would be better to have an OwnPtr<KURL> data member. It would be lazily initialized using m_string. That way only the first call to one of these functions would have to pay the cost of parsing the string.

> Tools/WebKitTestRunner/StringFunctions.h:101
> +    WKRetainPtr<WKStringRef> urlString(AdoptWK, WKURLCopyString(urlRef));
> +    return out << toSTD(urlString.get());

Can you use the adoptWK function here?

No need for the .get().

[Balazs Kelemen]

Created attachment 85598 [details]
Patch

[WebKit Review Bot]

Attachment 85598 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/Shared/API/c/WKURL.h:39:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/API/c/WKURL.h:40:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/Shared/API/c/WKURL.h:41:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

(In reply to comment #18)
> (From update of attachment 83899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83899&action=review
> 
> I think this is really close! Just a few comments.
> 
> >> Source/WebKit2/Shared/API/c/WKURL.h:43
> >> +WK_EXPORT WKStringRef WKURLCopyHostName(WKURLRef url);
> > 
> > The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> I think you should put these new functions just beneath WKURLCopyString, all in the same paragraph.

Done.

> 
> > Source/WebKit2/Shared/WebURL.h:62
> > +    String host() const
> > +    {
> > +        WebCore::KURL url(WebCore::KURL(), m_string);
> > +        return url.isValid() ? url.host() : String();
> > +    }
> > +    
> > +    String protocol() const
> > +    {
> > +        WebCore::KURL url(WebCore::KURL(), m_string);
> > +        return url.isValid() ? url.protocol() : String();
> > +    }
> 
> Maybe it would be better to have an OwnPtr<KURL> data member. It would be lazily initialized using m_string. That way only the first call to one of these functions would have to pay the cost of parsing the string.

Done.

> 
> > Tools/WebKitTestRunner/StringFunctions.h:101
> > +    WKRetainPtr<WKStringRef> urlString(AdoptWK, WKURLCopyString(urlRef));
> > +    return out << toSTD(urlString.get());
> 
> Can you use the adoptWK function here?

Done. adoptWK was defined in InjectedBundlePage.cpp so I moved it to here
(this file is included almost everywhere).

I will upload the bug for the check-webkit-style as discussed on IRC in a day
or two.

[Balazs Kelemen]

(In reply to comment #20)
> Attachment 85598 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebKit2/Shared/API/c/WKURL.h:39:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WebKit2/Shared/API/c/WKURL.h:40:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/WebKit2/Shared/API/c/WKURL.h:41:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 3 in 12 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Filed bug with patch against check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=56321

[Adam Roben (:aroben)]

Comment on attachment 85598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85598&action=review

> Source/WebKit2/Shared/WebURL.h:72
> +        if (!!m_parsedURL)

No need for the !! here.

> Source/WebKit2/Shared/WebURL.h:74
> +        m_parsedURL.set(new WebCore::KURL(WebCore::KURL(), m_string));

Please use adoptPtr and assignment here instead of OwnPtr::set.

> Tools/WebKitTestRunner/StringFunctions.h:100
> +inline std::ostream& operator<<(std::ostream& out, const WKURLRef urlRef)

No need for the "const" here.

[Balazs Kelemen]

Committed r81084: <http://trac.webkit.org/changeset/81084>