Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10914119:
Prime the Windows theme DLL earlier. (Closed)
|
Created:
8 years, 3 months ago by MAD Modified:
8 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrime the Windows theme DLL earlier.
On Windows 8, we need to check if we are touch-enabled before we load our resources, and this happens before the code to prime the theme DLL gets executed. Since checking for touch-enable requires a call into the theme dll, it didn't get properly initialized in the render process... So moving this code to be done earlier was required...
At the same time, I changed the code a bit to rely on the current window station to no be WinSta0 to decide whether we need to do this or not, as opposed to looking at the sandbox state.
BUG=139241
TEST=Make sure we always get the proper theme for UI controls like scrollbars.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156232
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed Robert's nits. #Patch Set 3 : Simplified name check, and also not renderer specific anymore. #Patch Set 4 : Using InputDesktop instead #
Total comments: 6
Patch Set 5 : Fixing owners nit... #
Messages
Total messages: 22 (0 generated)
Thanks! BYE MAD...
Adding Ricardo.
nits and the like https://chromiumcodereview.appspot.com/10914119/diff/1/content/app/content_ma... File content/app/content_main_runner.cc (right): https://chromiumcodereview.appspot.com/10914119/diff/1/content/app/content_ma... content/app/content_main_runner.cc:111: static_buffer[0] = '\0'; char static_buffer[kStaticBufferLength] = ""; https://chromiumcodereview.appspot.com/10914119/diff/1/content/app/content_ma... content/app/content_main_runner.cc:112: const char * current_station_name = static_buffer; nit: const char* current_station_name = static_buffer; https://chromiumcodereview.appspot.com/10914119/diff/1/content/app/content_ma... content/app/content_main_runner.cc:115: std::string dynamic_buffer; Couldn't we simplify this by always using dynamic_buffer? Initialize dynamic_buffer to an empty string of length kStaticBufferLength: std::string dynamic_buffer('\0', kStaticBufferLength); https://chromiumcodereview.appspot.com/10914119/diff/1/content/app/content_ma... content/app/content_main_runner.cc:138: winsta0 = ::OpenWindowStationA("WinSta0", FALSE, GENERIC_READ); shorten this to HWINSTA winsta0 = ::OpenWindowStationA("WinSta0", FALSE, GENERIC_READ); https://chromiumcodereview.appspot.com/10914119/diff/1/content/app/content_ma... content/app/content_main_runner.cc:156: if (!current_station || !::SetProcessWindowStation(current_station)) { is it possible to get here if current_station is NULL? https://chromiumcodereview.appspot.com/10914119/diff/1/content/app/content_ma... content/app/content_main_runner.cc:642: // IsTouchEnanble, needed to load ressources, may call into the theme dll. *IsTouchEnabled, *resources
The winsta checking code seems very weird here, given that we only need to determine if we are sandboxed. What is the advantange of the winsta check? If any you can probably do a clearner one using WTSGetActiveConsoleSessionId and ProcessIdToSessionId or even better if it works just call OpenInputDesktop, hopefully it fails if you are not in an interactive windowstation.
Well... This is what I said in another email, I wonder why we need to know when we are sandboxed, because the failure to initialize the uxtheme.dll is only indirectly based on sandboxed or not, it's actually based on the winsta, so this is why I check the winsta name, which is what uxtheme.dll does in it's initialization to decide whether it disables itself or not... Am I missing something? On Thu, Sep 6, 2012 at 5:37 PM, <cpu@chromium.org> wrote: > The winsta checking code seems very weird here, given that we only need to > determine if we are sandboxed. > > What is the advantange of the winsta check? > > If any you can probably do a clearner one using > > WTSGetActiveConsoleSessionId and ProcessIdToSessionId or even better if it > works > just call OpenInputDesktop, hopefully it fails if you are not in an > interactive > windowstation. > > http://codereview.chromium.**org/10914119/<http://codereview.chromium.org/109... >
The old code was simpler :( Why not just always set the window station? http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... File content/app/content_main_runner.cc (right): http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... content/app/content_main_runner.cc:128: const_cast<char*>(dynamic_buffer.c_str()), WriteInto ?
I was trying to avoid doing it when we don't need to do it... If you all agree that it's not worth the added complexity of looking up the name of the current window station, then I'll remove it... On Thu, Sep 6, 2012 at 7:19 PM, <rvargas@chromium.org> wrote: > The old code was simpler :( > > Why not just always set the window station? > > > http://codereview.chromium.**org/10914119/diff/1/content/** > app/content_main_runner.cc<http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runner.cc> > File content/app/content_main_**runner.cc (right): > > http://codereview.chromium.**org/10914119/diff/1/content/** > app/content_main_runner.cc#**newcode128<http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runner.cc#newcode128> > content/app/content_main_**runner.cc:128: > const_cast<char*>(dynamic_**buffer.c_str()), > WriteInto ? > > http://codereview.chromium.**org/10914119/<http://codereview.chromium.org/109... >
How about this for the nits? We still need to decide whether we want to only do this when needed or all the time so that we can avoid checking the station name... BYE MAD http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... File content/app/content_main_runner.cc (right): http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... content/app/content_main_runner.cc:111: static_buffer[0] = '\0'; On 2012/09/06 18:35:56, robertshield wrote: > char static_buffer[kStaticBufferLength] = ""; Done. http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... content/app/content_main_runner.cc:112: const char * current_station_name = static_buffer; On 2012/09/06 18:35:56, robertshield wrote: > nit: const char* current_station_name = static_buffer; Done. http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... content/app/content_main_runner.cc:115: std::string dynamic_buffer; On 2012/09/06 18:35:56, robertshield wrote: > Couldn't we simplify this by always using dynamic_buffer? Initialize > dynamic_buffer to an empty string of length kStaticBufferLength: > std::string dynamic_buffer('\0', kStaticBufferLength); Done. http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... content/app/content_main_runner.cc:138: winsta0 = ::OpenWindowStationA("WinSta0", FALSE, GENERIC_READ); On 2012/09/06 18:35:56, robertshield wrote: > shorten this to HWINSTA winsta0 = ::OpenWindowStationA("WinSta0", FALSE, > GENERIC_READ); Done. http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... content/app/content_main_runner.cc:156: if (!current_station || !::SetProcessWindowStation(current_station)) { On 2012/09/06 18:35:56, robertshield wrote: > is it possible to get here if current_station is NULL? Done. http://codereview.chromium.org/10914119/diff/1/content/app/content_main_runne... content/app/content_main_runner.cc:642: // IsTouchEnanble, needed to load ressources, may call into the theme dll. On 2012/09/06 18:35:56, robertshield wrote: > *IsTouchEnabled, *resources Done.
I am happy with either 1) the way it was (function takes a bool) or 2) the use of WTSGetActiveConsoleSessionId and ProcessIdToSessionId or OpenInputDesktop if it works. This method has no string processing. The rationale for 1 is that only the sandbox will send a chrome process to another winsta. Besides if it where to be done after lockdown it will probably fail even if the windowstation was winsta0. The rationale for 2 is that... well, I have talked myself into option one. Sorry for the runaround.
Thanks for the info... But I don't think I agree... Here's why: What I don't like of 1) is that it is based on the assumption that this is needed for the sandbox, when this is actually needed because we run from a different windowstation, which is just a side effect of the sandbox... Currently the only way, but still... I feel it's not as clean as relying on the direct information... The previous code was simply assuming sandbox because it only ran for the renderer. Also it was only using the bool to identify if we are NOT running in the sandbox because there was a command line switch stating so (kNoSandBox). I also found weird that it still created the window even with no sandbox (and thus no WindowStation change). Do you know why we did that? Do you have an idea why we would need to do that? This is why I thought it was simpler (and more accurate) to only execute this code when we are not running on the winsta0. I'll see if I can make the session id stuff work instead of using the station names... I still need to find a proper home for this code anyway... So I'll have to shuffle things around... once it stabilizes somewhere that I feel makes sense, I'll go for another round of code review... OK? On Thu, Sep 6, 2012 at 8:32 PM, <cpu@chromium.org> wrote: > I am happy with either > > 1) the way it was (function takes a bool) or > 2) the use of WTSGetActiveConsoleSessionId and ProcessIdToSessionId or > OpenInputDesktop if it works. This method has no string processing. > > The rationale for 1 is that only the sandbox will send a chrome process to > another winsta. Besides if it where to be done after lockdown it will > probably > fail even if the windowstation was winsta0. > > The rationale for 2 is that... well, I have talked myself into option one. > Sorry > for the runaround. > > http://codereview.chromium.**org/10914119/<http://codereview.chromium.org/109... >
(more details below)
OK, so we have 3 options now:
1. Always set the window station to WinSta0 when running in the renderer
unless the command line switch says no_sandbox
2. Only set the Window Station when the current one is not WinSta0.
3. Only set the Window Station when OpenInputDesktop returns NULL.
I prefer 2. because this is explicitly what uxtheme.dll does to identify if
it should let us use themes or not, check the name of the Window Station.
1. is exactly what we are doing today and would only need to be moved
earlier.
The other dimension of choice is whether we should only do this when
running in the renderer, which we need if we choose option 1. If we choose
2. or 3., we can do that test all the time, for any process, though we
could also decide only to do it for the renderer... If we choose to only do
this for the renderer, we also need to find a proper place in
content/render to do this, currently, there is no code in there that is
called early enough, so we would need to add a new hook to it from within
the content main runner.
Good? Bad? Ugly?
*More Details...*
I just confirmed that in the case of a renderer with a different Window
Station than WinSta0, OpenInputDesktop returns NULL, and after we set the
Window station to WinSta0, it returns a non-NULL value... So we might be
able to use it instead of looking for the Window station name (as long as
there aren't any edge cases that would make it behave differently, although
I must admin that this line in the MSDN
doc<http://msdn.microsoft.com/en-us/library/windows/desktop/ms684309(v=vs.85).aspx>is
encouraging: "
*Remarks: The calling process must have an associated window station,
either assigned by the system when the process is created, or set by the
SetProcessWindowStation function. The window station associated with the
calling process must be capable of receiving input.*"
Also, I simplified the Window Station name check to the following:
HWINSTA current_station = ::GetProcessWindowStation();
static const char* kWindowStation0 = "WinSta0";
static const size_t kWindowStation0Length = 8;
char current_station_name[kWindowStation0Length];
current_station_name[0] = '\0';
DWORD needed_length = 0;
if (::GetUserObjectInformationA(current_station,
UOI_NAME,
current_station_name,
kWindowStation0Length,
&needed_length) &&
base::strncasecmp(current_station_name,
kWindowStation0,
kWindowStation0Length) == 0) {
return;
}
Other suggestions are welcome...
On Mon, Sep 10, 2012 at 12:25 PM, Marc-Andre Decoste <mad@chromium.org>wrote:
> Thanks for the info... But I don't think I agree... Here's why:
>
> What I don't like of 1) is that it is based on the assumption that this is
> needed for the sandbox, when this is actually needed because we run from a
> different windowstation, which is just a side effect of the sandbox...
> Currently the only way, but still... I feel it's not as clean as relying on
> the direct information...
>
> The previous code was simply assuming sandbox because it only ran for the
> renderer. Also it was only using the bool to identify if we are NOT running
> in the sandbox because there was a command line switch stating so
> (kNoSandBox).
>
> I also found weird that it still created the window even with no sandbox
> (and thus no WindowStation change). Do you know why we did that? Do you
> have an idea why we would need to do that?
>
> This is why I thought it was simpler (and more accurate) to only execute
> this code when we are not running on the winsta0.
>
> I'll see if I can make the session id stuff work instead of using the
> station names... I still need to find a proper home for this code anyway...
> So I'll have to shuffle things around... once it stabilizes somewhere that
> I feel makes sense, I'll go for another round of code review...
>
> OK?
>
>
> On Thu, Sep 6, 2012 at 8:32 PM, <cpu@chromium.org> wrote:
>
>> I am happy with either
>>
>> 1) the way it was (function takes a bool) or
>> 2) the use of WTSGetActiveConsoleSessionId and ProcessIdToSessionId or
>> OpenInputDesktop if it works. This method has no string processing.
>>
>> The rationale for 1 is that only the sandbox will send a chrome process to
>> another winsta. Besides if it where to be done after lockdown it will
>> probably
>> fail even if the windowstation was winsta0.
>>
>> The rationale for 2 is that... well, I have talked myself into option
>> one. Sorry
>> for the runaround.
>>
>>
http://codereview.chromium.**org/10914119/<http://codereview.chromium.org/109...
>>
>
>
Ping? Are you guys OK with this version? As mentioned before, I prefer to test the Window station name because this is what uxtheme's initialization code does... An alternative would be to try to open the input desktop, which seems to only be available on WinSta0, so we would avoid fetching the Window Station name and a string compare... But we would be dependent on something different from what uxtheme rely on to decide whether they enable or disable the theme resources. Or do you really want to keep the code as it is today and only move it earlier? Thanks!
I like the OpenInputDesktop version. Checking for strings is not great.
Like this then?
lgtm
Ho, cool... Jam@, do you agree as an owner? thanks!
Ho, cool... Jam@, do you agree as an owner? thanks!
lgtm
lgtm with nits http://codereview.chromium.org/10914119/diff/7/content/app/content_main_runne... File content/app/content_main_runner.cc (right): http://codereview.chromium.org/10914119/diff/7/content/app/content_main_runne... content/app/content_main_runner.cc:59: #include <string> nit: this is very redundant, no need to add http://codereview.chromium.org/10914119/diff/7/content/app/content_main_runne... content/app/content_main_runner.cc:103: // This needs to be done before we lock down the renderer. Officially this renderer->process to make it generic, now that this is in app http://codereview.chromium.org/10914119/diff/7/content/app/content_main_runne... content/app/content_main_runner.cc:138: // confuse the renderer enough that we should kill it now. ditto
Cool... Done... Thanks! CQ'ing... BYE MAD https://chromiumcodereview.appspot.com/10914119/diff/7/content/app/content_ma... File content/app/content_main_runner.cc (right): https://chromiumcodereview.appspot.com/10914119/diff/7/content/app/content_ma... content/app/content_main_runner.cc:59: #include <string> On 2012/09/12 00:36:05, John Abd-El-Malek wrote: > nit: this is very redundant, no need to add Done. https://chromiumcodereview.appspot.com/10914119/diff/7/content/app/content_ma... content/app/content_main_runner.cc:103: // This needs to be done before we lock down the renderer. Officially this On 2012/09/12 00:36:05, John Abd-El-Malek wrote: > renderer->process to make it generic, now that this is in app Done. https://chromiumcodereview.appspot.com/10914119/diff/7/content/app/content_ma... content/app/content_main_runner.cc:138: // confuse the renderer enough that we should kill it now. On 2012/09/12 00:36:05, John Abd-El-Malek wrote: > ditto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/10914119/9007
Change committed as 156232 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
