Bug 131104

Summary: [EFL] TLSErrors do not cause page load to fail when not ignored
Product: WebKit Reporter: Peter Molnar <pmolnar.u-szeged>
Component: WebKit EFLAssignee: Peter Molnar <pmolnar.u-szeged>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, brian.holt, bunhere, cdumez, cgarcia, commit-queue, danw, gustavo, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, mario, mrobinson, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 121548    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch - unit test only
gyuyoung.kim: review+
patch for landing none

Description Peter Molnar 2014-04-02 06:54:22 PDT
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.
Comment 1 Peter Molnar 2014-04-02 06:57:34 PDT
Created attachment 228394 [details]
patch
Comment 2 Brian Holt 2014-04-02 08:14:30 PDT
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.
Comment 3 Gyuyoung Kim 2014-04-02 18:41:34 PDT
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.
Comment 4 Peter Molnar 2014-04-03 01:25:16 PDT
(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.
Comment 5 Gyuyoung Kim 2014-04-03 05:47:43 PDT
(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.
Comment 6 Peter Molnar 2014-04-03 05:56:17 PDT
(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.
Comment 7 Mario Sanchez Prada 2014-04-30 02:45:37 PDT
(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
Comment 8 Carlos Garcia Campos 2014-04-30 03:07:53 PDT
(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)
Comment 9 Brian Holt 2014-04-30 03:19:44 PDT
(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.
Comment 10 Mario Sanchez Prada 2014-04-30 03:23:43 PDT
(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
Comment 11 Peter Molnar 2014-04-30 04:03:50 PDT
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
Comment 12 Mario Sanchez Prada 2014-05-02 04:07:52 PDT
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!
Comment 13 Gyuyoung Kim 2014-05-02 20:51:13 PDT
(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.
Comment 14 Peter Molnar 2014-05-07 03:24:49 PDT
Created attachment 230984 [details]
patch - unit test only

Updated the patch to contain only the unit test.
Comment 15 Gyuyoung Kim 2014-05-12 04:58:29 PDT
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.
Comment 16 Peter Molnar 2014-05-12 05:10:48 PDT
Created attachment 231287 [details]
patch for landing

Patch for landing.
Comment 17 WebKit Commit Bot 2014-05-12 05:49:26 PDT
Comment on attachment 231287 [details]
patch for landing

Clearing flags on attachment: 231287

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