Fix to make Chromium more verbose about component extension loading error

Fix to make Chromium more verbose about component extension loading error

Change some DLOG and NOTREACHED to LOG, to make debugging component extensions on Release build easier
Add a ValidateExtension call after loading a component extension to find errors such as file loading failure
Change the signature of ValidateExtension for const correctness

BUG=None
TEST=None

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

[hashimoto, 2011-08-05 07:38:44]

Hello altimofeev,

I will appreciate if you could review this change.
This change will reduce the difficulty of debugging component extensions.

[altimofeev, 2011-08-08 09:22:55]

I think that Aaron (aa@chromium.org) is the better person to review this CL.

On 2011/08/05 07:38:44, hashimoto wrote:
> Hello altimofeev,
> 
> I will appreciate if you could review this change.
> This change will reduce the difficulty of debugging component extensions.

[hashimoto, 2011-08-08 09:36:15]

Hello Aaron, could you review this CL?

On 2011/08/08 09:22:55, altimofeev wrote:
> I think that Aaron (mailto:aa@chromium.org) is the better person to review
this CL.
> 
> On 2011/08/05 07:38:44, hashimoto wrote:
> > Hello altimofeev,
> > 
> > I will appreciate if you could review this change.
> > This change will reduce the difficulty of debugging component extensions.

[Aaron Boodman, 2011-08-08 21:31:42]

http://codereview.chromium.org/7585003/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7585003/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:1098: if
(!extension_file_util::ValidateExtension(extension, &error)) {
Please remove this.

Component extensions must be fast to load since they are enabled by default with
every build. In particular, they should not cause any file IO in the typical
case.

This adds a bunch of file IO, and not only that puts it on the UI thread. Chrome
must never do file IO on the UI thread.

It's better to have component extensions be a bit harder to debug than to slow
down Chrome's normal startup path.

[hashimoto, 2011-08-09 06:19:52]

Thank you for your kind review.
I fixed the patch.

http://codereview.chromium.org/7585003/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7585003/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service.cc:1098: if
(!extension_file_util::ValidateExtension(extension, &error)) {
Thank you for your detailed explanation.

I abandoned this part and switched to make more log when the content of an
extension is actually loaded. ( http://codereview.chromium.org/7528021/ )

On 2011/08/08 21:31:42, Aaron Boodman wrote:
> Please remove this.
> 
> Component extensions must be fast to load since they are enabled by default
with
> every build. In particular, they should not cause any file IO in the typical
> case.
> 
> This adds a bunch of file IO, and not only that puts it on the UI thread.
Chrome
> must never do file IO on the UI thread.
> 
> It's better to have component extensions be a bit harder to debug than to slow
> down Chrome's normal startup path.

[Aaron Boodman, 2011-08-09 18:42:19]

LGTM

Please be careful in the future when using any function that looks like it might
be calling OS routines, or doing file IO. Make sure you know which thread you
are on when you call it.

We have code in Chrome that attempts to warn you when you do this, but it is
disabled in some cases for legacy reasons, so we still need to be vigilant.

[Aaron Boodman, 2011-08-09 18:47:05]

One more thing. It looks to me like base::ThreadRestrictions should have caused
a crash with this change. Can you investigate why it didn't?

[Aaron Boodman, 2011-08-09 18:55:45]

It might have just been a DBG/RELEASE issue. ThreadRestrictions only warns in
debug mode.

[hashimoto, 2011-08-10 09:04:15]

I am mainly working with ChromeOS, and Chrome on ChromeOS built with
BUILDTYPE=Debug does not work well for some unknown reasons now.
So I had no idea to try testing my code with Debug build.

Thank you for your kind advice.
I should, at least for non-ChromeOS-specific changes, do test with Debug build
on my Linux workstation.

On 2011/08/09 18:55:45, Aaron Boodman wrote:
> It might have just been a DBG/RELEASE issue. ThreadRestrictions only warns in
> debug mode.