Parse messages.json in ExtensionUnpacker (like we do for manifest)....

Parse messages.json in ExtensionUnpacker (like we do for manifest) and pass them to sandboxed_extension_unpacker.
Added unittest files for unpacker and sandboxed unpacker.

TEST=Try loading any of the unpacker samples added in this CL. They should either pass, or show error and fail.
BUG=27362

[Nebojša Ćirić, 2009-11-12 02:38:09]

I hope this covers out of browser parse.

Lots of data files/test extensions - just looks big :)

[Aaron Boodman, 2009-11-12 07:46:46]

This is missing the part where we pass the parsed message catalogs back over IPC
and let the browser re-serialize them.

See: chrome/utility/utility_thread.cc ::OnUnpackExtension() and
chrome/browser/extensions/sandboxed_extension_unpacker.cc
::OnUnpackExtensionSucceeded().

This is important because otherwise a compromised renderer could just write
whatever it wants for the resource catalogs and we would try to read them in the
browser. The browser must be in charge of actually serializing any data it will
later try to parse.

http://codereview.chromium.org/390019/diff/1/29
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/390019/diff/1/29#newcode116
Line 116: bool* has_default_locale,
It looks like has_default_locale is not necessary here. Returning false from the
function means there was an error, returning true and an empty locale means
there was no default locale, returning true and a non-empty locale means there
was a locale.

[Nebojša Ćirić, 2009-11-13 22:38:42]

All done.

I've decided to pass catalogs like we pass images - as a pickled file.
Added unittest for sandboxed_extension_unpacker, and refactored some of the code
there.

All unittests pass locally, but some fail on try bots. Will work on figuring
that out.

On 2009/11/12 07:46:46, Aaron Boodman wrote:
> This is missing the part where we pass the parsed message catalogs back over
IPC
> and let the browser re-serialize them.
> 
> See: chrome/utility/utility_thread.cc ::OnUnpackExtension() and
> chrome/browser/extensions/sandboxed_extension_unpacker.cc
> ::OnUnpackExtensionSucceeded().
> 
> This is important because otherwise a compromised renderer could just write
> whatever it wants for the resource catalogs and we would try to read them in
the
> browser. The browser must be in charge of actually serializing any data it
will
> later try to parse.
> 
> http://codereview.chromium.org/390019/diff/1/29
> File chrome/common/extensions/extension_unpacker.cc (right):
> 
> http://codereview.chromium.org/390019/diff/1/29#newcode116
> Line 116: bool* has_default_locale,
> It looks like has_default_locale is not necessary here. Returning false from
the
> function means there was an error, returning true and an empty locale means
> there was no default locale, returning true and a non-empty locale means there
> was a locale.

[Nebojša Ćirić, 2009-11-17 02:26:45]

I've made one uber-catalog with locale paths being keys and sub-dictionaries
being catalogs. I took care that keys don't have . in them (which they shouldn't
anyways - they are of the form _locales/en_US).

I think it's better than playing with vectors and linked_ptrs (I did it both
ways, and vector/linked_ptr just didn't look natural in render_messages).

[Aaron Boodman, 2009-11-17 22:49:44]

This is looking a lot better, nice work. Particularly appreciate the detailed
testing.

The one thing I still see structurally is that the validation of the locale
directory structure is all done in ExtensionUnpacker. This logic should really
be in extension_file_util::ValidateExtension(). That way it will be get used
whenever we use that function. For example, we also call that function when
using the "Load unpacked extension..." button and when packing extensions.

http://codereview.chromium.org/390019/diff/9009/9040
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/390019/diff/9009/9040#newcode124
Line 124: if (!file_util::WriteFile(manifest_path,
I like the way you pulled out rewriting the other resources into separate
utility functions. Maybe do the same for the manifest?

http://codereview.chromium.org/390019/diff/9009/9040#newcode338
Line 338: FilePath path_suffix = path_suffix.FromWStringHack(*key_it);
path_suffix is an odd name for this variable. It's just the path, right?

http://codereview.chromium.org/390019/diff/9009/9041
File chrome/browser/extensions/sandboxed_extension_unpacker.h (right):

http://codereview.chromium.org/390019/diff/9009/9041#newcode15
Line 15: #include "chrome/common/extensions/extension_unpacker.h"
Looks like this include isn't necessary?

http://codereview.chromium.org/390019/diff/9009/9043
File chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/390019/diff/9009/9043#newcode53
Line 53: public:
Indent 1 space.

http://codereview.chromium.org/390019/diff/9009/9037
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/390019/diff/9009/9037#newcode38
Line 38: const char kParsedCatalogsFilename[] = "PARSED_CATALOGS";
Constant is not used.

http://codereview.chromium.org/390019/diff/9009/9037#newcode117
Line 117: static bool ParseDefaultLocale(const DictionaryValue* manifest,
It would probably be cleaner to make this a method so that it could access
error_. That way you don't need to pass it as an out param.

http://codereview.chromium.org/390019/diff/9009/9037#newcode123
Line 123: if (has_default_locale &&
Would improve readability to break this in two:

if (manifest->HasKey(..)) {
  if (!manifest->GetString(...) || default_locale->empty()) {
    .. failure ...
  }
}

http://codereview.chromium.org/390019/diff/9009/9037#newcode164
Line 164: SetError(errors::kLocalesTreeMissing);
Isn't this error more like "default locale not found"?

http://codereview.chromium.org/390019/diff/9009/9037#newcode172
Line 172: // for testing and for creating super dictionary with paths as keys.
Please update this comment as we discussed in irc.

http://codereview.chromium.org/390019/diff/9009/9034
File chrome/common/extensions/extension_unpacker.h (right):

http://codereview.chromium.org/390019/diff/9009/9034#newcode57
Line 57: bool ReadMessageCatalogs();
How about naming this one ReadAllMessageCatalogs(), and renaming
AddParsedCatalog() ReadMessageCatalog().

[Nebojša Ćirić, 2009-11-18 01:19:29]

All done. Moved verification code to extension file utils.

Re-added default_locale to extension object.

http://codereview.chromium.org/390019/diff/9009/9040
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/390019/diff/9009/9040#newcode124
Line 124: if (!file_util::WriteFile(manifest_path,
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> I like the way you pulled out rewriting the other resources into separate
> utility functions. Maybe do the same for the manifest?

Done.

http://codereview.chromium.org/390019/diff/9009/9040#newcode338
Line 338: FilePath path_suffix = path_suffix.FromWStringHack(*key_it);
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> path_suffix is an odd name for this variable. It's just the path, right?

I've used similar name we used for image extraction. I've changed it to
relative_path (this is not full path to the resource).

http://codereview.chromium.org/390019/diff/9009/9041
File chrome/browser/extensions/sandboxed_extension_unpacker.h (right):

http://codereview.chromium.org/390019/diff/9009/9041#newcode15
Line 15: #include "chrome/common/extensions/extension_unpacker.h"
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> Looks like this include isn't necessary?

Done.

http://codereview.chromium.org/390019/diff/9009/9043
File chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/390019/diff/9009/9043#newcode53
Line 53: public:
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> Indent 1 space.

Done.

http://codereview.chromium.org/390019/diff/9009/9037
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/390019/diff/9009/9037#newcode38
Line 38: const char kParsedCatalogsFilename[] = "PARSED_CATALOGS";
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> Constant is not used.

Done.

http://codereview.chromium.org/390019/diff/9009/9037#newcode117
Line 117: static bool ParseDefaultLocale(const DictionaryValue* manifest,
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> It would probably be cleaner to make this a method so that it could access
> error_. That way you don't need to pass it as an out param.

Done.

http://codereview.chromium.org/390019/diff/9009/9037#newcode123
Line 123: if (has_default_locale &&
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> Would improve readability to break this in two:
> 
> if (manifest->HasKey(..)) {
>   if (!manifest->GetString(...) || default_locale->empty()) {
>     .. failure ...
>   }
> }

Done.

http://codereview.chromium.org/390019/diff/9009/9037#newcode164
Line 164: SetError(errors::kLocalesTreeMissing);
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> Isn't this error more like "default locale not found"?

Done.

http://codereview.chromium.org/390019/diff/9009/9037#newcode172
Line 172: // for testing and for creating super dictionary with paths as keys.
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> Please update this comment as we discussed in irc.

Done.

http://codereview.chromium.org/390019/diff/9009/9034
File chrome/common/extensions/extension_unpacker.h (right):

http://codereview.chromium.org/390019/diff/9009/9034#newcode57
Line 57: bool ReadMessageCatalogs();
On 2009/11/17 22:49:44, Aaron Boodman wrote:
> How about naming this one ReadAllMessageCatalogs(), and renaming
> AddParsedCatalog() ReadMessageCatalog().

Done.

[Aaron Boodman, 2009-11-18 01:31:42]

Cool, lgtm

http://codereview.chromium.org/390019/diff/11001/11043
File chrome/browser/extensions/extension_file_util.h (right):

http://codereview.chromium.org/390019/diff/11001/11043#newcode98
Line 98: bool ValidateLocaleInfo(const Extension& extension, std::string*
error);
Since this doesn't need to be called from outside the .cc file, you should make
it static and omit this in the header.