Set up a interposing library for Carbon calls made by plugins.

Set up a interposing library for Carbon calls made by plugins.

This gives us a library that's inserted into plugin process via DYLD_INSERT_LIBRARIES to intercept Carbon calls, and moves the window/process activation handling into that library (based on Carbon window activation/deactivation calls, rather than polling the front window). Over time we'll interpose more, but this gives us the foundation.

This fixes both the "window loses focus when loading a page with plugins" and "can't click on YouTube controls" bugs.

BUG=18203, 18553
TEST=Clicking on Flash plugins should work much more reliably, opening a page with a plugin shouldn't cause the window to lose focus.

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

[Mark Mentovai, 2009-08-06 22:43:48]

I didn't look closely at the bits in webkit.

You need to update build/mac/dump_app_syms so that we save symbols from your
dylib.

http://codereview.chromium.org/164100/diff/1/2
File chrome/browser/plugin_carbon_interpose_mac.cc (right):

http://codereview.chromium.org/164100/diff/1/2#newcode5
Line 5: #import <Carbon/Carbon.h>
#include

http://codereview.chromium.org/164100/diff/1/2#newcode12
Line 12: ProcessSerialNumber g_saved_front_process = { 0, 0 };
No reason for this to be global.  Make it static or put it in an anonymous
namespace.  Static is probably best because you're using static for functions in
this file...

http://codereview.chromium.org/164100/diff/1/2#newcode56
Line 56: FakePluginWindowTracker::SharedInstance()->GetDelegateForFakeWindow(
Fix the indentation or do something else here so that it's clear that this is a
continuation and the result is being assigned into |delegate|.

http://codereview.chromium.org/164100/diff/1/2#newcode91
Line 91: __attribute__((used)) static const interpose_substitution
substitutions[] \
Blank line before this.  It looks like it's part of the macro definition, but
it's not.

The \ at the end of this line is not needed.

http://codereview.chromium.org/164100/diff/1/3
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/164100/diff/1/3#newcode416
Line 416: if (!PathService::Get(base::FILE_EXE, &binary_path))
Actually, you don't need to do this.  You can put @executable_path into
DYLD_INSERT_LIBRARIES.

When we have a separate plugin.app, we'll want your dylib to live within that
bundle, so the dirname of FILE_EXE in the browser process won't be right anyway.
 Another win for @executable_path.

http://codereview.chromium.org/164100/diff/1/3#newcode421
Line 421: "DYLD_INSERT_LIBRARIES", interpose_lib.value().c_str()));
What if DYLD_INSERT_LIBRARIES is already set?

http://codereview.chromium.org/164100/diff/1/5
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/164100/diff/1/5#newcode42
Line 42: unsetenv("DYLD_INSERT_LIBRARIES");
What if you fix the issue I raised in the other file about DYLD_INSERT_LIBRARIES
already being set?  We'd only want to remove your interpose lib here.

[stuartmorgan, 2009-08-07 00:51:07]

Addressed everything but the build/mac/dump_app_syms part.

Mark, I'll ping you tomorrow about how we should handle dump_app_syms, since it
seems like having *three* copy-and-pasted copies of the same block of code is
getting to be a bit much.

http://codereview.chromium.org/164100/diff/1/2
File chrome/browser/plugin_carbon_interpose_mac.cc (right):

http://codereview.chromium.org/164100/diff/1/2#newcode5
Line 5: #import <Carbon/Carbon.h>
On 2009/08/06 22:43:48, Mark Mentovai wrote:
> #include

Done.

http://codereview.chromium.org/164100/diff/1/2#newcode12
Line 12: ProcessSerialNumber g_saved_front_process = { 0, 0 };
On 2009/08/06 22:43:48, Mark Mentovai wrote:
> No reason for this to be global.  Make it static or put it in an anonymous
> namespace.  Static is probably best because you're using static for functions
in
> this file...

Done.

http://codereview.chromium.org/164100/diff/1/2#newcode56
Line 56: FakePluginWindowTracker::SharedInstance()->GetDelegateForFakeWindow(
On 2009/08/06 22:43:48, Mark Mentovai wrote:
> Fix the indentation or do something else here so that it's clear that this is
a
> continuation and the result is being assigned into |delegate|.

Done.

http://codereview.chromium.org/164100/diff/1/2#newcode91
Line 91: __attribute__((used)) static const interpose_substitution
substitutions[] \
On 2009/08/06 22:43:48, Mark Mentovai wrote:
> Blank line before this.  It looks like it's part of the macro definition, but
> it's not.
> 
> The \ at the end of this line is not needed.

Done.

http://codereview.chromium.org/164100/diff/1/3
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/164100/diff/1/3#newcode416
Line 416: if (!PathService::Get(base::FILE_EXE, &binary_path))
On 2009/08/06 22:43:48, Mark Mentovai wrote:
> Actually, you don't need to do this.  You can put @executable_path into
> DYLD_INSERT_LIBRARIES.
> 
> When we have a separate plugin.app, we'll want your dylib to live within that
> bundle, so the dirname of FILE_EXE in the browser process won't be right
anyway.
>  Another win for @executable_path.

Done.

http://codereview.chromium.org/164100/diff/1/3#newcode421
Line 421: "DYLD_INSERT_LIBRARIES", interpose_lib.value().c_str()));
On 2009/08/06 22:43:48, Mark Mentovai wrote:
> What if DYLD_INSERT_LIBRARIES is already set?

I was hoping we could just not care and punt, but Done.

http://codereview.chromium.org/164100/diff/1/5
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/164100/diff/1/5#newcode42
Line 42: unsetenv("DYLD_INSERT_LIBRARIES");
On 2009/08/06 22:43:48, Mark Mentovai wrote:
> What if you fix the issue I raised in the other file about
DYLD_INSERT_LIBRARIES
> already being set?  We'd only want to remove your interpose lib here.

Done.

[Mark Mentovai, 2009-08-07 01:29:04]

LGTM with these changes, but I have a feeling you might want another pass just
because there are a bunch of comments?

This time I've read the bits in webkit too (obviously).

http://codereview.chromium.org/164100/diff/33/1006
File chrome/browser/plugin_carbon_interpose_constants_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1006#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
...not in this file.

http://codereview.chromium.org/164100/diff/33/1007
File chrome/browser/plugin_carbon_interpose_constants_mac.h (right):

http://codereview.chromium.org/164100/diff/33/1007#newcode7
Line 7: extern const char kDYLDInsertLibrariesKey[];
Minor style thing: no indentation inside a namespace.  Be sure to provide blank
lines around the contents.  Same in the .cc file.

http://codereview.chromium.org/164100/diff/33/1007#newcode9
Line 9: }  // namespace plugin_interpose_strings
#include guards go in this file...

http://codereview.chromium.org/164100/diff/33/1008
File chrome/browser/plugin_carbon_interpose_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1008#newcode89
Line 89: { (const void*)ChromePlugin##function, (const void*)function }
One more thing: prefer c++<style>(casts).

http://codereview.chromium.org/164100/diff/33/1009
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/164100/diff/33/1009#newcode423
Line 423: interpose_list.append(":");
I was expecting to append kInterposeLibraryPath to the variable instead of
prepending it.  When DYLD_INSERT_LIBRARIES is used for things like GuardMalloc,
we'd want GuardMalloc to be the allocator in the dylib's code too, and I'm not
positive that it would be if we prepended our dylib.

http://codereview.chromium.org/164100/diff/33/1011
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/164100/diff/33/1011#newcode30
Line 30: #if defined(OS_MACOSX)
This can be an #elif on the group above, which is already #if win/#elif linux.

http://codereview.chromium.org/164100/diff/33/1011#newcode37
Line 37: static void TrimInterposeEnvironment() {
I think this can be made much less complex.  If we always set-or-prepend in
plugin_process_host, this function can do:

if getenv(DYLD_INSERT_LIBRARIES) == kInterposeLibraryPath
  unsetenv(DYLD_INSERT_LIBRARIES)
else if substr(getenv(DYLD_INSERT_LIBRARIES), 0, strlen(kInterposeLibraryPath) +
1) == kInterposeLibraryPath + ":"
  setenv(DYLD_INSERT_LIBRARIES, substr(getenv(DYLD_INSERT_LIBRARIES,
strlen(kInterposeLibraryPath + 1))
else
  NOTREACHED()

as long as you leave behind a comment in this function and in the other one that
indicates that they cooperate.  That's a good idea anyway.

Also, obviously, change this up as needed if we're always set-or-append instead
of set-or-prepend as I suggested in the other file.

http://codereview.chromium.org/164100/diff/33/1011#newcode67
Line 67: #endif
A long #ifdef like this deserves an // OS_MACOSX comment at the close.

http://codereview.chromium.org/164100/diff/33/1012
File webkit/glue/plugins/fake_plugin_window_tracker_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1012#newcode8
Line 8: window_to_delegate_map_ = CFDictionaryCreateMutable(kCFAllocatorDefault,
If using a scoped_cftyperef, do this in an initializer list.

If you're not using a scoped_cftyperef, it makes no difference.

http://codereview.chromium.org/164100/diff/33/1012#newcode13
Line 13: CFRelease(window_to_delegate_map_);
Do we need to assert that the map was empty?  Dispose any of the windows from
the map?  If so, maybe we do need this destructor after all.  (But you still
might want to use a scoped_cftyperef?)

http://codereview.chromium.org/164100/diff/33/1012#newcode17
Line 17: static FakePluginWindowTracker* tracker = NULL;
You can simplify this, since the compiler's effectively going to write the |if|
for you anyway:

{
  static FakePluginWindowTracker* tracker = new FakePluginWindowTracker();
  return tracker;
}

http://codereview.chromium.org/164100/diff/33/1013
File webkit/glue/plugins/fake_plugin_window_tracker_mac.h (right):

http://codereview.chromium.org/164100/diff/33/1013#newcode14
Line 14: // Serves as a bridge between password delegate instances, which know
about
Make the map a scoped_cftyperef and eliminate the explicit Gozer.

http://codereview.chromium.org/164100/diff/33/1013#newcode17
Line 17: // the state of the world.
This sentence is long and I'm not tired.  Future readers may also be tired. 
Split it up into two sentences just to be safe.

"Serves as a bridge between passwordorplugin delegate instances and the Carbon
interposing library.  Blahblah about the delegates, and more blahblah about the
interposing library and WindowRefs."

http://codereview.chromium.org/164100/diff/33/1013#newcode21
Line 21: ~FakePluginWindowTracker();
Hold the dictionary in a scoped_cftyperef so that you don't need this Gozer at
all.

[TVL, 2009-08-07 03:38:58]

http://codereview.chromium.org/164100/diff/33/1006
File chrome/browser/plugin_carbon_interpose_constants_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1006#newcode12
Line 12: const char kInterposeLibraryPath[] =
drop the indent again

http://codereview.chromium.org/164100/diff/33/1008
File chrome/browser/plugin_carbon_interpose_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1008#newcode21
Line 21: SameProcess(&this_process, &front_process, &matched);
Do you need to check any of the error results here?

http://codereview.chromium.org/164100/diff/33/1010
File chrome/chrome.gyp (right):

http://codereview.chromium.org/164100/diff/33/1010#newcode4459
Line 4459: 'chrome_dll',
why does this depend on chrome_dll? is this for the window tracker?

http://codereview.chromium.org/164100/diff/33/1012
File webkit/glue/plugins/fake_plugin_window_tracker_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1012#newcode21
Line 21: return tracker;
is there any threading worry to make sure only one exists?

http://codereview.chromium.org/164100/diff/33/1012#newcode51
Line 51: CFDictionaryRemoveValue(window_to_delegate_map_, delegate);
do you need to double check that window and delegate were related?

why wouldn't they pass window?  and if they don't what happens since the windows
doesn't get disposed of?

[stuartmorgan, 2009-08-07 18:37:01]

http://codereview.chromium.org/164100/diff/33/1006
File chrome/browser/plugin_carbon_interpose_constants_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1006#newcode12
Line 12: const char kInterposeLibraryPath[] =
On 2009/08/07 03:38:58, TVL wrote:
> drop the indent again

Done.

http://codereview.chromium.org/164100/diff/33/1007
File chrome/browser/plugin_carbon_interpose_constants_mac.h (right):

http://codereview.chromium.org/164100/diff/33/1007#newcode7
Line 7: extern const char kDYLDInsertLibrariesKey[];
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> Minor style thing: no indentation inside a namespace.  Be sure to provide
blank
> lines around the contents.  Same in the .cc file.

Done.

http://codereview.chromium.org/164100/diff/33/1007#newcode9
Line 9: }  // namespace plugin_interpose_strings
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> #include guards go in this file...

Whoops! Done.

http://codereview.chromium.org/164100/diff/33/1008
File chrome/browser/plugin_carbon_interpose_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1008#newcode21
Line 21: SameProcess(&this_process, &front_process, &matched);
On 2009/08/07 03:38:58, TVL wrote:
> Do you need to check any of the error results here?

Can't hurt certainly; done.

http://codereview.chromium.org/164100/diff/33/1008#newcode89
Line 89: { (const void*)ChromePlugin##function, (const void*)function }
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> One more thing: prefer c++<style>(casts).

Done.

http://codereview.chromium.org/164100/diff/33/1009
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/164100/diff/33/1009#newcode423
Line 423: interpose_list.append(":");
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> I was expecting to append kInterposeLibraryPath to the variable instead of
> prepending it.  When DYLD_INSERT_LIBRARIES is used for things like
GuardMalloc,
> we'd want GuardMalloc to be the allocator in the dylib's code too, and I'm not
> positive that it would be if we prepended our dylib.

Done.

http://codereview.chromium.org/164100/diff/33/1010
File chrome/chrome.gyp (right):

http://codereview.chromium.org/164100/diff/33/1010#newcode4459
Line 4459: 'chrome_dll',
On 2009/08/07 03:38:58, TVL wrote:
> why does this depend on chrome_dll? is this for the window tracker?

Yep.

http://codereview.chromium.org/164100/diff/33/1011
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/164100/diff/33/1011#newcode30
Line 30: #if defined(OS_MACOSX)
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> This can be an #elif on the group above, which is already #if win/#elif linux.

Done.

http://codereview.chromium.org/164100/diff/33/1011#newcode37
Line 37: static void TrimInterposeEnvironment() {
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> I think this can be made much less complex.

I figured it was better to spend a bit of extra effort to make it handle
everything, so we don't need to revisit it later (say, if we change the order of
interposing again for whatever reason). I especially wanted to prevent the
possibility that somehow we'd end up with a different list without the code on
this end being updated, thus causing us to bleed our interposing library without
us noticing.

http://codereview.chromium.org/164100/diff/33/1011#newcode67
Line 67: #endif
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> A long #ifdef like this deserves an // OS_MACOSX comment at the close.

Done.

http://codereview.chromium.org/164100/diff/33/1012
File webkit/glue/plugins/fake_plugin_window_tracker_mac.cc (right):

http://codereview.chromium.org/164100/diff/33/1012#newcode8
Line 8: window_to_delegate_map_ = CFDictionaryCreateMutable(kCFAllocatorDefault,
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> If using a scoped_cftyperef, do this in an initializer list.

Done.

http://codereview.chromium.org/164100/diff/33/1012#newcode13
Line 13: CFRelease(window_to_delegate_map_);
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> Do we need to assert that the map was empty?  Dispose any of the windows from
> the map? 

Since the goal is to eventually not even have the windows, it's probably not
worthwhile. In practice it'll be process termination before this class goes away
anyway.

http://codereview.chromium.org/164100/diff/33/1012#newcode17
Line 17: static FakePluginWindowTracker* tracker = NULL;
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> You can simplify this, since the compiler's effectively going to write the
|if|
> for you anyway:

Done.

http://codereview.chromium.org/164100/diff/33/1012#newcode21
Line 21: return tracker;
On 2009/08/07 03:38:58, TVL wrote:
> is there any threading worry to make sure only one exists?

AFAIK our process only interacts with plugins on the main thread (since it's
mostly event and drawing stuff), and the Carbon calls have to be on the main
thread.

http://codereview.chromium.org/164100/diff/33/1012#newcode51
Line 51: CFDictionaryRemoveValue(window_to_delegate_map_, delegate);
On 2009/08/07 03:38:58, TVL wrote:
> do you need to double check that window and delegate were related?

Done. (The fact that |window| is passed in at all is a bit of a hack that's just
there because it's messy to go from delegate to window).

> why wouldn't they pass window?

They would; it's just a defense against crashing if somehow we never
successfully generated an window for them in the first place. Comment added.

http://codereview.chromium.org/164100/diff/33/1013
File webkit/glue/plugins/fake_plugin_window_tracker_mac.h (right):

http://codereview.chromium.org/164100/diff/33/1013#newcode17
Line 17: // the state of the world.
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> This sentence is long and I'm not tired.  Future readers may also be tired. 
> Split it up into two sentences just to be safe.
> 
> "Serves as a bridge between passwordorplugin delegate instances and the Carbon
> interposing library.  Blahblah about the delegates, and more blahblah about
the
> interposing library and WindowRefs."

Done.

http://codereview.chromium.org/164100/diff/33/1013#newcode21
Line 21: ~FakePluginWindowTracker();
On 2009/08/07 01:29:04, Mark Mentovai wrote:
> Hold the dictionary in a scoped_cftyperef so that you don't need this Gozer at
> all.

Done.

[TVL, 2009-08-07 18:44:14]

lgtm

[Mark Mentovai, 2009-08-07 18:44:37]

http://codereview.chromium.org/164100/diff/1019/1023
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/164100/diff/1019/1023#newcode38
Line 38: getenv(plugin_interpose_strings::kDYLDInsertLibrariesKey);
indentify

http://codereview.chromium.org/164100/diff/1019/1023#newcode40
Line 40: return;
NOTREACHED here like you do if there is a list but it doesn't contain the thing.

http://codereview.chromium.org/164100/diff/1019/1023#newcode56
Line 56: // Pull out our library path, plus the dividing ':' on one side, and
This is not careful to ensure that it's actually getting a ":", and it should
be.

In other words, if some joker sets DYLD_INSERT_LIBRARIES to
/path/to/carbon_interpose.dylib/path/to/my_secret_interpose.dylib, we'll get it
here as
/path/to/carbon_interpose.dylib/path/to/my_secret_interpose.dylib:/path/to/carbon_interpose.dylib,
and then we'll trim it down to
/path/to/my_secret_interpose.dylib:/path/to/carbon_interpose.dylib.  Not
correct.

This is another reason I think the less complex "find"-free code is better...

[stuartmorgan, 2009-08-07 20:47:32]

http://codereview.chromium.org/164100/diff/1019/1023
File chrome/plugin/plugin_main.cc (right):

http://codereview.chromium.org/164100/diff/1019/1023#newcode38
Line 38: getenv(plugin_interpose_strings::kDYLDInsertLibrariesKey);
On 2009/08/07 18:44:37, Mark Mentovai wrote:
> indentify

Done. I sure do love how Xcode helpfully re-indents everything when you move
code via cut/paste!

http://codereview.chromium.org/164100/diff/1019/1023#newcode40
Line 40: return;
On 2009/08/07 18:44:37, Mark Mentovai wrote:
> NOTREACHED here like you do if there is a list but it doesn't contain the
thing.

Done.

http://codereview.chromium.org/164100/diff/1019/1023#newcode56
Line 56: // Pull out our library path, plus the dividing ':' on one side, and
On 2009/08/07 18:44:37, Mark Mentovai wrote:
> This is not careful to ensure that it's actually getting a ":", and it should
> be.

Done, using something more like what you suggested before but for suffix
testing, which unfortunately makes it complicated again.

[Mark Mentovai, 2009-08-07 20:49:59]

LGTM now