Bug 131104 – [EFL] TLSErrors do not cause page load to fail when not ignored

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Peter Molnar]

TLSErrors do not cause page load to fail when not ignored

Created an EFL test case to reproduce this GTK bug (original patch by Brian Holt): https://bugs.webkit.org/show_bug.cgi?id=121548 ,
because the bug also exists on EFL.

This way we can catch this kind of TLS handling bugs in the future.

[Peter Molnar]

Created attachment 228394 [details]
patch

[Brian Holt]

Thanks for the test case Peter! Once your patch is in, I'd like to add the unit test (I know it will need some changing to GTK+) and push my patch again.

[Gyuyoung Kim]

Comment on attachment 228394 [details]
patch

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

> Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:70
> +    GTlsCertificateFlags tlsFlags = static_cast<GTlsCertificateFlags>(0);

Wouldn't it good this patch is landed by Bug 131104 ? Then, how about landing this patch ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:44
> +WTF::CString http_uri;

Not webkit coding style.

[Peter Molnar]

(In reply to comment #3)
> (From update of attachment 228394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228394&action=review
> 
> > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:70
> > +    GTlsCertificateFlags tlsFlags = static_cast<GTlsCertificateFlags>(0);
> 
> Wouldn't it good this patch is landed by Bug 131104 ? Then, how about landing this patch ?

This bug is 131104. Did you mean 121548?

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:44
> > +WTF::CString http_uri;
> 
> Not webkit coding style.

[Gyuyoung Kim]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 228394 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=228394&action=review
> > 
> > > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:70
> > > +    GTlsCertificateFlags tlsFlags = static_cast<GTlsCertificateFlags>(0);
> > 
> > Wouldn't it good this patch is landed by Bug 131104 ? Then, how about landing this patch ?
> 
> This bug is 131104. Did you mean 121548?

Oops, right.

[Peter Molnar]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 228394 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=228394&action=review
> > > 
> > > > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:70
> > > > +    GTlsCertificateFlags tlsFlags = static_cast<GTlsCertificateFlags>(0);
> > > 
> > > Wouldn't it good this patch is landed by Bug 131104 ? Then, how about landing this patch ?
> > 
> > This bug is 131104. Did you mean 121548?
> 
> Oops, right.

Ok, I'm setting it as depencency.

[Mario Sanchez Prada]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 228394 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=228394&action=review
> > > 
> > > > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:70
> > > > +    GTlsCertificateFlags tlsFlags = static_cast<GTlsCertificateFlags>(0);
> > > 
> > > Wouldn't it good this patch is landed by Bug 131104 ? Then, how about landing this patch ?
> > 
> > This bug is 131104. Did you mean 121548?
> 
> Oops, right.

I'm not sure if Brian will be able to land this as part of 121548 promptly enough (Brian, please confirm that), so perhaps a better approach would be to land it as an EFL patch now that we have an unit test in one upstream port at least and, later on, finishing GTK's patch by adding an unit test for the same thing there as well.

But IMHO we should not leave this *important* bug unfixed any longer.

I'd be happy to r+ this patch based on Carlos's comment on bug 121548[1], if somebody else (Gyuyoung?) reviewed the EFL unit test.

[1] https://bugs.webkit.org/show_bug.cgi?id=121548#c7

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (From update of attachment 228394 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=228394&action=review
> > > > 
> > > > > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:70
> > > > > +    GTlsCertificateFlags tlsFlags = static_cast<GTlsCertificateFlags>(0);
> > > > 
> > > > Wouldn't it good this patch is landed by Bug 131104 ? Then, how about landing this patch ?
> > > 
> > > This bug is 131104. Did you mean 121548?
> > 
> > Oops, right.
> 
> I'm not sure if Brian will be able to land this as part of 121548 promptly enough (Brian, please confirm that), so perhaps a better approach would be to land it as an EFL patch now that we have an unit test in one upstream port at least and, later on, finishing GTK's patch by adding an unit test for the same thing there as well.
> 
> But IMHO we should not leave this *important* bug unfixed any longer.
> 
> I'd be happy to r+ this patch based on Carlos's comment on bug 121548[1], if somebody else (Gyuyoung?) reviewed the EFL unit test.

Since bug #121548 has more information about the issue than this one, what I would do is:

1.- Rename #121548 to use [SOUP] prefix instead of [GTK]  and land only the original brian's patch with the fix
2.- Attach a patch here to include the EFL unit test
3.- File a new bug report to include the GTK unit test (that I hope is added soon)

[Brian Holt]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #3)
> > > > > (From update of attachment 228394 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=228394&action=review
> > > > > 
> > > > > > Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:70
> > > > > > +    GTlsCertificateFlags tlsFlags = static_cast<GTlsCertificateFlags>(0);
> > > > > 
> > > > > Wouldn't it good this patch is landed by Bug 131104 ? Then, how about landing this patch ?
> > > > 
> > > > This bug is 131104. Did you mean 121548?
> > > 
> > > Oops, right.
> > 
> > I'm not sure if Brian will be able to land this as part of 121548 promptly enough (Brian, please confirm that), so perhaps a better approach would be to land it as an EFL patch now that we have an unit test in one upstream port at least and, later on, finishing GTK's patch by adding an unit test for the same thing there as well.
> > 
> > But IMHO we should not leave this *important* bug unfixed any longer.
> > 
> > I'd be happy to r+ this patch based on Carlos's comment on bug 121548[1], if somebody else (Gyuyoung?) reviewed the EFL unit test.
> 
> Since bug #121548 has more information about the issue than this one, what I would do is:
> 
> 1.- Rename #121548 to use [SOUP] prefix instead of [GTK]  and land only the original brian's patch with the fix
> 2.- Attach a patch here to include the EFL unit test
> 3.- File a new bug report to include the GTK unit test (that I hope is added soon)

I'm sorry I've been so quiet, been working downstream a lot over the past months.

I'll take the EFL unit test, modify it to GTK and submit a new patch. Should have something today.

[Mario Sanchez Prada]

(In reply to comment #9)
> [...]
> > Since bug #121548 has more information about the issue than this one, what I would do is:
> > 
> > 1.- Rename #121548 to use [SOUP] prefix instead of [GTK]  and land only the original brian's patch with the fix
> > 2.- Attach a patch here to include the EFL unit test
> > 3.- File a new bug report to include the GTK unit test (that I hope is added soon)
> 
> I'm sorry I've been so quiet, been working downstream a lot over the past months.
> 
> I'll take the EFL unit test, modify it to GTK and submit a new patch. Should have something today.

Before doing that, I'd suggest that you follow the priority list from Carlos above, and that you just start by renaming your previous bug 121548, addressing the last comment that Carlos pointed it there and land that one. That should be a very quick thing, probably a matter of minutes :)

After that, Peter can work on submitting a new patch here just with the EFL test and you will have more time to work on the same thing for GTK, as Carlos suggested.

But let's get the fix landed first as main prio, I'd say

[Peter Molnar]

The problem I see here is that in this case, the fix would be landed without any unit test. So there will be no test for a (hopefully) brief amount of time until my EFL test is landed as a separate bug. If this is acceptable, then we should go on and land the renamed bug #121548 now, and after it has landed, I can update this test.

(In reply to comment #10)
> (In reply to comment #9)
> > [...]
> > > Since bug #121548 has more information about the issue than this one, what I would do is:
> > > 
> > > 1.- Rename #121548 to use [SOUP] prefix instead of [GTK]  and land only the original brian's patch with the fix
> > > 2.- Attach a patch here to include the EFL unit test
> > > 3.- File a new bug report to include the GTK unit test (that I hope is added soon)
> > 
> > I'm sorry I've been so quiet, been working downstream a lot over the past months.
> > 
> > I'll take the EFL unit test, modify it to GTK and submit a new patch. Should have something today.
> 
> Before doing that, I'd suggest that you follow the priority list from Carlos above, and that you just start by renaming your previous bug 121548, addressing the last comment that Carlos pointed it there and land that one. That should be a very quick thing, probably a matter of minutes :)
> 
> After that, Peter can work on submitting a new patch here just with the EFL test and you will have more time to work on the same thing for GTK, as Carlos suggested.
> 
> But let's get the fix landed first as main prio, I'd say

[Mario Sanchez Prada]

Peter, Gyuyoung: do you think it would be possible to get a review over the EFL unit test *only*, so the patch is ready to land as soon as the real fix lands as part of bug 121548?

See https://bugs.webkit.org/show_bug.cgi?id=121548#c23. Thanks!

[Gyuyoung Kim]

(In reply to comment #12)
> Peter, Gyuyoung: do you think it would be possible to get a review over the EFL unit test *only*, so the patch is ready to land as soon as the real fix lands as part of bug 121548?
> 
> See https://bugs.webkit.org/show_bug.cgi?id=121548#c23. Thanks!

As I said in comment #3, I also think this patch is able to be landed with only EFL unit test after finishing Bug 121548.

[Peter Molnar]

Created attachment 230984 [details]
patch - unit test only

Updated the patch to contain only the unit test.

[Gyuyoung Kim]

Comment on attachment 230984 [details]
patch - unit test only

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

LGTM except for a trivial naming nit.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_ssl.cpp:44
> +WTF::CString http_uri;

Please don't use _ in variable name. httpUri is ok to me.

[Peter Molnar]

Created attachment 231287 [details]
patch for landing

Patch for landing.

[WebKit Commit Bot]

Comment on attachment 231287 [details]
patch for landing

Clearing flags on attachment: 231287

Committed r168619: <http://trac.webkit.org/changeset/168619>