Make it possible to have 1 PpapiGlobals per thread. Update unit tests.

Make it possible to have 1 PpapiGlobals per thread. Update unit tests.

This allows us to distinguish trackers in the unit tests, instead of all vars/resources going in 1 tracker. This should also allow us to unit-test PPB proxies.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117399

[dmichael (off chromium), 2012-01-03 23:27:44]

This isn't 100% ready for review, but I wanted to get some feedback on the
approach of using TLS in PpapiGlobals in case I need to stop now.

TODO:
1) Figure out why InstancePrivate test fails, fix it.
2) Move lock to PpapiGlobals so this stuff works right for threading tests.
3) ???
4) Profit!

http://codereview.chromium.org/9034035/diff/2001/ppapi/proxy/ppp_instance_pri...
File ppapi/proxy/ppp_instance_private_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/2001/ppapi/proxy/ppp_instance_pri...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:148: EXPECT_EQ(1,
host().var_tracker().GetRefCountForObject(host_var->GetPPVar()));
I'm getting a 2 here and some other surprises below. Just getting started
figuring out why.

http://codereview.chromium.org/9034035/diff/2001/ppapi/proxy/ppp_instance_pri...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:157: EXPECT_EQ(2,
host().var_tracker().GetRefCountForObject(host_pp_var));
Here I get 4

http://codereview.chromium.org/9034035/diff/2001/ppapi/proxy/ppp_instance_pri...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:166: EXPECT_EQ(-1,
host().var_tracker().GetRefCountForObject(host_pp_var));
Here I get 3.

Might be a real bug...  but this is a bit complex & confusing stuff, so it's
likely a mistake on my part at this point. Will investigate.

[brettw, 2012-01-03 23:47:40]

I don't understand what you mean by "distinguish trackers for the unit tests"

[dmichael (off chromium), 2012-01-04 00:00:40]

I mean when you expect a certain var in the host tracker, you talk to the
host test harness and get the host var tracker. Same for checking plugin
side vars or resources. Currently in the unit tests, bars and resources all
get stuck in 1 tracker. The status quo is workable if this idea is too
ugly. But we can't write any proxy unit tests for ppb interfaces with the
status quo.
On Jan 3, 2012 4:47 PM, <brettw@chromium.org> wrote:

> I don't understand what you mean by "distinguish trackers for the unit
> tests"
>
>
http://codereview.chromium.**org/9034035/<http://codereview.chromium.org/9034...
>

[dmichael (off chromium), 2012-01-04 03:41:25]

Now that I'm not typing on a phone, let me clarify. This CL adds the
capability to register a "PpapiGlobals" singleton that applies for a given
thread, using TLS. This capability is *only* for testing purposes. The
thread that's acting like a plugin process gets a "PluginGlobals" and the
thread that's acting like a host process gets a "TestGlobals".

In the "TwoWay" proxy unit tests, we set up a 'remote' thread with which we
communicate via IPC. Currently, we only test PPP interfaces this way, where
the main thread acts as the host and the remote thread works as the plugin.
It's a bit wacky in that the trackers are shared across "host" and
"plugin". We get around this by having HostVarSerializationRules access the
PPB_Var interface instead of using the shared Var/VarTracker stuff
directly, which allows us to stub out the interface. (See the changes to
host_dispatcher.cc and host_var_serialization_rules.cc in this CL for some
cleanup we can do with the proposed approach).

The other thing that we can't do in the face of 1 PpapiGlobals is test PPB
interfaces. This is because the Thunk layer relies on PpapiGlobals to get
interfaces, so if there's only 1, the host side doesn't work. With this
approach, we can write unit tests for the PPB proxies, because each thread
gets its own "Global" with its own interface functions (and trackers).

The proxy unit tests are useful for a few reasons:
1) They're fast.
2) They're easy to debug.
3) They allow us to test backwards compatibility in a nice way (see the
ppp_instance test, for example, though the test has recently been broken).
...and I was just starting to add perf tests. I can do PPP stuff already
(e.g., PPP_Messaging) but it would be nice to be able to test the
performance of PPB interface calls all the way through the proxy,
especially var transmission.

The proxy unit tests are admittedly a bit fragile, and the TLS in
PpapiGlobals is arguably a heinous hack. But I'm feeling like with a little
cleanup, it's worth the trouble.

On Tue, Jan 3, 2012 at 5:00 PM, David Michael <dmichael@chromium.org> wrote:

> I mean when you expect a certain var in the host tracker, you talk to the
> host test harness and get the host var tracker. Same for checking plugin
> side vars or resources. Currently in the unit tests, bars and resources all
> get stuck in 1 tracker. The status quo is workable if this idea is too
> ugly. But we can't write any proxy unit tests for ppb interfaces with the
> status quo.
> On Jan 3, 2012 4:47 PM, <brettw@chromium.org> wrote:
>
>> I don't understand what you mean by "distinguish trackers for the unit
>> tests"
>>
>>
http://codereview.chromium.**org/9034035/<http://codereview.chromium.org/9034...
>>
>

[dmichael (off chromium), 2012-01-05 22:50:51]

brettw: if you have time.
piman: just FYI

http://codereview.chromium.org/9034035/diff/8001/ppapi/proxy/ppp_instance_pri...
File ppapi/proxy/ppp_instance_private_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/8001/ppapi/proxy/ppp_instance_pri...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:99:
PPB_Var_Shared::GetVarInterface1_0()->VarToUtf8,
Just use the real PPB_Var implementation...

http://codereview.chromium.org/9034035/diff/8001/ppapi/proxy/ppp_instance_pro...
File ppapi/proxy/ppp_instance_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/8001/ppapi/proxy/ppp_instance_pro...
ppapi/proxy/ppp_instance_proxy_unittest.cc:123: PPP_INSTANCE_INTERFACE_1_1));
The "View" part isn't really related, but the test got broken with the recent
change to DidChangeView. This fixes it (and it's kind of nice; this is now a
small/simple test to make sure the proxy does the down-conversion from 1.1 to
1.0 correctly).

[brettw, 2012-01-05 23:41:20]

http://codereview.chromium.org/9034035/diff/8001/ppapi/proxy/ppp_instance_pri...
File ppapi/proxy/ppp_instance_private_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/8001/ppapi/proxy/ppp_instance_pri...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:21: PP_Var
GetPPVarNoAddRef(Var* var) {
Blank line above this.

http://codereview.chromium.org/9034035/diff/8001/ppapi/shared_impl/proxy_lock.cc
File ppapi/shared_impl/proxy_lock.cc (right):

http://codereview.chromium.org/9034035/diff/8001/ppapi/shared_impl/proxy_lock...
ppapi/shared_impl/proxy_lock.cc:16: base::Lock*
lock(PpapiGlobals::Get()->GetProxyLock());
Is this class useful anymore? I probably just would put Acquire and Release on
the globals object.

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/host_var_serial...
File ppapi/proxy/host_var_serialization_rules.cc (right):

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/host_var_serial...
ppapi/proxy/host_var_serialization_rules.cc:38: if (var.type ==
PP_VARTYPE_STRING) {
Can you remove the {} here for consistency?

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
File ppapi/proxy/ppp_instance_private_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:21: PP_Var
GetPPVarNoAddRef(Var* var) {
Blank line before this.

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:28: class NPObjectVar :
public Var {
Is it possible to put this in the anon namespace for this file?

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
File ppapi/proxy/ppp_instance_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
ppapi/proxy/ppp_instance_proxy_unittest.cc:166: ScopedPPResource::PassRef(),
This should be indented 2 more spaces.

[dmichael (off chromium), 2012-01-06 18:18:45]

new snap up.

http://codereview.chromium.org/9034035/diff/8001/ppapi/proxy/ppp_instance_pri...
File ppapi/proxy/ppp_instance_private_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/8001/ppapi/proxy/ppp_instance_pri...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:21: PP_Var
GetPPVarNoAddRef(Var* var) {
On 2012/01/05 23:41:20, brettw wrote:
> Blank line above this.
Done (and moved this function down to the unnamed namespace)

http://codereview.chromium.org/9034035/diff/8001/ppapi/shared_impl/proxy_lock.cc
File ppapi/shared_impl/proxy_lock.cc (right):

http://codereview.chromium.org/9034035/diff/8001/ppapi/shared_impl/proxy_lock...
ppapi/shared_impl/proxy_lock.cc:16: base::Lock*
lock(PpapiGlobals::Get()->GetProxyLock());
On 2012/01/05 23:41:20, brettw wrote:
> Is this class useful anymore? I probably just would put Acquire and Release on
> the globals object. 
I agree, that would be nicer for right now, and I almost did it. But I think
we're going to need access to the naked lock later when we implement blocking
completion callbacks (presumably using condition variables). So I'm leaning
towards leaving just the GetProxyLock there and these convenience classes here.
If you don't want me to leave this "speculative" design choice in, though, just
let me know. Your suggestion is definitely cleaner in the short term.

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/host_var_serial...
File ppapi/proxy/host_var_serialization_rules.cc (right):

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/host_var_serial...
ppapi/proxy/host_var_serialization_rules.cc:38: if (var.type ==
PP_VARTYPE_STRING) {
On 2012/01/05 23:41:20, brettw wrote:
> Can you remove the {} here for consistency?
Done.

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
File ppapi/proxy/ppp_instance_private_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:21: PP_Var
GetPPVarNoAddRef(Var* var) {
On 2012/01/05 23:41:20, brettw wrote:
> Blank line before this.
Done.

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
ppapi/proxy/ppp_instance_private_proxy_unittest.cc:28: class NPObjectVar :
public Var {
On 2012/01/05 23:41:20, brettw wrote:
> Is it possible to put this in the anon namespace for this file?
Unfortunately not; I'm providing a definition for the NPObjectVar that
ppapi/shared_impl/var.h forward declares. It doesn't conflict with anything
since these tests never link in anything from webkit/plugins/ppapi. If that ever
changes (though I think we should avoid that), I can probably just use the real
thing.

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
File ppapi/proxy/ppp_instance_proxy_unittest.cc (right):

http://codereview.chromium.org/9034035/diff/12003/ppapi/proxy/ppp_instance_pr...
ppapi/proxy/ppp_instance_proxy_unittest.cc:166: ScopedPPResource::PassRef(),
On 2012/01/05 23:41:20, brettw wrote:
> This should be indented 2 more spaces.
Done.

[dmichael (off chromium), 2012-01-11 18:29:30]

piman, could you take a look since brettw's not available?

[piman, 2012-01-11 19:23:25]

http://codereview.chromium.org/9034035/diff/31002/ppapi/proxy/plugin_globals.h
File ppapi/proxy/plugin_globals.h (right):

http://codereview.chromium.org/9034035/diff/31002/ppapi/proxy/plugin_globals....
ppapi/proxy/plugin_globals.h:33: return
static_cast<PluginGlobals*>(PpapiGlobals::Get());
Could we add a way to DCHECK that the cast is safe? Right now we rely on the
fact that it's only called on a "plugin" thread (something that is dependent on
each test to set it up correctly), but we have no way of enforcing it's true.

Also, since that's the case, could we always return
static_cast<PluginGlobals*>(PpapiGlobals::Get()); ?

[dmichael (off chromium), 2012-01-12 04:42:23]

http://codereview.chromium.org/9034035/diff/31002/ppapi/proxy/plugin_globals.h
File ppapi/proxy/plugin_globals.h (right):

http://codereview.chromium.org/9034035/diff/31002/ppapi/proxy/plugin_globals....
ppapi/proxy/plugin_globals.h:33: return
static_cast<PluginGlobals*>(PpapiGlobals::Get());
On 2012/01/11 19:23:25, piman wrote:
> Could we add a way to DCHECK that the cast is safe? Right now we rely on the
> fact that it's only called on a "plugin" thread (something that is dependent
on
> each test to set it up correctly), but we have no way of enforcing it's true.
> 
> Also, since that's the case, could we always return
> static_cast<PluginGlobals*>(PpapiGlobals::Get()); ?
Good ideas. Done.

[piman, 2012-01-12 06:36:42]

lgtm

[commit-bot: I haz the power, 2012-01-12 07:00:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/9034035/37002

[commit-bot: I haz the power, 2012-01-12 08:15:42]

Change committed as 117399