Bug 15938 – Inspector/Qt requires patch to SecurityOrigin

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Holger Freyther]

On the Qt port the Inspector will be embedded into the binary using QResources (qrc). The embedded files can be accessed using the qrc protocol.
The issue is that the Inspector JavaScript requires certain priviliges to function properly. This is where isSafeScript and SecurityOrigin are involved. For the mac/win port the inspector.html is found inside the bundle and can be accessed using the file protocol. So SecurityOrigin grants the needed priviliges when using 'file' but not when using 'qrc'. One solution would be to use FrameLoader::canTreatAsLocalURL in SecurityOrigin::canAccess.

[Holger Freyther]

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

[Sam Weinig]

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).

[Holger Freyther]

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

[Holger Freyther]

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.

[Darin Adler]

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.

[Holger Freyther]

(In reply to comment #5)
> (From update of attachment 17243 [details] [edit])
> 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?

[Holger Freyther]

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.

[Holger Freyther]

Comment on attachment 17483 [details]
ASCII Trie implemented with HasMap<UChar,...>

-Sorry this is the HashMap implementation

[Holger Freyther]

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.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

I would also be happy to performance test any patches that need it.

[Sam Weinig]

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.

[Holger Freyther]

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.

[Holger Freyther]

Created attachment 17608 [details]
Performance question

This replaces the char compare s[0], s[1]... 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?

[Darin Adler]

Comment on attachment 17608 [details]
Performance question

I suspect umemcasecmp will be much slower because of function call overhead.

[Maciej Stachowiak]

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.

[Sam Weinig]

Comment on attachment 17607 [details]
Add shouldTreatSchemeAsLocal to FrameLoader

r=me if you address the comments I had in IRC

[Mark Rowe (bdash)]

This was landed in 28343.