Bug 131758

Summary: Session-aware plugin autostart data
Product: WebKit Reporter: Martin Hock <mhock>
Component: WebKit2Assignee: Martin Hock <mhock>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dino, esprehn+autocc, gyuyoung.kim, jeffm, jonlee, roger_fong
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
copyright
none
address comments + fix bugs
none
style
none
rebase
none
rebase
none
address comments + fix bug
ap: review+, ap: commit-queue-
fix braces none

Description Martin Hock 2014-04-16 13:47:19 PDT
Add WK2 API for plugin autostart data for sessions.
Comment 1 Martin Hock 2014-04-16 13:48:52 PDT
<rdar://problem/15906540>
Comment 2 Martin Hock 2014-04-16 14:14:26 PDT
Created attachment 229479 [details]
patch
Comment 3 Martin Hock 2014-04-29 14:01:32 PDT
Created attachment 230412 [details]
patch
Comment 4 Martin Hock 2014-04-29 14:20:48 PDT
Created attachment 230416 [details]
copyright
Comment 5 Darin Adler 2014-04-29 16:00:47 PDT
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.
Comment 6 Martin Hock 2014-04-30 16:54:02 PDT
Created attachment 230537 [details]
address comments + fix bugs
Comment 7 Martin Hock 2014-04-30 16:55:22 PDT
Created attachment 230538 [details]
style
Comment 8 Martin Hock 2014-04-30 17:07:39 PDT
(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.
Comment 9 Martin Hock 2014-04-30 17:17:38 PDT
Created attachment 230542 [details]
rebase
Comment 10 Martin Hock 2014-05-01 10:13:04 PDT
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.
Comment 11 Martin Hock 2014-05-01 10:16:38 PDT
Created attachment 230587 [details]
rebase
Comment 12 Alexey Proskuryakov 2014-05-01 11:46:57 PDT
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?
Comment 13 Martin Hock 2014-05-01 18:10:46 PDT
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!
Comment 14 WebKit Commit Bot 2014-05-01 18:11:58 PDT
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.
Comment 15 Alexey Proskuryakov 2014-05-01 18:18:15 PDT
Comment on attachment 230637 [details]
address comments + fix bug

I agree with the bot, please add the braces.
Comment 16 Martin Hock 2014-05-01 19:20:35 PDT
Created attachment 230643 [details]
fix braces

I actually fixed the braces but forgot to git add the file :( oops!
Comment 17 Alexey Proskuryakov 2014-05-02 15:19:21 PDT
Can this patch be landed now?
Comment 18 Martin Hock 2014-05-05 09:49:58 PDT
Comment on attachment 230643 [details]
fix braces

Clearing flags on attachment: 230643

Committed r168290: <http://trac.webkit.org/changeset/168290>
Comment 19 Martin Hock 2014-05-05 09:50:05 PDT
All reviewed patches have been landed.  Closing bug.