Summary: | [EFL] TLSErrors do not cause page load to fail when not ignored | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Molnar <pmolnar.u-szeged> | ||||||||
Component: | WebKit EFL | Assignee: | 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
Peter Molnar
2014-04-02 06:54:22 PDT
Created attachment 228394 [details]
patch
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 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. (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. (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. (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. (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 (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) (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. (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 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 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! (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. Created attachment 230984 [details]
patch - unit test only
Updated the patch to contain only the unit test.
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. Created attachment 231287 [details]
patch for landing
Patch for landing.
Comment on attachment 231287 [details] patch for landing Clearing flags on attachment: 231287 Committed r168619: <http://trac.webkit.org/changeset/168619> |