Create a hash that uniquely identifies a combination of origins for plug-ins.
<rdar://problem/12778949>
Created attachment 176795 [details] Patch
Created attachment 176798 [details] Patch
Created attachment 176800 [details] Patch
Comment on attachment 176800 [details] Patch I don't think you want to force localHost if the plug-in or page-host are empty. In addition, you should also check the plug-in MIME type. I'm wondering if this should just take a HTMLPlugInElement instead.
Comment on attachment 176800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176800&action=review r- for layering violation. > Source/WebCore/ChangeLog:18 > + In the case where the host has an empty string (like a file URL), we substitute in "localhost". Why is that desirable? > Source/WebCore/platform/PlugInOriginHash.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY What's "Apple Computer, Inc."? > Source/WebCore/platform/PlugInOriginHash.cpp:41 > +static inline uint64_t codeFoldedHash(const String& string) > +{ > + if (string.is8Bit()) > + return StringHasher::computeHash<LChar, CaseFoldingHash::foldCase<LChar> >(string.characters8(), string.length()); The function you are calling is named "compute hash", so why does the new function not have this verb in its name? > Source/WebCore/platform/PlugInOriginHash.h:34 > +typedef uint64_t PlugInOriginHash; Our existing types named "Hash" are classes, see e.g. ones in StringHash.h. It's quite confusing to have a drastically different one for plug-ins. > Source/WebCore/platform/PlugInOriginHash.h:36 > +PlugInOriginHash plugInOriginHash(const Page*, const KURL& srcURL); Code in WebCore/platform cannot use Page or Frame classes. It's either (1) platform API wrappers that do not know about Web classes or concepts or (2) platform level code that we build from scratch. Even having "plugin" in the name is quite suspect.
Created attachment 177446 [details] Patch
Comment on attachment 177446 [details] Patch Attachment 177446 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15100945
Comment on attachment 177446 [details] Patch Attachment 177446 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15132359
Comment on attachment 177446 [details] Patch Attachment 177446 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15098972
Comment on attachment 177446 [details] Patch Attachment 177446 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15097949
Comment on attachment 177446 [details] Patch Attachment 177446 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15120760
Comment on attachment 177446 [details] Patch Attachment 177446 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15126551
Created attachment 177451 [details] Patch
Comment on attachment 177451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177451&action=review review- because there is a much better way to do this hash correctly > Source/WebCore/ChangeLog:19 > + benefit either, since retrieving the host from the KURL creates a brand new string every time. We shift > + the page's host hash, since otherwise, in the case of where the plug-in is loading from the same domain > + as the page, it would lead to a combined hash of 0. I don’t understand why a right shift of 1 is the right way to solve that problem. It throws away 1/32 of the host hash. I’d think a rotate would be better than a shift. > Source/WebCore/plugins/PlugInOriginHash.cpp:44 > +static inline unsigned computeCaseFoldedHash(const String& string) > +{ > + if (string.is8Bit()) > + return StringHasher::computeHash<LChar, CaseFoldingHash::foldCase<LChar> >(string.characters8(), string.length()); > + return StringHasher::computeHash<UChar, CaseFoldingHash::foldCase<UChar> >(string.characters16(), string.length()); > +} The fact that this is not a duplicate of the CaseFoldingHash::hash function from StringHash.h is a subtle detail that is not explained here. The change log mentions that we don’t want to mask the top 8 bits, but it’s easy to overlook that so we need a comment explaining that, not just change log. And I also think we should consider putting this function into StringHash.h in the future. We just need to figure out the right name for it. > Source/WebCore/plugins/PlugInOriginHash.cpp:51 > + String plugInSrcHost = plugInURL.host(); Why put this into a local variable, but not put plugInElement->serviceType() into a local variable? > Source/WebCore/plugins/PlugInOriginHash.cpp:53 > + return (computeCaseFoldedHash(pageHost) >> 1) ^ computeCaseFoldedHash(plugInSrcHost) ^ computeCaseFoldedHash(plugInElement->serviceType()); I’m not sure that exclusive or is a good way to combine these three hashes; normally we hash the hashes, or make a single hash. And the need to shift the hash of the page host and the source host is sufficiently non-obvious that it needs to be in a comment, not just change log. I think I know a much better way to do this. Here’s my idea. First, we refactor the StringHasher::computeHash static member function into a new StringHasher::addCharacters(const T* data, unsigned length) non-static member function, so we have a function we can use on an existing StringHasher multiple times. Then we write a function to add a string to a hash named addCaseFoldedCharacters, which calls addCharacters. Then we write this: StringHasher hasher; addCaseFoldedCharacters(hasher, page->mainFrame()->document()->baseURL().host()); hasher.addCharacter(0); addCaseFoldedCharacters(hasher, plugInURL.host()); hasher.addCharacter(0); addCaseFoldedCharacters(hasher, plugInElement->serviceType()); return hasher.hash(); This way, we won’t have to do any tricky shifting or worry about how well exclusive or works. > Source/WebCore/plugins/PlugInOriginHash.h:35 > + static unsigned hash(HTMLPlugInImageElement* plugInElement, const KURL& plugInURL); Normally we need an equality function to go along with a hash function. Are we actually going to rely on these hashes without also doing equality checks?
Comment on attachment 177451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177451&action=review >> Source/WebCore/plugins/PlugInOriginHash.cpp:53 >> + return (computeCaseFoldedHash(pageHost) >> 1) ^ computeCaseFoldedHash(plugInSrcHost) ^ computeCaseFoldedHash(plugInElement->serviceType()); > > I’m not sure that exclusive or is a good way to combine these three hashes; normally we hash the hashes, or make a single hash. And the need to shift the hash of the page host and the source host is sufficiently non-obvious that it needs to be in a comment, not just change log. > > I think I know a much better way to do this. Here’s my idea. First, we refactor the StringHasher::computeHash static member function into a new StringHasher::addCharacters(const T* data, unsigned length) non-static member function, so we have a function we can use on an existing StringHasher multiple times. Then we write a function to add a string to a hash named addCaseFoldedCharacters, which calls addCharacters. Then we write this: > > StringHasher hasher; > addCaseFoldedCharacters(hasher, page->mainFrame()->document()->baseURL().host()); > hasher.addCharacter(0); > addCaseFoldedCharacters(hasher, plugInURL.host()); > hasher.addCharacter(0); > addCaseFoldedCharacters(hasher, plugInElement->serviceType()); > return hasher.hash(); > > This way, we won’t have to do any tricky shifting or worry about how well exclusive or works. Excellent idea. I've posted a patch to implement StringHasher::addCharacters() in bug 104076. >> Source/WebCore/plugins/PlugInOriginHash.h:35 >> + static unsigned hash(HTMLPlugInImageElement* plugInElement, const KURL& plugInURL); > > Normally we need an equality function to go along with a hash function. Are we actually going to rely on these hashes without also doing equality checks? Normally you would, but this hash is not being used as a hash function for any data structure, and the intent was to only have the hashes shared among the processes like visited links. Having an equal() would imply needing to create a new struct for plug-in origins, and then sharing that data. I'm hoping with your proposed scheme that we avoid collisions. If we find ourselves with collisions, then I think we can revisit this, and switch from using HashSet<unsigned> to HashSet<PlugInOrigin> or something to that effect.
Created attachment 177773 [details] Patch
Comment on attachment 177773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177773&action=review Looks like Darin's comments have been addressed. > Source/WebCore/plugins/PlugInOriginHash.cpp:39 > +static inline void addCaseFoldedCharacters(StringHasher &hasher, const String& string) Misplaced & here. > Source/WebCore/plugins/PlugInOriginHash.cpp:49 > + if (!plugInElement->document()->page()) > + return 0; That's quite likely to cause collisions! Perhaps we should require the document to be in page instead of silently returning a bad hash? > Source/WebCore/plugins/PlugInOriginHash.cpp:59 > + LOG(Plugins, "Hash: %s %s %s", plugInElement->document()->page()->mainFrame()->document()->baseURL().host().utf8().data(), plugInURL.host().utf8().data(), plugInElement->serviceType().utf8().data()); Do you still need this? It looks like it will be too chatty with Plugins channel enabled. > Source/WebCore/plugins/PlugInOriginHash.h:35 > + static unsigned hash(HTMLPlugInImageElement* plugInElement, const KURL& plugInURL); Argument names seem unnecessary here.
Comment on attachment 177773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177773&action=review Thanks for reviewing! >> Source/WebCore/plugins/PlugInOriginHash.cpp:39 >> +static inline void addCaseFoldedCharacters(StringHasher &hasher, const String& string) > > Misplaced & here. Fixed. >> Source/WebCore/plugins/PlugInOriginHash.cpp:49 >> + return 0; > > That's quite likely to cause collisions! > > Perhaps we should require the document to be in page instead of silently returning a bad hash? I can convert this to an assert. >> Source/WebCore/plugins/PlugInOriginHash.cpp:59 >> + LOG(Plugins, "Hash: %s %s %s", plugInElement->document()->page()->mainFrame()->document()->baseURL().host().utf8().data(), plugInURL.host().utf8().data(), plugInElement->serviceType().utf8().data()); > > Do you still need this? It looks like it will be too chatty with Plugins channel enabled. I'd like to keep this in for a little while. It will only appear for ports that use plug-in snapshotting, anyway. >> Source/WebCore/plugins/PlugInOriginHash.h:35 >> + static unsigned hash(HTMLPlugInImageElement* plugInElement, const KURL& plugInURL); > > Argument names seem unnecessary here. Removed.
Committed r136932: <http://trac.webkit.org/changeset/136932>
And build fix r136933.