Bug 131758 – Session-aware plugin autostart data

• Component: WebKit2
• Status: RESOLVED FIXED

[Martin Hock]

Add WK2 API for plugin autostart data for sessions.

[Martin Hock]

<rdar://problem/15906540>

[Martin Hock]

Created attachment 229479 [details]
patch

[Martin Hock]

Created attachment 230412 [details]
patch

[Martin Hock]

Created attachment 230416 [details]
copyright

[Darin Adler]

Comment on attachment 230416 [details]
copyright

View in context: https://bugs.webkit.org/attachment.cgi?id=230416&action=review

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:2
> + * Copyright (C) 2012 - 2014 Apple Inc. All rights reserved.

No spaces around the "-".

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:56
> +    if ((m_hashToOriginMap.contains(session) && m_hashToOriginMap.find(session)->value.contains(plugInOriginHash)) || m_hashToOriginMap.find(SessionID::defaultSessionID())->value.contains(plugInOriginHash))

It’s not good to do the same hash table lookup multiple times here as we do with contains, find and then add below. The design of add is supposed to allow using it in a way that avoids the multiple hash table lookups.

Also, find(x)->value is the same thing as get(x), so we should probably use get.

It’s really worthwhile to structure the hash table accesses so we don’t hash the same value more than once.

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:73
> +    if (!m_autoStartTable.contains(SessionID::defaultSessionID()))
> +        return copyMap;

Same problem with multiple hash table lookups. The find call is designed so you can find and then find again.

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:77
> +    AutoStartTable::const_iterator end = table.end();
> +    for (AutoStartTable::const_iterator it = table.begin(); it != end; ++it) {

This is much better written as a C++ for loop:

    for (auto& x : table)

Not sure what to name "x".

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:89
> +    if (!m_autoStartTable.contains(SessionID::defaultSessionID()))
> +        return ImmutableDictionary::create(std::move(map));

And again.

[Martin Hock]

Created attachment 230537 [details]
address comments + fix bugs

[Martin Hock]

Created attachment 230538 [details]
style

[Martin Hock]

(In reply to comment #5)

Thanks for your comments!

> (From update of attachment 230416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230416&action=review
> 
> > Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:2
> > + * Copyright (C) 2012 - 2014 Apple Inc. All rights reserved.
> 
> No spaces around the "-".

Done.

> > Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:56
> > +    if ((m_hashToOriginMap.contains(session) && m_hashToOriginMap.find(session)->value.contains(plugInOriginHash)) || m_hashToOriginMap.find(SessionID::defaultSessionID())->value.contains(plugInOriginHash))
> 
> It’s not good to do the same hash table lookup multiple times here as we do with contains, find and then add below. The design of add is supposed to allow using it in a way that avoids the multiple hash table lookups.

I made some changes to not check for defaultSessionID because I make sure it's there. There is one place where I do have to check twice:

+void PlugInAutoStartProvider::addAutoStartOriginHash(const String& pageOrigin, unsigned plugInOriginHash, SessionID sessionID)
 {
-    if (m_hashToOriginMap.contains(plugInOriginHash))
+    const auto& sessionIterator = m_hashToOriginMap.find(sessionID);
+    if ((sessionIterator != m_hashToOriginMap.end() && sessionIterator->value.contains(plugInOriginHash)) || m_hashToOriginMap.get(SessionID::defaultSessionID()).contains(plugInOriginHash))
         return;

The reason I have to check sessionIterator->value.contains(plugInOriginHash) here instead of doing an add is that we may also find the result in the other location (under defaultSessionID).

I considered the possibility of having a handle into a hash table that would make subsequent insertion faster, but after consulting with Anders, it seems like more trouble than it's worth. Hashing a sessionID should be fast because we just return the ID itself (some modulus must be going on too, though).

> Also, find(x)->value is the same thing as get(x), so we should probably use get.

Thanks for pointing that out, I had the mistaken impression that it could expensively copy sometimes. Fixed.

> It’s really worthwhile to structure the hash table accesses so we don’t hash the same value more than once.

Tried to fix this as best I could.

> > Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:73
> > +    if (!m_autoStartTable.contains(SessionID::defaultSessionID()))
> > +        return copyMap;
> 
> Same problem with multiple hash table lookups. The find call is designed so you can find and then find again.

Fixed.

> > Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:77
> > +    AutoStartTable::const_iterator end = table.end();
> > +    for (AutoStartTable::const_iterator it = table.begin(); it != end; ++it) {
> 
> This is much better written as a C++ for loop:
> 
>     for (auto& x : table)
> 
> Not sure what to name "x".

I tried to give a somewhat descriptive name modeled after the underlying key/value type. Another possibility would be to give the type name instead of auto and use a generic variable name.
> 
> > Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:89
> > +    if (!m_autoStartTable.contains(SessionID::defaultSessionID()))
> > +        return ImmutableDictionary::create(std::move(map));
> 
> And again.

Done, thanks. I also made similar changes to WebProcess.

There is probably some common code that can be pulled out, though I'm not sure where it should be put.

[Martin Hock]

Created attachment 230542 [details]
rebase

[Martin Hock]

Comment on attachment 230542 [details]
rebase

I thought there was something wrong with this patch on mac, but I am pretty sure it was fixed by http://trac.webkit.org/changeset/168093.

[Martin Hock]

Created attachment 230587 [details]
rebase

[Alexey Proskuryakov]

Comment on attachment 230542 [details]
rebase

View in context: https://bugs.webkit.org/attachment.cgi?id=230542&action=review

Looks good to me overall, with some comments to address.

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:76
> +    for (const auto& keyOriginHash : m_autoStartTable.get(SessionID::defaultSessionID())) {
> +        for (const auto& originHash : keyOriginHash.value)
> +            copyMap.set(originHash.key, originHash.value);

This function produces a map that's sent to newly created WebProcesses, and it only goes over defaultSessionID.

This patch adds code to propagate changes for all sessions to all processes live, but not when a new process is created.

> Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:179
>      m_context->client().plugInAutoStartOriginHashesChanged(m_context);

Client doesn't know about non-default sessions (there is simply no way to express those in current API), so there is no way to notify it unless it's the default session that changes.

Longer term, we should extend the API to support sessions.

> Source/WebKit2/WebProcess/WebProcess.h:288
> +    HashMap<WebCore::SessionID, HashMap<unsigned, double>> m_plugInAutoStartOriginHashes;

This is a pre-existing issue, but calling maps "hashes" is so misleading that maybe you can fix it now?

[Martin Hock]

Created attachment 230637 [details]
address comments + fix bug

I noticed a bug in my last patch which Alexey helped me find the root cause of - thanks! The bug was in the implementation of PlugInAutoStartProvider::addAutoStartOriginHash. I'm not convinced that the new implementation is the nicest way to write it, but it does seem to work, at least. I repeat myself in two different branches in order to avoid doing redundant hash lookups. I'd appreciate any opinions on my hash travails!

[WebKit Commit Bot]

Attachment 230637 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Plugins/PlugInAutoStartProvider.cpp:82:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 230637 [details]
address comments + fix bug

I agree with the bot, please add the braces.

[Martin Hock]

Created attachment 230643 [details]
fix braces

I actually fixed the braces but forgot to git add the file :( oops!

[Alexey Proskuryakov]

Can this patch be landed now?

[Martin Hock]

Comment on attachment 230643 [details]
fix braces

Clearing flags on attachment: 230643

Committed r168290: <http://trac.webkit.org/changeset/168290>

[Martin Hock]

All reviewed patches have been landed.  Closing bug.