|Summary:||Inspector/Qt requires patch to SecurityOrigin|
|Product:||WebKit||Reporter:||Holger Freyther <zecke>|
|Component:||WebKit Qt||Assignee:||George Staikos <staikos>|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.4|
Description Holger Freyther 2007-11-10 17:35:54 PST
Comment 1 Holger Freyther 2007-11-10 17:41:07 PST
Created attachment 17177 [details] Use FrameLoader::shouldTreatURLAsLocal from SecurityOrigin::canAccess This patch is using FrameLoader::shouldTreatURLAsLocal instead of 'file' in SecurityOrigin::canAccess. isSecureTransition is not changed. FrameLoader::shouldTreatURLAsLocal requires a String as argument and passing m_protocol would not work so I decided to keep the KURL::url() in m_url. I wonder why the comparsion of 'file' is hardcoded and not KURL::isLocalFile is used... I would like to receive comments regarding: -Correctnes -Security -Speed
Comment 2 Sam Weinig 2007-11-11 11:45:45 PST
Comment on attachment 17177 [details] Use FrameLoader::shouldTreatURLAsLocal from SecurityOrigin::canAccess r-. I don't think adding the m_url member variable is a good solution as it adds redundant information. Either we should only have the m_url or we should make a new method that takes the protocol and determines if it should be treated as local (eg. FrameLoader::shouldTreatProtocolAsLocal).
Comment 3 Holger Freyther 2007-11-11 13:26:03 PST
Created attachment 17188 [details] Use FrameLoader::shouldTreatURLAsLocal from SecurityOrigin::canAccess Remove the unused m_portSet. Remove m_host, m_protocol and m_port in favor of storing a KURL directly. Use KURL::isLocalFile instead of comparing to "file" directly. Avoid calling lower() on (Deprecated)String but use the case insensitive compare besides in ::setFrame. Like the last patch no regressions were introduced and please take a look at it in regard to: -performance -security -correctness
Comment 4 Holger Freyther 2007-11-13 16:05:05 PST
Created attachment 17243 [details] Add FrameLoader::shouldTreatSchemeAsLocal Altnernative implementation. Add FrameLoader::shouldTreatSchemeAsLocal and call it with KURL::protocol from within FrameLoader and SecurityOrigin when we have a KURL. In CachedResource and Document we continue to use the shouldTreatURLAsLocal function as we have (Deprecated)Strings there. This function is doing the splitting of the URL first before it is comparing file and http as fast path. And due not using .find in shouldTreatSchemeAsLocal we need to check for the null string otherwise we end up with a crash in the HashTable. This introduces no regressions and I would welcome feedback and specially in regard to Performance and Correctness.
Comment 5 Darin Adler 2007-11-17 15:35:58 PST
Comment on attachment 17243 [details] Add FrameLoader::shouldTreatSchemeAsLocal The additional string allocation from calling left() on the URL string will almost certainly cause an unacceptable slowdown; there's a reason we added those special cases for "http" and "file" to avoid memory allocation. The concept of the patch is fine, but we need a more efficient version.
Comment 6 Holger Freyther 2007-11-18 12:10:02 PST
(In reply to comment #5) > (From update of attachment 17243 [details] ) > The additional string allocation from calling left() on the URL string will > almost certainly cause an unacceptable slowdown; there's a reason we added > those special cases for "http" and "file" to avoid memory allocation. I don't wear my Knuth is my homeboy T-Shirt but seeing numbers would be appreciated. E.g. is FrameLoader calling the Scheme version more often than Document and CachedResource the URL version? I admit to have not researched the details of the original function because I assume(d) that the tests are not public. > > The concept of the patch is fine, but we need a more efficient version. This boils down to: a) Code duplication? b) Better data structure for the local schemes. e.g. a Trie? c) Something I miss. So what solution is preferred?
Comment 7 Holger Freyther 2007-11-24 04:29:15 PST
Created attachment 17483 [details] ASCII Trie implemented with HasMap<UChar,...> - Security: I changed both occurences of "file" in SecuritOrigin to use FrameLoader::shouldTreatSchemeAsLocal - Implement looking up the scheme using a Trie. As of the URI RFC the scheme must be alpha and this is why I can use a fixed size array for lookup. - No idea how the ASCIIType usage impacts the performance. Comments are welcome.
Comment 8 Holger Freyther 2007-11-24 04:30:52 PST
Comment on attachment 17483 [details] ASCII Trie implemented with HasMap<UChar,...> -Sorry this is the HashMap implementation
Comment 9 Holger Freyther 2007-11-24 04:33:24 PST
Created attachment 17484 [details] ASCII Trie with wasting memory for faster lookup - SecurityOrigin now uses FrameLoader::shouldTreatSchemeAsLocal in two places, please verify that carefully! - Use an array with 'z'-'a' number of elements as we can guarantee to only use lower case ascii. This is backed by the URI RFC where the scheme is of type ALPHA.
Comment 10 Maciej Stachowiak 2007-11-24 18:55:11 PST
I don't think using a custom data structure for this is necessary. Just having both shouldTreatURLAsLocal() and shouldTreatSchemeAsLocal functions do the fast path check for http and file and maybe share code through a common function should be sufficient.
Comment 11 Maciej Stachowiak 2007-11-24 18:55:31 PST
I would also be happy to performance test any patches that need it.
Comment 12 Sam Weinig 2007-11-24 19:35:21 PST
I agree with Maciej that the Trie seems a little overkill for this situation. I would like to add that it might be helpful to test https in addition to http and file as it is also quite common.
Comment 13 Holger Freyther 2007-11-30 09:58:14 PST
Created attachment 17607 [details] Add shouldTreatSchemeAsLocal to FrameLoader Same as the older patch. For performance reasons the shouldTreatURLAsLocal case is not reduced to the shouldTreatSchemeAsLocal case. This adds a little code duplication to avoid doing the malloc.
Comment 14 Holger Freyther 2007-11-30 10:04:07 PST
Created attachment 17608 [details] Performance question This replaces the char compare s, s... with WTF::Unicode::umemcasecmp. There should be no extra mallocs, we catch big and small letters and I wonder if that would be even faster?
Comment 15 Darin Adler 2007-11-30 17:35:08 PST
Comment on attachment 17608 [details] Performance question I suspect umemcasecmp will be much slower because of function call overhead.
Comment 16 Maciej Stachowiak 2007-12-02 23:01:49 PST
Comment on attachment 17607 [details] Add shouldTreatSchemeAsLocal to FrameLoader I performance-tested this patch. It appears to be faster on both HTML iBench and PLT than baseline.
Comment 17 Sam Weinig 2007-12-02 23:19:05 PST
Comment on attachment 17607 [details] Add shouldTreatSchemeAsLocal to FrameLoader r=me if you address the comments I had in IRC
Comment 18 Mark Rowe (bdash) 2007-12-03 07:09:25 PST
This was landed in 28343.