Review Request 111335: Fix for one of the oldest KIO bugs: multiple dialogs when KIO encounters SSL errors

[Dawit Alemayehu]

> On July 3, 2013, 10:48 a.m., Andreas Hartmetz wrote:
> > kio/kio/scheduler.cpp, line 660
> > <http://git.reviewboard.kde.org/r/111335/diff/1/?file=166900#file166900line660>
> >
> >     Just return a QString. With the pointer you have more verbosity in the caller, and the state of key is unclear after the call because it may or may not have been changed.
> >     
> >     The Request could be passed as a const reference here, so why not do that?
> >     
> >     Would it make sense to have this method in the Request class as
> >     QString key() const ?

I agree it makes more sense to have this in the Request class itself. Done.


> On July 3, 2013, 10:48 a.m., Andreas Hartmetz wrote:
> > kio/kio/scheduler.cpp, line 781
> > <http://git.reviewboard.kde.org/r/111335/diff/1/?file=166900#file166900line781>
> >
> >     What the heck did I do there? :O
> >     (/*H4X*/ is something I use to mark temporary hacks)

No idea, but if it is you that did that then I will let you resolve it since that if statement is currently unnecessary. :)


> On July 3, 2013, 10:48 a.m., Andreas Hartmetz wrote:
> > kio/kio/scheduler.cpp, line 808
> > <http://git.reviewboard.kde.org/r/111335/diff/1/?file=166900#file166900line808>
> >
> >     m_cachedResults is never cleaned - I admit that this is not much different from the SSL config file never losing exceptions stored there.
> >     
> >     So I suggest cleaning m_cachedResults, but the current patch is not significantly worse than what we have.
> >     
> >     This is related to making an SSL exception for "current sessions only", which is actually for 30 minutes. Unless you also make it really for current sessions, after 30 minutes would be a good time to clear such entries.

Wow. I did not realize "current sessions only" meant 30 minutes! That is very misleading. To me "current sessions only" means the "current browsing session", i.e. until the window that requested it is closed. Anyhow, my original assumption here was that this is a relatively rare occurrence and as such m_cachedResults is very unlikely to grow to the point where I needed to worry about it. However, since assumptions are usually the mother of all ****ups, I rather simply convert the container to a QCache and avoid the whole issue of clearing it after 30 minutes.


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111335/#review35497
-----------------------------------------------------------


On July 1, 2013, 5:10 a.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111335/
> -----------------------------------------------------------
> 
> (Updated July 1, 2013, 5:10 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> The attached patch addresses one of the oldest bugs in KIO. Due to the muti-process nature of KIO, if any of the ioslaves encounter something that requires user input, the user might end up getting prompted multiple times. The best example of this is SSL error warnings sent to the client by kio_http. The patch completely resolves this problem using the same approach as kpasswdserver, but without the need for an additional kded process.
> 
> 
> This addresses bugs 154100 and 265228.
>     http://bugs.kde.org/show_bug.cgi?id=154100
>     http://bugs.kde.org/show_bug.cgi?id=265228
> 
> 
> Diffs
> -----
> 
>   kio/kio/scheduler.h 04edb40 
>   kio/kio/scheduler.cpp 802f8b8 
>   kio/kio/scheduler_p.h d68f645 
>   kio/kio/slaveinterface.h 4bfcec8 
>   kio/kio/slaveinterface.cpp aa0fc44 
>   kio/kio/slaveinterface_p.h e31ec5e 
> 
> Diff: http://git.reviewboard.kde.org/r/111335/diff/
> 
> 
> Testing
> -------
> 
> Visit a site that throws up SSL warnings and causes KIO to show more than one error dialog.
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130703/632a8aff/attachment.htm>