Bug 146199 – [EFL][CustomProtocol] Do not add duplicated custom scheme

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

Current WebSoupCustomProtocolRequestManager::registerSchemeForCustomProtocol generates a crash when duplicated scheme is registered. However application can register duplicate scheme by mistake or on purpose. Thus IMO it would be good if we don't register it instead of registering it or generating a crash on debug mode when duplicated scheme is registered.

[Gyuyoung Kim]

Created attachment 255340 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 255340 [details]
Patch

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

> Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:85
> +    ASSERT(!scheme.isNull());
> +    if (m_registeredSchemes.contains(scheme))
> +        return;

I don't think this is the right place to check this. I think APIs should check it earlier. Registering the same scheme twice is a programmer error, at least in GTK+ API, but with this patch, there could be a change in behaviour, I mean, imagine you do:

ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback1, nullptr);
ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback2, nullptr);

With this patch, the scheme will not be registered again, but the handler will be replaced with the second callback. If that's the desired behaviour for EFL, then the patch is correct. I'll add a g_return_if_fail protector to GTK+ API anyway.

[Carlos Garcia Campos]

I'm thinking that maybe replacing the callback is indeed the best approach. We would just need to document it, though.

[Gyuyoung Kim]

Comment on attachment 255340 [details]
Patch

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

>> Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:85
>> +        return;
> 
> I don't think this is the right place to check this. I think APIs should check it earlier. Registering the same scheme twice is a programmer error, at least in GTK+ API, but with this patch, there could be a change in behaviour, I mean, imagine you do:
> 
> ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback1, nullptr);
> ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback2, nullptr);
> 
> With this patch, the scheme will not be registered again, but the handler will be replaced with the second callback. If that's the desired behaviour for EFL, then the patch is correct. I'll add a g_return_if_fail protector to GTK+ API anyway.

I would like to suggest a feature that user can change registered callback for same scheme. If GTK port don't mind to check same scheme in this place, I upload a patch which modifies callback as you mentioned.

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 255340 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255340&action=review
> 
> >> Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:85
> >> +        return;
> > 
> > I don't think this is the right place to check this. I think APIs should check it earlier. Registering the same scheme twice is a programmer error, at least in GTK+ API, but with this patch, there could be a change in behaviour, I mean, imagine you do:
> > 
> > ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback1, nullptr);
> > ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback2, nullptr);
> > 
> > With this patch, the scheme will not be registered again, but the handler will be replaced with the second callback. If that's the desired behaviour for EFL, then the patch is correct. I'll add a g_return_if_fail protector to GTK+ API anyway.
> 
> I would like to suggest a feature that user can change registered callback
> for same scheme. If GTK port don't mind to check same scheme in this place,
> I upload a patch which modifies callback as you mentioned.

I'm not sure I get what you mean, this patch already modifies the callback if I understood it correctly. If that was your intention and the desired behaviour for EFL API, I suggest you to update the unit test to not only check you can register the same scheme twice without crashing, but also that the second callback is the one called after the second register.

[Gyuyoung Kim]

Comment on attachment 255340 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:85
>>>> +        return;
>>> 
>>> I don't think this is the right place to check this. I think APIs should check it earlier. Registering the same scheme twice is a programmer error, at least in GTK+ API, but with this patch, there could be a change in behaviour, I mean, imagine you do:
>>> 
>>> ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback1, nullptr);
>>> ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback2, nullptr);
>>> 
>>> With this patch, the scheme will not be registered again, but the handler will be replaced with the second callback. If that's the desired behaviour for EFL, then the patch is correct. I'll add a g_return_if_fail protector to GTK+ API anyway.
>> 
>> I would like to suggest a feature that user can change registered callback for same scheme. If GTK port don't mind to check same scheme in this place, I upload a patch which modifies callback as you mentioned.
> 
> I'm not sure I get what you mean, this patch already modifies the callback if I understood it correctly. If that was your intention and the desired behaviour for EFL API, I suggest you to update the unit test to not only check you can register the same scheme twice without crashing, but also that the second callback is the one called after the second register.

yes, you understand what I meant correctly. I'm modifying this test now.

[Gyuyoung Kim]

Created attachment 255352 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 255352 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:155
>      ASSERT_TRUE(waitUntilTrue(finishTest, testTimeoutSeconds));

How do you know it was the second callback the one finishing the test and not the first one if both change the same variable?

[Gyuyoung Kim]

Comment on attachment 255352 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:155
>>      ASSERT_TRUE(waitUntilTrue(finishTest, testTimeoutSeconds));
> 
> How do you know it was the second callback the one finishing the test and not the first one if both change the same variable?

To check if second callback is called correctly, I change custom scheme's url with *MyNewPath* in 154 line. Then the second callback(:= schemeRequestCallback2()) checks the changed url as below,

    static void schemeRequestCallback2(Ewk_Url_Scheme_Request* request, void* userData)
    {
        ...
        ASSERT_STREQ("fooscheme:MyNewPath", url);
        const char* path = ewk_url_scheme_request_path_get(request);
        ASSERT_STREQ("MyNewPath", path);
    }

Besides *finishTest* flag is reset to test second callback.

[Carlos Garcia Campos]

Comment on attachment 255352 [details]
Patch

Ah, ok, I didn't notice the paths detail, sorry.

[Gyuyoung Kim]

(In reply to comment #10)
> Comment on attachment 255352 [details]
> Patch
> 
> Ah, ok, I didn't notice the paths detail, sorry.

No problem. Your review is nice !

[WebKit Commit Bot]

Comment on attachment 255352 [details]
Patch

Clearing flags on attachment: 255352

Committed r185866: <http://trac.webkit.org/changeset/185866>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.