DCHECK earlier if pepper flash is missing even though FLAPPER_AVAILABLE is defined.

DCHECK earlier if pepper flash is missing even though FLAPPER_AVAILABLE is defined.

BUG=123632
TEST=The steps to trigger the bug don't crash anymore, but the browser crashes earlier if FLAPPER_AVAILABLE is misconfigured.


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

[Joao da Silva, 2012-04-18 11:51:22]

Please review, thanks!

[viettrungluu, 2012-04-18 16:24:39]

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
chrome/common/chrome_content_client.cc:234:
DCHECK(file_util::PathExists(plugin.path))
This, I believe, causes file IO to be done on the UI thread, which isn't good
practice even for a Debug build.

[Joao da Silva, 2012-04-19 13:27:22]

@viettrungluu: please consider the comment inline, do you still prefer to not
have this in?

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
chrome/common/chrome_content_client.cc:234:
DCHECK(file_util::PathExists(plugin.path))
On 2012/04/18 16:24:39, viettrungluu wrote:
> This, I believe, causes file IO to be done on the UI thread, which isn't good
> practice even for a Debug build.

This function is similar to ComputeBuiltInPlugins() above, which also checks for
the existence of the plugins listed there (and that's why those don't break).
This is also invoked during startup and before thread restrictions are
activated. An asynchronous interface around this would be better, but isn't
worth the complication.

This is causing issues at other completely unrelated levels, and was quite a
pain to track down. Having an early fail here spares a lot of time, and only
causes the IO penalty on debug builds.

[viettrungluu, 2012-04-19 23:34:24]

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
chrome/common/chrome_content_client.cc:234:
DCHECK(file_util::PathExists(plugin.path))
On 2012/04/19 13:27:22, Joao da Silva wrote:
> On 2012/04/18 16:24:39, viettrungluu wrote:
> > This, I believe, causes file IO to be done on the UI thread, which isn't
good
> > practice even for a Debug build.
> 
> This function is similar to ComputeBuiltInPlugins() above, which also checks
for
> the existence of the plugins listed there (and that's why those don't break).

ComputeBuiltInPlugins() should be fixed. Well, more generally, the
"path-getting" functions that check for existence of files should be fixed.
<shrug>

> This is also invoked during startup and before thread restrictions are
> activated.

Which is unfortunate, since our startup time is getting more and more dismal.
And people wonder why.

> An asynchronous interface around this would be better, but isn't
> worth the complication.
> 
> This is causing issues at other completely unrelated levels, and was quite a
> pain to track down. Having an early fail here spares a lot of time, and only
> causes the IO penalty on debug builds.

Well, ok. Skip the "Use --..." bit of the message though. If someone is doing
Official Debug builds, then they should be able to figure it out.

[viettrungluu, 2012-04-20 05:32:11]

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
chrome/common/chrome_content_client.cc:234:
DCHECK(file_util::PathExists(plugin.path))
Forgot to say: lgtm with a less verbose message.

On 2012/04/19 23:34:25, viettrungluu wrote:
> On 2012/04/19 13:27:22, Joao da Silva wrote:
> > On 2012/04/18 16:24:39, viettrungluu wrote:
> > > This, I believe, causes file IO to be done on the UI thread, which isn't
> good
> > > practice even for a Debug build.
> > 
> > This function is similar to ComputeBuiltInPlugins() above, which also checks
> for
> > the existence of the plugins listed there (and that's why those don't
break).
> 
> ComputeBuiltInPlugins() should be fixed. Well, more generally, the
> "path-getting" functions that check for existence of files should be fixed.
> <shrug>
> 
> > This is also invoked during startup and before thread restrictions are
> > activated.
> 
> Which is unfortunate, since our startup time is getting more and more dismal.
> And people wonder why.
> 
> > An asynchronous interface around this would be better, but isn't
> > worth the complication.
> > 
> > This is causing issues at other completely unrelated levels, and was quite a
> > pain to track down. Having an early fail here spares a lot of time, and only
> > causes the IO penalty on debug builds.
> 
> Well, ok. Skip the "Use --..." bit of the message though. If someone is doing
> Official Debug builds, then they should be able to figure it out.

[Joao da Silva, 2012-04-20 10:00:29]

Thanks for the review!

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/10035024/diff/1/chrome/common/chrome_content_c...
chrome/common/chrome_content_client.cc:234:
DCHECK(file_util::PathExists(plugin.path))
On 2012/04/20 05:32:11, viettrungluu wrote:
> Forgot to say: lgtm with a less verbose message.

Removed the log message altogether, and left a comment instead. The purpose is
to point people affected by this in the right direction.

> 
> On 2012/04/19 23:34:25, viettrungluu wrote:
> > On 2012/04/19 13:27:22, Joao da Silva wrote:
> > > On 2012/04/18 16:24:39, viettrungluu wrote:
> > > > This, I believe, causes file IO to be done on the UI thread, which isn't
> > good
> > > > practice even for a Debug build.
> > > 
> > > This function is similar to ComputeBuiltInPlugins() above, which also
checks
> > for
> > > the existence of the plugins listed there (and that's why those don't
> break).
> > 
> > ComputeBuiltInPlugins() should be fixed. Well, more generally, the
> > "path-getting" functions that check for existence of files should be fixed.
> > <shrug>

Yes indeed. I'm actually working on something related: http://crbug.com/119550

> > 
> > > This is also invoked during startup and before thread restrictions are
> > > activated.
> > 
> > Which is unfortunate, since our startup time is getting more and more
dismal.
> > And people wonder why.
> > 
> > > An asynchronous interface around this would be better, but isn't
> > > worth the complication.
> > > 
> > > This is causing issues at other completely unrelated levels, and was quite
a
> > > pain to track down. Having an early fail here spares a lot of time, and
only
> > > causes the IO penalty on debug builds.
> > 
> > Well, ok. Skip the "Use --..." bit of the message though. If someone is
doing
> > Official Debug builds, then they should be able to figure it out.
>

[commit-bot: I haz the power, 2012-04-20 11:54:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10035024/10001

[commit-bot: I haz the power, 2012-04-20 13:16:27]

Change committed as 133188