COM replaced with RPC for firing events in broker.

COM replaced with RPC for firing events in broker.

BUG=none
TEST=none

[Sigurður Ásgeirsson, 2010-11-05 16:05:59]

Hi Vitaly,

I only skimmed it quickly, but it looks good to me so far. Awesome!

Siggi

http://codereview.chromium.org/4508002/diff/15001/16002
File ceee/ie/broker/broker.gyp (right):

http://codereview.chromium.org/4508002/diff/15001/16002#newcode38
ceee/ie/broker/broker.gyp:38: 'broker_rpc_server.c',
'<(SHARED_INTERMEDIATE_DIR)/broker_rpc_server.c' ?

http://codereview.chromium.org/4508002/diff/15001/16005
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/4508002/diff/15001/16005#newcode61
ceee/ie/broker/broker_rpc_client.cc:61: 
There's no guarantee (aside from name uniqueness) that you'll connect to an
endpoint created by the same user. There's an arcane invocation to ensure that
you're connecting to the expected principal, it's in Keith Brown, I'll look it
up unless you beat me to it.
Add a todo here at minimum.

http://codereview.chromium.org/4508002/diff/15001/16012
File ceee/ie/broker/broker_rpc_utils.cc (right):

http://codereview.chromium.org/4508002/diff/15001/16012#newcode13
ceee/ie/broker/broker_rpc_utils.cc:13: std::wstring end_point =
ceee_module_util::GetBrokerProfileNameForIe();
also need something to separate admin and non-admin users, or a comment if the
profile name suffices.

[Vitaly Buka corp, 2010-11-11 01:31:02]

http://codereview.chromium.org/4508002/diff/15001/ceee/ie/broker/broker.gyp
File ceee/ie/broker/broker.gyp (right):

http://codereview.chromium.org/4508002/diff/15001/ceee/ie/broker/broker.gyp#n...
ceee/ie/broker/broker.gyp:38: 'broker_rpc_server.c',
I already tried. it is silently create projects without this files.

On 2010/11/05 16:05:59, Ruðrugis wrote:
> '<(SHARED_INTERMEDIATE_DIR)/broker_rpc_server.c' ?

http://codereview.chromium.org/4508002/diff/15001/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/4508002/diff/15001/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:61: 
On 2010/11/05 16:05:59, Ruðrugis wrote:
> There's no guarantee (aside from name uniqueness) that you'll connect to an
> endpoint created by the same user. There's an arcane invocation to ensure that
> you're connecting to the expected principal, it's in Keith Brown, I'll look it
> up unless you beat me to it.
> Add a todo here at minimum. 

Done.

http://codereview.chromium.org/4508002/diff/15001/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_utils.cc (right):

http://codereview.chromium.org/4508002/diff/15001/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.cc:13: std::wstring end_point =
ceee_module_util::GetBrokerProfileNameForIe();
On 2010/11/05 16:05:59, Ruðrugis wrote:
> also need something to separate admin and non-admin users, or a comment if the
> profile name suffices. 

Done.

[Sigurður Ásgeirsson, 2010-11-11 19:30:35]

very cool - I have some comments

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker.cc
File ceee/ie/broker/broker.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker.cc#ne...
ceee/ie/broker/broker.cc:24: void CeeeBroker::OnAddConnection(bool first_lock) {
Do we still need this?

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker.gyp
File ceee/ie/broker/broker.gyp (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker.gyp#n...
ceee/ie/broker/broker.gyp:39: 'broker_rpc_server.c',
suggest dropping these intermediate files and adding
 <(SHARED_INTERMEDIATE_DIR)/broker_rpc_server_s.c 
et al as source instead.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:60: DCHECK(!end_point.empty());
this function, and our code in general, should log on error cases. DCHECKs are
only present in debug builds, and should primarily be used to test invariants,
not for checking runtime conditions - with the rare exception of a runtime
condition that you're not sure of, but think is vanishingly unlikely to occur.  
Whenever there's a runtime condition that can fail, we want to be able to enable
logging on the user's system to field-diagnose what went wrong.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:65: // uniqueness) that it will connect to
an endpoint created by the same user.
The missing invocation is RpcBindingSetAuthInfoEx. 
There's a similar missing piece on the server side, where the server ought to
ensure that it's the same user that's running the server and connecting to it.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_l...
File ceee/ie/broker/broker_rpc_lib.idl (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_l...
ceee/ie/broker/broker_rpc_lib.idl:6: // TODO(vitalybuka@google.com): decide if
we need meaningfull version.
meaningfull -> meaningful

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_l...
ceee/ie/broker/broker_rpc_lib.idl:10: interface Broker {
Not that it matters, but perhaps something slightly more specific, like
CeeeBroker?

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:35: RPC_STATUS status =
RpcServerUseProtseqEp(
since use of RPC is so rare, can I ask you to sprinkle this function liberally
with running commentary on the purpose of the various invocations?

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:65: RPC_STATUS status =
RpcMgmtStopServerListening(NULL);
... same here ...

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:77: static long current_context = 0;
I imagine this'd be the place to add the client identity check. Maybe add a todo
here?

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.h (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.h:9: class BrokerRpcServer {
it looks like this class is intended for single-threaded usage? If so, can I ask
you to enforce that usage with some DCHECKs?

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_utils.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.cc:12: const wchar_t kRpcProtocol[] =
L"ncalrpc";
a quick one-line comment on ncalrpc (this is machine local only?)

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.cc:17:
process_utils_win::IsCurrentProcessUacElevated(&running_as_admin);
a quick comment on how and maybe why we construct the endpoint address. It'll
help maintainers.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_utils.h (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.h:7: #include <string>
missing include guards.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/ie.gyp
File ceee/ie/ie.gyp (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/ie.gyp#newcode104
ceee/ie/ie.gyp:104: 'Rpcrt4.lib',
nit: we lower case these :).

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/plugin/toolband/too...
File ceee/ie/plugin/toolband/toolband.gyp (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband.gyp:90: 'Rpcrt4.lib',
ditto.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/plugin/toolband/too...
File ceee/ie/plugin/toolband/toolband_module.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:289: ::CoInitializeEx(0,
COINIT_MULTITHREADED);
I believe this thread and all its associated machinery is redundant with your
change, as we can now simply fire events in-place. The problem this thread was
to solve, is the fact that under some circumstances (when you thread is
processing a sent message), you can't do outgoing COM invocations from the
thread.
You can land this change as-is, with a TODO (and perhaps a bug) to remove this,
or do it as part of this change.

http://codereview.chromium.org/4508002/diff/32018/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.h (right):

http://codereview.chromium.org/4508002/diff/32018/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.h:7: 
missing include guards.

http://codereview.chromium.org/4508002/diff/32018/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_utils.h (right):

http://codereview.chromium.org/4508002/diff/32018/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.h:6: 
missing include guards

[Vitaly Buka corp, 2010-11-11 23:12:09]

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker.cc
File ceee/ie/broker/broker.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker.cc#ne...
ceee/ie/broker/broker.cc:24: void CeeeBroker::OnAddConnection(bool first_lock) {
On 2010/11/11 19:30:35, Ruðrugis wrote:
> Do we still need this?

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker.gyp
File ceee/ie/broker/broker.gyp (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker.gyp#n...
ceee/ie/broker/broker.gyp:39: 'broker_rpc_server.c',
On 2010/11/11 19:30:35, Ruðrugis wrote:
> suggest dropping these intermediate files and adding
>  <(SHARED_INTERMEDIATE_DIR)/broker_rpc_server_s.c 
> et al as source instead.

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_c...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:60: DCHECK(!end_point.empty());
There is nothing to log in this place. It is just checks because I do
end_point[0] in following code.

On 2010/11/11 19:30:35, Ruðrugis wrote:
> this function, and our code in general, should log on error cases. DCHECKs are
> only present in debug builds, and should primarily be used to test invariants,
> not for checking runtime conditions - with the rare exception of a runtime
> condition that you're not sure of, but think is vanishingly unlikely to occur.
 
> Whenever there's a runtime condition that can fail, we want to be able to
enable
> logging on the user's system to field-diagnose what went wrong.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_c...
ceee/ie/broker/broker_rpc_client.cc:65: // uniqueness) that it will connect to
an endpoint created by the same user.
On 2010/11/11 19:30:35, Ruðrugis wrote:
> The missing invocation is RpcBindingSetAuthInfoEx. 
> There's a similar missing piece on the server side, where the server ought to
> ensure that it's the same user that's running the server and connecting to it.

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_l...
File ceee/ie/broker/broker_rpc_lib.idl (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_l...
ceee/ie/broker/broker_rpc_lib.idl:6: // TODO(vitalybuka@google.com): decide if
we need meaningfull version.
On 2010/11/11 19:30:35, Ruðrugis wrote:
> meaningfull -> meaningful

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_l...
ceee/ie/broker/broker_rpc_lib.idl:10: interface Broker {
On 2010/11/11 19:30:35, Ruðrugis wrote:
> Not that it matters, but perhaps something slightly more specific, like
> CeeeBroker?

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:35: RPC_STATUS status =
RpcServerUseProtseqEp(
On 2010/11/11 19:30:35, Ruðrugis wrote:
> since use of RPC is so rare, can I ask you to sprinkle this function liberally
> with running commentary on the purpose of the various invocations?

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:65: RPC_STATUS status =
RpcMgmtStopServerListening(NULL);
On 2010/11/11 19:30:35, Ruðrugis wrote:
> ... same here ... 

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:77: static long current_context = 0;
On 2010/11/11 19:30:35, Ruðrugis wrote:
> I imagine this'd be the place to add the client identity check. Maybe add a
todo
> here?

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.h (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.h:9: class BrokerRpcServer {
On 2010/11/11 19:30:35, Ruðrugis wrote:
> it looks like this class is intended for single-threaded usage? If so, can I
ask
> you to enforce that usage with some DCHECKs?

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_utils.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.cc:12: const wchar_t kRpcProtocol[] =
L"ncalrpc";
On 2010/11/11 19:30:35, Ruðrugis wrote:
> a quick one-line comment on ncalrpc (this is machine local only?)

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.cc:12: const wchar_t kRpcProtocol[] =
L"ncalrpc";
On 2010/11/11 19:30:35, Ruðrugis wrote:
> a quick one-line comment on ncalrpc (this is machine local only?)

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.cc:17:
process_utils_win::IsCurrentProcessUacElevated(&running_as_admin);
On 2010/11/11 19:30:35, Ruðrugis wrote:
> a quick comment on how and maybe why we construct the endpoint address. It'll
> help maintainers.

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
File ceee/ie/broker/broker_rpc_utils.h (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/broker/broker_rpc_u...
ceee/ie/broker/broker_rpc_utils.h:7: #include <string>
Sorry. I used to fix this after lint warnings.
On 2010/11/11 19:30:35, Ruðrugis wrote:
> missing include guards.

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/ie.gyp
File ceee/ie/ie.gyp (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/ie.gyp#newcode104
ceee/ie/ie.gyp:104: 'Rpcrt4.lib',
On 2010/11/11 19:30:35, Ruðrugis wrote:
> nit: we lower case these :).

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/plugin/toolband/too...
File ceee/ie/plugin/toolband/toolband.gyp (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband.gyp:90: 'Rpcrt4.lib',
On 2010/11/11 19:30:35, Ruðrugis wrote:
> ditto.

Done.

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/plugin/toolband/too...
File ceee/ie/plugin/toolband/toolband_module.cc (right):

http://codereview.chromium.org/4508002/diff/37002/ceee/ie/plugin/toolband/too...
ceee/ie/plugin/toolband/toolband_module.cc:289: ::CoInitializeEx(0,
COINIT_MULTITHREADED);
yes. I planed to change this in separate CL.
However starting broker in background thread may be required for IE9 performance
checks.

On 2010/11/11 19:30:35, Ruðrugis wrote:
> I believe this thread and all its associated machinery is redundant with your
> change, as we can now simply fire events in-place. The problem this thread was
> to solve, is the fact that under some circumstances (when you thread is
> processing a sent message), you can't do outgoing COM invocations from the
> thread.
> You can land this change as-is, with a TODO (and perhaps a bug) to remove
this,
> or do it as part of this change.

[Sigurður Ásgeirsson, 2010-11-12 14:40:24]

Awesome, looks good to me aside from a couple of last nits.

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_modul...
File ceee/ie/broker/broker_module.cc (right):

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_modul...
ceee/ie/broker/broker_module.cc:164: rpc_server_.Start();
Return error from the function on failure, as the broker's no use without the
RPC server.

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:18: current_thread_(GetCurrentThreadId()) {
ceee likes to prefix win32 API invocations with ::, e.g. ::GetCurrentThreadId(),
same for the Rpc invocations.

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:55: "Failed to register RPC interface.
RPC_STATUS=0x" << std::hex << status;
As RPC error codes are windows errors, you can log them with com::LogWe(status),
see ceee/common/com_utils.h. 
This generates human-readable error reports

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:76: // Stop server listening for PRC.
PRC->RPC :)

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:80: // Wait server stops listening threads.
Wait while?

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:115: DCHECK(context != NULL);
Hold on, so there's currently no association to a given executor registration?
I think we should fix that, but in a separate change.

[Vitaly Buka corp, 2010-11-12 20:58:48]

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_modul...
File ceee/ie/broker/broker_module.cc (right):

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_modul...
ceee/ie/broker/broker_module.cc:164: rpc_server_.Start();
On 2010/11/12 14:40:24, Ruðrugis wrote:
> Return error from the function on failure, as the broker's no use without the
> RPC server. 

Done.

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:18: current_thread_(GetCurrentThreadId()) {
On 2010/11/12 14:40:24, Ruðrugis wrote:
> ceee likes to prefix win32 API invocations with ::, e.g.
::GetCurrentThreadId(),
> same for the Rpc invocations.

Done.

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:55: "Failed to register RPC interface.
RPC_STATUS=0x" << std::hex << status;
On 2010/11/12 14:40:24, Ruðrugis wrote:
> As RPC error codes are windows errors, you can log them with
com::LogWe(status),
> see ceee/common/com_utils.h. 
> This generates human-readable error reports

Done.

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:76: // Stop server listening for PRC.
On 2010/11/12 14:40:24, Ruðrugis wrote:
> PRC->RPC :)

Done.

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:80: // Wait server stops listening threads.
On 2010/11/12 14:40:24, Ruðrugis wrote:
> Wait while?

Done.

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:115: DCHECK(context != NULL);
Please explain what do you mean.
is this about unregistering all executors corresponding to this context?

On 2010/11/12 14:40:24, Ruðrugis wrote:
> Hold on, so there's currently no association to a given executor registration?
> I think we should fix that, but in a separate change.

[Sigurður Ásgeirsson, 2010-11-12 21:10:14]

LGTM, please submit after addressing comments (provided you're a chrome
committer).

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/4508002/diff/90002/ceee/ie/broker/broker_rpc_s...
ceee/ie/broker/broker_rpc_server.cc:115: DCHECK(context != NULL);
On 2010/11/12 20:58:48, Vitaly Buka wrote:
> Please explain what do you mean.
> is this about unregistering all executors corresponding to this context?
> 
> On 2010/11/12 14:40:24, Ruðrugis wrote:
> > Hold on, so there's currently no association to a given executor
registration?
> > I think we should fix that, but in a separate change.
> 

Well, when one of the RPC clients winks out of existence, we get a pretty
immediate notification of its death. The executor that was registered as
associated with that same client (tab/bho/thread) is left dangling, and we'll
never clean it up AFAICT. 
It would make sense to tie the two together, so we can clean up the executor
registration that goes with a particular RPC connection.
We should do this in another change, however.

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_modu...
File ceee/ie/broker/broker_module.cc (right):

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_modu...
ceee/ie/broker/broker_module.cc:165: return RPC_E_FAULT;
you need to return an HRESULT error, this is a windows error message, which is
an HRESULT success. 
You can return e.g. 
HRESULT_FROM_WIN32(RPC_E_FAULT)

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
ceee/ie/broker/broker_rpc_client.cc:11: #include "broker_rpc_lib.h"
you can quench the lint error here by appending 

  // NOLINT

we like our files lint free if at all possible.

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
ceee/ie/broker/broker_rpc_server.cc:100: static long current_context = 0;
Use AtomicSequenceNumber, see
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/atomic_s...


It must either be a singleton or a static/global to avoid the initialization
race for local statics.

[Vitaly Buka corp, 2010-11-12 21:28:11]

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_modu...
File ceee/ie/broker/broker_module.cc (right):

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_modu...
ceee/ie/broker/broker_module.cc:165: return RPC_E_FAULT;
#define RPC_E_FAULT                      _HRESULT_TYPEDEF_(0x80010104L)

On 2010/11/12 21:10:15, Ruðrugis wrote:
> you need to return an HRESULT error, this is a windows error message, which is
> an HRESULT success. 
> You can return e.g. 
> HRESULT_FROM_WIN32(RPC_E_FAULT)

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
File ceee/ie/broker/broker_rpc_client.cc (right):

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
ceee/ie/broker/broker_rpc_client.cc:11: #include "broker_rpc_lib.h"
On 2010/11/12 21:10:15, Ruðrugis wrote:
> you can quench the lint error here by appending 
> 
>   // NOLINT
> 
> we like our files lint free if at all possible.

Done.

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
File ceee/ie/broker/broker_rpc_server.cc (right):

http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
ceee/ie/broker/broker_rpc_server.cc:100: static long current_context = 0;
On 2010/11/12 21:10:15, Ruðrugis wrote:
> Use AtomicSequenceNumber, see
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/atomic_s...
> 
> 
> It must either be a singleton or a static/global to avoid the initialization
> race for local statics.

Done.

[Sigurður Ásgeirsson, 2010-11-15 16:08:16]

Submitted as http://src.chromium.org/viewvc/chrome?view=rev&revision=66121.
The build was broken, but not by this change, so it'll probably stick.

On Fri, Nov 12, 2010 at 4:28 PM, <vitalybuka@google.com> wrote:

>
>
>
http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_modu...
> File ceee/ie/broker/broker_module.cc (right):
>
>
>
http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_modu...
> ceee/ie/broker/broker_module.cc:165: return RPC_E_FAULT;
> #define RPC_E_FAULT                      _HRESULT_TYPEDEF_(0x80010104L)
>
>
> On 2010/11/12 21:10:15, Ruðrugis wrote:
>
>> you need to return an HRESULT error, this is a windows error message,
>>
> which is
>
>> an HRESULT success.
>> You can return e.g.
>> HRESULT_FROM_WIN32(RPC_E_FAULT)
>>
>
>
>
http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
> File ceee/ie/broker/broker_rpc_client.cc (right):
>
>
>
http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
> ceee/ie/broker/broker_rpc_client.cc:11: #include "broker_rpc_lib.h"
> On 2010/11/12 21:10:15, Ruðrugis wrote:
>
>> you can quench the lint error here by appending
>>
>
>    // NOLINT
>>
>
>  we like our files lint free if at all possible.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
> File ceee/ie/broker/broker_rpc_server.cc (right):
>
>
>
http://codereview.chromium.org/4508002/diff/105001/ceee/ie/broker/broker_rpc_...
> ceee/ie/broker/broker_rpc_server.cc:100: static long current_context =
> 0;
> On 2010/11/12 21:10:15, Ruðrugis wrote:
>
>> Use AtomicSequenceNumber, see
>>
>
>
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/atomic_s...
>
>
>
>  It must either be a singleton or a static/global to avoid the
>>
> initialization
>
>> race for local statics.
>>
>
> Done.
>
>
> http://codereview.chromium.org/4508002/
>