Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697463002:
Convert utility process extension Unpacker IPC to mojo (Closed)
|
Created:
3 years, 10 months ago by Noel Gordon Modified:
3 years, 9 months ago Reviewers:
Ken Rockot(use gerrit already), Devlin, jochen (gone - plz use gerrit), tibell, dcheng, Sam McNally CC:
Aaron Boodman, abarth-chromium, Ben Goodger (Google), catmullings, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, extensions-reviews_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojo, used to (unzip and) unpack a chrome
extension, and expose it to the browser via the utility process
policy file. Make callers use a utility process mojo client.
Add [Native] IPC enum param traits via a mojo typemap to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch auto-generates them: manifest_location_param_traits.cc).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file. A follow-up patch deletes
the messages-file so DecodeImages needs a new home.
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files via "targets is_dirty" errors, see review comment #169).
Add TestContentBrowserClient to the extension test harness, and
also an overlay file in test/data (needed to expose the mojo in
the extension test harness to the unit tests listed below).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
Review-Url: https://codereview.chromium.org/2697463002
Cr-Commit-Position: refs/heads/master@{#457374}
Committed: https://chromium.googlesource.com/chromium/src/+/c8702c4cdecf0859c34a5ae564c89463ac727967
Patch Set 1 #Patch Set 2 : Support manifest_overlay.json in extensions unittests. #Patch Set 3 : Add TestContentUtilityClient::ExposeInterfacesToBrowser. #Patch Set 4 : Add extensions::unpacker::TakeParsedManifest(). #
Total comments: 10
Patch Set 5 : Use MakeUnique when creating the utility mojo client. #
Total comments: 33
Patch Set 6 : Address review comments. #Patch Set 7 : #include sequenced_worker_pool. #Patch Set 8 : Add it again (from a different machine). #
Total comments: 6
Patch Set 9 : Address review comments. #Patch Set 10 : Sync to ToT. #
Total comments: 12
Patch Set 11 : Take #2, Add full mojo enum traits. #Patch Set 12 : Sync, review comments, remove utility_process_mojo_client.h change. #
Total comments: 5
Patch Set 13 : Comment-only change in response to review comments. #
Total comments: 15
Patch Set 14 : Take #3, use [Native] enum param traits. #
Total comments: 2
Patch Set 15 : Take #3, use [Native] enum param traits, try build fix. #Patch Set 16 : Take #4, declare the IPC enum traits in the message file, try build fix. #
Total comments: 2
Patch Set 17 : Declare [Native] enum param traits in extensions_messages.h file, delete custom param traits. #Patch Set 18 : Post compile step fails: targets is_dirty: try make extension unittests depend on :mojo (failed). #Patch Set 19 : Make mojo build the manifest location enum param traits (works) ... or ... #Patch Set 20 : Update using BUILD files to depend on //extensions/common:common (works). #Patch Set 21 : Move DecodedImages into namespace extensions. #Patch Set 22 : Set the IPC enum traits limit to extensions::Manifest::NUM_LOCATIONS - 1. #Messages
Total messages: 217 (162 generated)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Convert utility process extension umpacker IPC to mojo BUG=691410 ========== to ========== Convert utility process extension unpacker IPC to mojo BUG=691410 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
noel@chromium.org changed reviewers: + dcheng@chromium.org, jochen@chromium.org, rdevlin.cronin@chromium.org, sammc@chromium.org, tibell@chromium.org
PTAL. A DEPS change here to allow utilty_handler to use service_manager (so utility_handler can expose its mojo interfaces and quiet the presubmit nit). Next, teach the extensions_test sub- system about mojo overlays and the service manager, so utility_handler can expose its mojo interfaces during extension unit tests. +sammc, tibell for Mojo +dcheng for IPC Security +rdevlin.cronin for unpacker +jochen for OWNERS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/zipfile_installer.cc:60: if (!base::CreateTemporaryDirInDir(dir_temp, dir_name, &temp_dir_)) { This class would be easier to understand if fields were only accessed on the UI thread. Returning the path to be set to a field on the UI thread would be nicer. https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/zipfile_installer.cc:73: void ZipFileInstaller::UnzipOnIOThread() { Let's run this on the UI thread instead and let the UtilityProcessMojoClient deal with the IO thread. https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/zipfile_installer.cc:80: utility_process_mojo_client_.reset( utility_process_mojo_client_ = base::MakeUnique<content::UtilityProcessMojoClient<extensions::mojom::ExtensionUnpacker>>(utility_process_name); https://codereview.chromium.org/2697463002/diff/80001/extensions/browser/sand... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/80001/extensions/browser/sand... extensions/browser/sandboxed_unpacker.cc:353: utility_process_mojo_client_.reset( utility_process_mojo_client_ = base::MakeUnique<content::UtilityProcessMojoClient<extensions::mojom::ExtensionUnpacker>>(utility_process_name); https://codereview.chromium.org/2697463002/diff/80001/extensions/utility/util... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/80001/extensions/utility/util... extensions/utility/utility_handler.cc:47: if (UnzipFileManifestIntoPath(file, path, &manifest)) { How about making UnzipFileManifestIntoPath() return a std::unique_ptr<base::DictionaryValue> and changing this to if (auto manifest = UnzipFileManifestIntoPath(file, path)) {
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/zipfile_installer.cc:60: if (!base::CreateTemporaryDirInDir(dir_temp, dir_name, &temp_dir_)) { On 2017/02/14 05:41:52, Sam McNally wrote: > This class would be easier to understand if fields were only accessed on the UI > thread. Returning the path to be set to a field on the UI thread would be nicer. Done. Passed the zip_file path in, passed out the temp_dir path, so fields (members) are now only accessed on the UI thread. https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/zipfile_installer.cc:73: void ZipFileInstaller::UnzipOnIOThread() { On 2017/02/14 05:41:52, Sam McNally wrote: > Let's run this on the UI thread instead and let the UtilityProcessMojoClient > deal with the IO thread. Done. Simplifies things and removes the need for LoadOnUIThread too. https://codereview.chromium.org/2697463002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/zipfile_installer.cc:80: utility_process_mojo_client_.reset( On 2017/02/14 05:41:52, Sam McNally wrote: > utility_process_mojo_client_ = > base::MakeUnique<content::UtilityProcessMojoClient<extensions::mojom::ExtensionUnpacker>>(utility_process_name); Done. https://codereview.chromium.org/2697463002/diff/80001/extensions/browser/sand... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/80001/extensions/browser/sand... extensions/browser/sandboxed_unpacker.cc:353: utility_process_mojo_client_.reset( On 2017/02/14 05:41:52, Sam McNally wrote: > utility_process_mojo_client_ = > base::MakeUnique<content::UtilityProcessMojoClient<extensions::mojom::ExtensionUnpacker>>(utility_process_name); Done. https://codereview.chromium.org/2697463002/diff/80001/extensions/utility/util... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/80001/extensions/utility/util... extensions/utility/utility_handler.cc:47: if (UnzipFileManifestIntoPath(file, path, &manifest)) { On 2017/02/14 05:41:52, Sam McNally wrote: > How about making UnzipFileManifestIntoPath() return a > std::unique_ptr<base::DictionaryValue> and changing this to > if (auto manifest = UnzipFileManifestIntoPath(file, path)) { Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
FYI, +catmullings@ is also looking into converting extension messages to mojo (she has an in-progress CL to convert ParseManifest* et al). I've made a canonical bug at crbug.com/692120 for extension IPC messages; let's use that to make sure that we don't overlap efforts. https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:61: base::FilePath temp_dir; dir_temp vs temp_dir is a pretty ambiguous distinction :) Maybe something like temp_root and new_temp_dir, or similar? https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:102: ReportErrorOnUIThread(std::string(kExtensionHandlerFileUnzipError)); Since all this happens on the UI thread, we can just inline the success/failures into one method. void UnzipDone(bool success) { if (!extension_service_weak_) return; if (!success) { ErrorRreporter::GetInstance()->ReportLoadError(); return; } UnpackedInstaller::Create()->Load(); } https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.h (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.h:16: #include "extensions/common/extension_unpacker.mojom.h" Could this just be a forward declaration? https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.h:46: base::FilePath temp_dir_; Add member comments https://codereview.chromium.org/2697463002/diff/100001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:198: UtilityHandler::UtilityThreadStarted(); Why do we need this method, as opposed to calling UtilityHandler::UtilityThreadStarted() directly? https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:347: if (utility_process_mojo_client_) Should this happen? It looks like we're clearing the client after each operation. https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:440: RewriteManifestFile(*manifest.get())); *foo.get() is redundant. Prefer just *foo. https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.h (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... extensions/browser/sandboxed_unpacker.h:124: // SandboxedUnpacker::UnpackExtensionSucceeded() What's the motivation for changing these names? OnFoo seems like it's just as accurate or more accurate. https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, Mojo supports enums - can we do that instead? https://codereview.chromium.org/2697463002/diff/100001/extensions/test/test_c... File extensions/test/test_content_browser_client.cc (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/test/test_c... extensions/test/test_content_browser_client.cc:28: NOTREACHED(); nit: Given this is test-only code, these may as well be CHECK()s. https://codereview.chromium.org/2697463002/diff/100001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/utility/uti... extensions/utility/utility_handler.cc:46: if (auto manifest = UnzipFileManifest(file, path)) { I think auto in this case reduces readability. Please spell out the type. https://codereview.chromium.org/2697463002/diff/100001/extensions/utility/uti... extensions/utility/utility_handler.cc:67: if (unpacker.Run()) { optional nit: presumably, unpacker.error_message() is empty on success, and the manifest is empty on failure. So we could probably just do: unpacker.Run(); callback.Run(unpacker.error_message(), unpacker.TakeParsedManifest()); But if you prefer this way, I don't feel strongly. https://codereview.chromium.org/2697463002/diff/100001/extensions/utility/uti... extensions/utility/utility_handler.cc:84: } else { no else {} after return.
On 2017/02/14 17:24:59, Devlin wrote: > FYI, +catmullings@ is also looking into converting extension messages to mojo > (she has an in-progress CL to convert ParseManifest* et al). I have one for that IPC too ... based on the current patch. > I've made a canonical bug at crbug.com/692120 for extension IPC messages; let's use that to > make sure that we don't overlap efforts. You might of missed this, but so that efforts do not overlap, mojo maintains a public document listing what's conversions are been worked on, and by whom. https://docs.google.com/spreadsheets/d/1pGWX_wxGdjAVtQOmlDDfhuIc3Pbwg9FtvFXAX... My name is on five entries in total, three related to the utility process, they each have a crbug and are marked in progress, hence the current patch. I think it'd help if we used the spreadsheet to co-ordinate our efforts?
The CQ bit was checked by noel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:61: base::FilePath temp_dir; On 2017/02/14 17:24:59, Devlin wrote: > dir_temp vs temp_dir is a pretty ambiguous distinction :) Code on diff-left was that too :) and a tried to make it clearer ... > Maybe something like temp_root and new_temp_dir, or similar? since new_temp_dir was a little confusing to me in relation to the other local variables (ambiguous). Given what we do with it, maybe unzip_dir? https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:102: ReportErrorOnUIThread(std::string(kExtensionHandlerFileUnzipError)); On 2017/02/14 17:24:59, Devlin wrote: > Since all this happens on the UI thread, we can just inline the success/failures > into one method. I'll take part of the early return idea, but please note that the ReportErrorOnUIThread helper is called from two places in the code. It didn't really make sense to me to inline it's body in those two places, so I just used the helper. https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.h (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.h:16: #include "extensions/common/extension_unpacker.mojom.h" On 2017/02/14 17:24:59, Devlin wrote: > Could this just be a forward declaration? Possible, but not something we've conventionally done when converting utility process IPC to mojo ... since mojo tends to be very namespace nested. Once we have compilers with nested namespace support, it'll be a lot easier to do then. Maybe we should change the mojo code generator to create a fwd declaration .h or something, dunno. https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.h:46: base::FilePath temp_dir_; On 2017/02/14 17:24:59, Devlin wrote: > Add member comments When adding mojo to other folks code that we are not familiar with, we often don't know what members do, so any added comments about them from us are ~useless :) I added comments for the members vars I do know something about. https://codereview.chromium.org/2697463002/diff/100001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:198: UtilityHandler::UtilityThreadStarted(); On 2017/02/14 17:24:59, Devlin wrote: > Why do we need this method, as opposed to calling > UtilityHandler::UtilityThreadStarted() directly? ExtensionsHandler owns a UtilityHandler utility_handler_, so layering, we let ExtensionsHandler control it. And similarly below in ExtensionsHandler::ExposeInterfacesToBrowser. https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:347: if (utility_process_mojo_client_) On 2017/02/14 17:24:59, Devlin wrote: > Should this happen? It looks like we're clearing the client after each > operation. Yes it will happen. We clear it on failures [1], but barring those, we clear it only in SandboxedUnpacker::UnpackDone. SandboxedUnpacker::StartWithCrx calls unzip, _then_ unpack. SandboxedUnpacker::StartWithDirectory calls unpack only. So an unpack call, UnpackOnIOThread, has to start the utility process if we don't yet have one. Note in diff-left, old code at @903 the "if (!utility_host_) {", and the line here @347 diff-right matches that. [1] It is reset on failures: note this blocks the mojo client from calling back from that point on. https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:440: RewriteManifestFile(*manifest.get())); On 2017/02/14 17:24:59, Devlin wrote: > *foo.get() is redundant. Prefer just *foo. Done. https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:677: original_manifest.get(), extension_.get(), Perhaps client_->OnUnpackSuccess could take ownership the original_manifest. Quick look at it suggest it makes a deep copy of the original_manifest.get(). Dunno enough about extensions myself to say if that extra copy matters or not. https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.h (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... extensions/browser/sandboxed_unpacker.h:124: // SandboxedUnpacker::UnpackExtensionSucceeded() On 2017/02/14 17:24:59, Devlin wrote: > What's the motivation for changing these names? OnFoo seems like it's just as > accurate or more accurate. Short discussion about that in connection with a browser message filter conversion example here: https://www.chromium.org/developers/design-documents/mojo/mojo-migration-guide TLDR: OnFoo-ness is the old message system. Mojo is more orientated around procedure calls (local or remote). If I added a string copy service to mojo say, I'd maybe name its API strcpy() rather than OnStrCpy. We take out the On's, but retain the postfix about which thread it runs on, if any. https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, On 2017/02/14 17:24:59, Devlin wrote: > Mojo supports enums - can we do that instead? Yeah I cargo-culted the existing API here, there were CHECK's in the existing IPC handler to test if location is sain. I retained those. "Can we do that instead?" yeah we could, but all clients of SandboxedUnpacker would need to be changed to use the mojo enum defines (unless you want to maintain two enums). My feeling is there are lots of clients, and updating them might be a patch in itself. Maybe a job for crbug.com/692120 ? https://codereview.chromium.org/2697463002/diff/100001/extensions/test/test_c... File extensions/test/test_content_browser_client.cc (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/test/test_c... extensions/test/test_content_browser_client.cc:28: NOTREACHED(); On 2017/02/14 17:24:59, Devlin wrote: > nit: Given this is test-only code, these may as well be CHECK()s. Done. https://codereview.chromium.org/2697463002/diff/100001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/utility/uti... extensions/utility/utility_handler.cc:46: if (auto manifest = UnzipFileManifest(file, path)) { On 2017/02/14 17:24:59, Devlin wrote: > I think auto in this case reduces readability. Ok removed the auto & put the code back the way it originally was. An out param works for me. https://codereview.chromium.org/2697463002/diff/100001/extensions/utility/uti... extensions/utility/utility_handler.cc:67: if (unpacker.Run()) { On 2017/02/14 17:24:59, Devlin wrote: > optional nit: > presumably, unpacker.error_message() is empty on success, and the manifest is > empty on failure. So we could probably just do: > unpacker.Run(); > callback.Run(unpacker.error_message(), unpacker.TakeParsedManifest()); Dunno enough about unpacker to say, I was being conservative. > But if you prefer this way, I don't feel strongly. Yeah, my strong feeling here was to match what the exiting code did. Saved me reading too far into the unpacker code :) https://codereview.chromium.org/2697463002/diff/100001/extensions/utility/uti... extensions/utility/utility_handler.cc:84: } else { On 2017/02/14 17:24:59, Devlin wrote: > no else {} after return. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/02/15 13:35:13, noel gordon wrote: > On 2017/02/14 17:24:59, Devlin wrote: > > FYI, +catmullings@ is also looking into converting extension messages to mojo > > (she has an in-progress CL to convert ParseManifest* et al). > > I have one for that IPC too ... based on the current patch. For ParseManifest, See https://codereview.chromium.org/2699663003
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.h (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.h:16: #include "extensions/common/extension_unpacker.mojom.h" On 2017/02/15 17:28:08, noel gordon wrote: > On 2017/02/14 17:24:59, Devlin wrote: > > Could this just be a forward declaration? > > Possible, but not something we've conventionally done when converting utility > process IPC to mojo ... since mojo tends to be very namespace nested. Once we > have compilers with nested namespace support, it'll be a lot easier to do then. > > Maybe we should change the mojo code generator to create a fwd declaration .h or > something, dunno. Sorry, I don't follow - what's wrong with namespace extensions { namespace mojom { class ExtensionUnpacker; } ... } // namespace extensions ? We have nested namespace forward declarations in a lot of places in the code... https://codereview.chromium.org/2697463002/diff/100001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:198: UtilityHandler::UtilityThreadStarted(); On 2017/02/15 17:28:08, noel gordon wrote: > On 2017/02/14 17:24:59, Devlin wrote: > > Why do we need this method, as opposed to calling > > UtilityHandler::UtilityThreadStarted() directly? > > ExtensionsHandler owns a UtilityHandler utility_handler_, so layering, we let > ExtensionsHandler control it. And similarly below in > ExtensionsHandler::ExposeInterfacesToBrowser. I don't quite follow - this is a static method, so ownership in member variables doesn't really matter. In ExposeInterfacesToBrowser(), the layering is important, because it adds extra logic that UtilityHandler doesn't do. Here, it just seems to add an extra function call. https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, On 2017/02/15 17:28:09, noel gordon wrote: > On 2017/02/14 17:24:59, Devlin wrote: > > Mojo supports enums - can we do that instead? > > Yeah I cargo-culted the existing API here, there were CHECK's in the existing > IPC handler to test if location is sain. I retained those. > > "Can we do that instead?" yeah we could, but all clients of SandboxedUnpacker > would need to be changed to use the mojo enum defines (unless you want to > maintain two enums). > > My feeling is there are lots of clients, and updating them might be a patch in > itself. Maybe a job for crbug.com/692120 ? My understanding from rockot@ is that mojo can reuse the existing IPC_ENUM_TRAITS declarations, which would mean that we wouldn't need to update callers. Is that not the case? https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:83: content::UtilityProcessMojoClient<extensions::mojom::ExtensionUnpacker>>( nit: don't need extensions:: prefix here https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:102: ReportErrorOnUIThread(std::string(kExtensionHandlerFileUnzipError)); nit: I think it's okay to use the implicit ctor of std::string and just pass kExtensionHandlerFileUnzipError directly here. https://codereview.chromium.org/2697463002/diff/160001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/160001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:344: void SandboxedUnpacker::StartUtilityProcess(const base::FilePath& dir) { Given both instances of this call it with temp_dir_.GetPath(), can we remove the param and do that in the body of the function?
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #9 (id:180001) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:200001) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.h (right): https://codereview.chromium.org/2697463002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.h:16: #include "extensions/common/extension_unpacker.mojom.h" On 2017/02/17 15:53:56, Devlin wrote: > On 2017/02/15 17:28:08, noel gordon wrote: > > On 2017/02/14 17:24:59, Devlin wrote: > > > Could this just be a forward declaration? > > > > Possible, but not something we've conventionally done when converting utility > > process IPC to mojo ... since mojo tends to be very namespace nested. Once we > > have compilers with nested namespace support, it'll be a lot easier to do > then. > > > > Maybe we should change the mojo code generator to create a fwd declaration .h > or > > something, dunno. > > Sorry, I don't follow - what's wrong with > > namespace extensions { > namespace mojom { > class ExtensionUnpacker; > } > > ... > > } // namespace extensions > ? > > We have nested namespace forward declarations in a lot of places in the code... OK done. https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, On 2017/02/17 15:53:56, Devlin wrote: > On 2017/02/15 17:28:09, noel gordon wrote: > > On 2017/02/14 17:24:59, Devlin wrote: > > > Mojo supports enums - can we do that instead? > > > > Yeah I cargo-culted the existing API here, there were CHECK's in the existing > > IPC handler to test if location is sain. I retained those. > > > > "Can we do that instead?" yeah we could, but all clients of SandboxedUnpacker > > would need to be changed to use the mojo enum defines (unless you want to > > maintain two enums). > > > > My feeling is there are lots of clients, and updating them might be a patch in > > itself. Maybe a job for crbug.com/692120 ? > > My understanding from rockot@ is that mojo can reuse the existing > IPC_ENUM_TRAITS declarations, which would mean that we wouldn't need to update > callers. Is that not the case? Yes, but that would also restrict the code to C++ callers only. This CL allows any caller, be they C++, Java, JavaScript etc. https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:83: content::UtilityProcessMojoClient<extensions::mojom::ExtensionUnpacker>>( On 2017/02/17 15:53:56, Devlin wrote: > nit: don't need extensions:: prefix here Removed. https://codereview.chromium.org/2697463002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:102: ReportErrorOnUIThread(std::string(kExtensionHandlerFileUnzipError)); On 2017/02/17 15:53:56, Devlin wrote: > nit: I think it's okay to use the implicit ctor of std::string and just pass > kExtensionHandlerFileUnzipError directly here. Done. https://codereview.chromium.org/2697463002/diff/160001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/160001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:344: void SandboxedUnpacker::StartUtilityProcess(const base::FilePath& dir) { On 2017/02/17 15:53:56, Devlin wrote: > Given both instances of this call it with temp_dir_.GetPath(), can we remove the > param and do that in the body of the function? Done.
https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/100001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, On 2017/02/27 11:47:50, noel gordon wrote: > On 2017/02/17 15:53:56, Devlin wrote: > > On 2017/02/15 17:28:09, noel gordon wrote: > > > On 2017/02/14 17:24:59, Devlin wrote: > > > > Mojo supports enums - can we do that instead? > > > > > > Yeah I cargo-culted the existing API here, there were CHECK's in the > existing > > > IPC handler to test if location is sain. I retained those. > > > > > > "Can we do that instead?" yeah we could, but all clients of > SandboxedUnpacker > > > would need to be changed to use the mojo enum defines (unless you want to > > > maintain two enums). > > > > > > My feeling is there are lots of clients, and updating them might be a patch > in > > > itself. Maybe a job for crbug.com/692120 ? > > > > My understanding from rockot@ is that mojo can reuse the existing > > IPC_ENUM_TRAITS declarations, which would mean that we wouldn't need to update > > callers. Is that not the case? > > Yes, but that would also restrict the code to C++ callers only. This CL allows > any caller, be they C++, Java, JavaScript etc. I'd rather we optimize for the C++ case, since we don't currently have any plans to call this from any other language. :)
(other than that, this lg)
https://codereview.chromium.org/2697463002/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:77: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Does this need to be on the UI thread? Underneath, doesn't Mojo have to post the task to the IO thread to do the sending anyway? https://codereview.chromium.org/2697463002/diff/240001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:672: std::unique_ptr<base::DictionaryValue> original_manifest, Why pass as unique_ptr? It seems like we're not actually passing ownership anyway, so const base::DictionaryValue& is a clearer indication of that. https://codereview.chromium.org/2697463002/diff/240001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, Document location and creation_flags. Location should probably just be plumbed through as an enum. creation_flags looks like a bitmask, but let's have a breadcrumb that points at the C++ definitions for the enum flags. https://codereview.chromium.org/2697463002/diff/240001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/utility/uti... extensions/utility/utility_handler.cc:72: std::unique_ptr<base::DictionaryValue>()); nullptr
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(I'm waiting for the other reviewers to finish their reviews first)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2697463002/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/zipfile_installer.cc (right): https://codereview.chromium.org/2697463002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/zipfile_installer.cc:77: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/02/28 06:12:49, dcheng wrote: > Does this need to be on the UI thread? The user of this class expects replies on the UI thread. > Underneath, doesn't Mojo have to post the > task to the IO thread to do the sending anyway? Yes, but the mojo callbacks happen on the thread we called it from, in this case, the UI thread. https://codereview.chromium.org/2697463002/diff/240001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:672: std::unique_ptr<base::DictionaryValue> original_manifest, On 2017/02/28 06:12:49, dcheng wrote: > Why pass as unique_ptr? https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... , a question I had that went unanswered. > It seems like we're not actually passing ownership > anyway, so const base::DictionaryValue& is a clearer indication of that. Done. https://codereview.chromium.org/2697463002/diff/240001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, On 2017/02/28 06:12:49, dcheng wrote: > Document location and creation_flags. Location should probably just be plumbed > through as an enum. OK, added complete mojo enum type traits for the location. > creation_flags looks like a bitmask, but let's have a > breadcrumb that points at the C++ definitions for the enum flags. The C++ definitions are now all in enum traits files (which is a bit more than a breadcrumb). What is the breadcrumb you're asking for, and what additional file or files need it now we have mojo traits? https://codereview.chromium.org/2697463002/diff/240001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/utility/uti... extensions/utility/utility_handler.cc:72: std::unique_ptr<base::DictionaryValue>()); On 2017/02/28 06:12:49, dcheng wrote: > nullptr Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2697463002/diff/240001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:672: std::unique_ptr<base::DictionaryValue> original_manifest, On 2017/03/06 13:12:03, noel gordon wrote: > On 2017/02/28 06:12:49, dcheng wrote: > > Why pass as unique_ptr? > > https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... > , a question I had that went unanswered. > > > It seems like we're not actually passing ownership > > anyway, so const base::DictionaryValue& is a clearer indication of that. > > Done. I see. It might be worth pursuing, but I would suggest a followup https://codereview.chromium.org/2697463002/diff/240001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, On 2017/03/06 13:12:03, noel gordon wrote: > On 2017/02/28 06:12:49, dcheng wrote: > > Document location and creation_flags. Location should probably just be plumbed > > through as an enum. > > OK, added complete mojo enum type traits for the location. > > > creation_flags looks like a bitmask, but let's have a > > breadcrumb that points at the C++ definitions for the enum flags. > > The C++ definitions are now all in enum traits files (which is a bit more than a > breadcrumb). What is the breadcrumb you're asking for, and what additional file > or files need it now we have mojo traits? I think just have a pointer to what the underlying C++ enum is for creation_flags (I believe it's https://cs.chromium.org/chromium/src/extensions/common/extension.h?rcl=f763e1...) https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... extensions/utility/utility_handler.cc:59: CHECK_NE(location, Manifest::INVALID_LOCATION); I would suggest DCHECK_NE here, as we should be able to trust what the browser process sends us.
Description was changed from ========== Convert utility process extension unpacker IPC to mojo BUG=691410 ========== to ========== Convert utility process extension Unpacker IPC to mojo BUG=691410 ==========
https://codereview.chromium.org/2697463002/diff/240001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:672: std::unique_ptr<base::DictionaryValue> original_manifest, On 2017/03/07 09:45:13, dcheng wrote: > On 2017/03/06 13:12:03, noel gordon wrote: > > On 2017/02/28 06:12:49, dcheng wrote: > > > Why pass as unique_ptr? > > > > > https://codereview.chromium.org/2697463002/diff/100001/extensions/browser/san... > > , a question I had that went unanswered. > > > > > It seems like we're not actually passing ownership > > > anyway, so const base::DictionaryValue& is a clearer indication of that. > > > > Done. > > I see. It might be worth pursuing, but I would suggest a followup Might be, yes. I filed https://crbug.com/699528 about it. https://codereview.chromium.org/2697463002/diff/240001/extensions/common/exte... File extensions/common/extension_unpacker.mojom (right): https://codereview.chromium.org/2697463002/diff/240001/extensions/common/exte... extensions/common/extension_unpacker.mojom:25: int32 location, On 2017/03/07 09:45:13, dcheng wrote: > On 2017/03/06 13:12:03, noel gordon wrote: > > On 2017/02/28 06:12:49, dcheng wrote: > > > Document location and creation_flags. Location should probably just be > plumbed > > > through as an enum. > > > > OK, added complete mojo enum type traits for the location. > > > > > creation_flags looks like a bitmask, but let's have a > > > breadcrumb that points at the C++ definitions for the enum flags. > > > > The C++ definitions are now all in enum traits files (which is a bit more than > a > > breadcrumb). What is the breadcrumb you're asking for, and what additional > file > > or files need it now we have mojo traits? > > I think just have a pointer to what the underlying C++ enum is for > creation_flags Added a creation_flags breadcrumb. (I believe it's > https://cs.chromium.org/chromium/src/extensions/common/extension.h?rcl=f763e1...) Thank you and yeap, I think that's the one also. https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... extensions/utility/utility_handler.cc:59: CHECK_NE(location, Manifest::INVALID_LOCATION); On 2017/03/07 09:45:14, dcheng wrote: > I would suggest DCHECK_NE here, as we should be able to trust what the browser > process sends us. See https://codereview.chromium.org/2711513003 Turns out this CHECK recently fired in the field a bit too often. The CL added CHECKs on the browser-side, the underlying issue is still being investigated. While those browser-side CHECK's are there, I'm ok with DCHECK_NE here as you suggested. But note: we'd need to change it back to CHECK_NE if those browser-side CHECKs are removed. Maybe safer to just leave it as a CHECK_NE for now, thoughts?
lgtm % nits https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... extensions/utility/utility_handler.cc:59: CHECK_NE(location, Manifest::INVALID_LOCATION); On 2017/03/08 13:44:26, noel gordon wrote: > On 2017/03/07 09:45:14, dcheng wrote: > > I would suggest DCHECK_NE here, as we should be able to trust what the browser > > process sends us. > > See https://codereview.chromium.org/2711513003 > > Turns out this CHECK recently fired in the field a bit too often. The CL added > CHECKs on the browser-side, the underlying issue is still being investigated. > > While those browser-side CHECK's are there, I'm ok with DCHECK_NE here as you > suggested. But note: we'd need to change it back to CHECK_NE if those > browser-side CHECKs are removed. > > Maybe safer to just leave it as a CHECK_NE for now, thoughts? As Noel points out, this has been a problem lately and we're looking into it. I'd like to keep the CHECK in place, and I'll update/remove it once we solve the issue. https://codereview.chromium.org/2697463002/diff/300001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2697463002/diff/300001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:179: ExtensionsHandler::~ExtensionsHandler() = default; nit: I'd slightly prefer to avoid making stylistic changes that are orthogonal to the patch, but don't feel strongly. https://codereview.chromium.org/2697463002/diff/300001/extensions/browser/san... File extensions/browser/sandboxed_unpacker_unittest.cc (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/browser/san... extensions/browser/sandboxed_unpacker_unittest.cc:5: #include "extensions/browser/sandboxed_unpacker.h" ditto re orthogonal stylistic changes https://codereview.chromium.org/2697463002/diff/300001/extensions/common/exte... File extensions/common/extension_utility_types.h (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/common/exte... extensions/common/extension_utility_types.h:13: #include "ui/gfx/ipc/skia/gfx_skia_param_traits.h" needed? https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... File extensions/common/manifest_location.mojom (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... extensions/common/manifest_location.mojom:11: enum ManifestLocation { If it were me, I'd probably just opt for using [Native] enum ManifestLocation and adding IPC_ENUM_TRAITS_MIN and IPC_ENUM_TRAITS_MAX for the location, so that this is a lot less verbose (and probably a bit less error prone). But I suppose it's largely an IPC decision, so I'll defer to dcheng. https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... extensions/common/manifest_location.mojom:13: INTERNAL, // A crx file from the internal Extensions directory. If we do stick with this approach, we shouldn't duplicate each comment. Instead, just say "See Manifest::Location for what these enums mean." That way, if/when we change any, we don't have to update comments in multiple locations.
https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... extensions/utility/utility_handler.cc:59: CHECK_NE(location, Manifest::INVALID_LOCATION); On 2017/03/08 16:22:24, Devlin wrote: > On 2017/03/08 13:44:26, noel gordon wrote: > > On 2017/03/07 09:45:14, dcheng wrote: > > > I would suggest DCHECK_NE here, as we should be able to trust what the > browser > > > process sends us. > > > > See https://codereview.chromium.org/2711513003 > > > > Turns out this CHECK recently fired in the field a bit too often. The CL added > > CHECKs on the browser-side, the underlying issue is still being investigated. > > > > While those browser-side CHECK's are there, I'm ok with DCHECK_NE here as you > > suggested. But note: we'd need to change it back to CHECK_NE if those > > browser-side CHECKs are removed. > > > > Maybe safer to just leave it as a CHECK_NE for now, thoughts? > > As Noel points out, this has been a problem lately and we're looking into it. > I'd like to keep the CHECK in place, and I'll update/remove it once we solve the > issue. My vote is leave the CHECK. https://codereview.chromium.org/2697463002/diff/300001/chrome/utility/extensi... File chrome/utility/extensions/extensions_handler.cc (right): https://codereview.chromium.org/2697463002/diff/300001/chrome/utility/extensi... chrome/utility/extensions/extensions_handler.cc:179: ExtensionsHandler::~ExtensionsHandler() = default; On 2017/03/08 16:22:24, Devlin wrote: > nit: I'd slightly prefer to avoid making stylistic changes that are orthogonal > to the patch, but don't feel strongly. Don't feel strongly about this type of change so much, but in general, my view is orthogonal changes are orthogonal and imho, best dealt with in follow-up bugs (eg., the bug I filed about original_manifest). https://codereview.chromium.org/2697463002/diff/300001/extensions/browser/san... File extensions/browser/sandboxed_unpacker_unittest.cc (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/browser/san... extensions/browser/sandboxed_unpacker_unittest.cc:5: #include "extensions/browser/sandboxed_unpacker.h" On 2017/03/08 16:22:24, Devlin wrote: > ditto re orthogonal stylistic changes Yeah I started up the track of forward declaring more classes when you mentioned forward declaring the mojo type. Patch became too large and I backed out, but a % git cl format caught this nit. Kept it since 1) git cl format is correct, and 2) it helped remind reviewers that this feature has tests. With my mojo-hat on, we really appreciate it when we find good test coverage for our conversions. https://codereview.chromium.org/2697463002/diff/300001/extensions/common/exte... File extensions/common/extension_utility_types.h (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/common/exte... extensions/common/extension_utility_types.h:13: #include "ui/gfx/ipc/skia/gfx_skia_param_traits.h" On 2017/03/08 16:22:24, Devlin wrote: > needed? Not sure, will check this tomorrow. https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... File extensions/common/manifest_location.mojom (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... extensions/common/manifest_location.mojom:11: enum ManifestLocation { On 2017/03/08 16:22:24, Devlin wrote: > If it were me, I'd probably just opt for using [Native] enum ManifestLocation > and adding IPC_ENUM_TRAITS_MIN and IPC_ENUM_TRAITS_MAX for the location, /me would have just stuck with the ints :) They seemed fine to me, and they have been working for you for more than 5.5 years, without problem, when I looked back through git blame. For a [Native] enum ManifestLocation, we'd need to define IPC_ENUM_TRAITS somewhere (they don't already exist), somewhere outside of the -message.h file (since my goal in a follow-up patch is to delete the -message.h file). > so that > this is a lot less verbose (and probably a bit less error prone). Agree about full mojo enum traits being verbose (I had to type it all out, I'd not done one before so it was good practice for me, but yeah, verbose). sammc@ has been thinking about how we (mojo) can make them better. > But I suppose > it's largely an IPC decision, so I'll defer to dcheng. Hmmmm. While I was writing them, I was thinking about you and your team, and how y'all are going to manage them in future, do they add maintenance burden, how easy are they change and update, and so on. I'm don't feel it's largely an IPC decision when I consider those other factors as well. It's good we explore the issues: using ints is KISS to me, others have other view-points. https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... extensions/common/manifest_location.mojom:13: INTERNAL, // A crx file from the internal Extensions directory. On 2017/03/08 16:22:24, Devlin wrote: > If we do stick with this approach, we shouldn't duplicate each comment. > Instead, just say "See Manifest::Location for what these enums mean." That way, > if/when we change any, we don't have to update comments in multiple locations. Agree if we go this way. Not sure we should go this way: I wrote the mojo enum and traits out so we could see what they are like. I'm looking for the simplest thing that could possibly work. I'm happy to do what works best for you.
https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... File extensions/common/manifest_location.mojom (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... extensions/common/manifest_location.mojom:11: enum ManifestLocation { On 2017/03/08 17:56:09, noel gordon wrote: > On 2017/03/08 16:22:24, Devlin wrote: > > If it were me, I'd probably just opt for using [Native] enum ManifestLocation > > and adding IPC_ENUM_TRAITS_MIN and IPC_ENUM_TRAITS_MAX for the location, > > /me would have just stuck with the ints :) They seemed fine to me, and they > have been working for you for more than 5.5 years, without problem, when I > looked back through git blame. > > For a [Native] enum ManifestLocation, we'd need to define IPC_ENUM_TRAITS > somewhere (they don't already exist), somewhere outside of the -message.h file > (since my goal in a follow-up patch is to delete the -message.h file). > > > so that > > this is a lot less verbose (and probably a bit less error prone). > > Agree about full mojo enum traits being verbose (I had to type it all out, I'd > not done one before so it was good practice for me, but yeah, verbose). sammc@ > has been thinking about how we (mojo) can make them better. > > > But I suppose > > it's largely an IPC decision, so I'll defer to dcheng. > > Hmmmm. While I was writing them, I was thinking about you and your team, and > how y'all are going to manage them in future, do they add maintenance burden, > how easy are they change and update, and so on. I'm don't feel it's largely an > IPC decision when I consider those other factors as well. > > It's good we explore the issues: using ints is KISS to me, others have other > view-points. My vote would be to add IPC_ENUM_TRAITS and use native. It's a lot simpler and gives us the same benefit, and reduces the risk of introduces a mismatch between the enum conversion and the enum values (and has no maintenance burden, since invalid and num locations are always the first/last values). Additionally, the main benefit that having full mojo type traits is having language agnosticism, but these enums are only ever used in C++. Since sammc@ and rockot@ are investigating how to make these cleaner in the future, I'd say we do the IPC_ENUM_TRAITS + [Native] for now, and update to these once they figure it out. :) Re just sticking with ints, it's true that it has worked in the past, but it's not the right thing to do - enums should be passed as enums, not as ints. Also, as you mentioned, the messages were originally added 5.5 years ago - but IPC enum validation is only 3.75 years old. :P [1] It's good practice to make sure this is safe. [1] https://chromiumcodereview.appspot.com/15841011
lgtm
lgtm https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... File extensions/utility/utility_handler.cc (right): https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... extensions/utility/utility_handler.cc:59: CHECK_NE(location, Manifest::INVALID_LOCATION); On 2017/03/08 17:56:09, noel gordon wrote: > On 2017/03/08 16:22:24, Devlin wrote: > > On 2017/03/08 13:44:26, noel gordon wrote: > > > On 2017/03/07 09:45:14, dcheng wrote: > > > > I would suggest DCHECK_NE here, as we should be able to trust what the > > browser > > > > process sends us. > > > > > > See https://codereview.chromium.org/2711513003 > > > > > > Turns out this CHECK recently fired in the field a bit too often. The CL > added > > > CHECKs on the browser-side, the underlying issue is still being > investigated. > > > > > > While those browser-side CHECK's are there, I'm ok with DCHECK_NE here as > you > > > suggested. But note: we'd need to change it back to CHECK_NE if those > > > browser-side CHECKs are removed. > > > > > > Maybe safer to just leave it as a CHECK_NE for now, thoughts? > > > > As Noel points out, this has been a problem lately and we're looking into it. > > I'd like to keep the CHECK in place, and I'll update/remove it once we solve > the > > issue. > > My vote is leave the CHECK. OK, I'm OK with that as long as we figure out what's going on and can make this a debug assertion after that.
lgtm https://codereview.chromium.org/2697463002/diff/300001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:671: const std::unique_ptr<base::DictionaryValue>& original_manifest, This is weird. Either take a std::unique_ptr<base::DictionaryValue> or a const base::DictionaryValue&.
Patchset #14 (id:320001) has been deleted
PTAL https://codereview.chromium.org/2697463002/diff/300001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:671: const std::unique_ptr<base::DictionaryValue>& original_manifest, On 2017/03/09 08:50:18, Sam McNally wrote: > This is weird. Either take a std::unique_ptr<base::DictionaryValue> or a const > base::DictionaryValue&. Done, takes a std::unique_ptr<base::DictionaryValue, that makes https://crbug.com/699528 easier to resolve. https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... File extensions/common/manifest_location.mojom (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/common/mani... extensions/common/manifest_location.mojom:11: enum ManifestLocation { On 2017/03/08 18:05:17, Devlin wrote: > On 2017/03/08 17:56:09, noel gordon wrote: > > On 2017/03/08 16:22:24, Devlin wrote: > > > If it were me, I'd probably just opt for using [Native] enum > ManifestLocation > > > and adding IPC_ENUM_TRAITS_MIN and IPC_ENUM_TRAITS_MAX for the location, > > > > /me would have just stuck with the ints :) They seemed fine to me, and they > > have been working for you for more than 5.5 years, without problem, when I > > looked back through git blame. > > > > For a [Native] enum ManifestLocation, we'd need to define IPC_ENUM_TRAITS > > somewhere (they don't already exist), somewhere outside of the -message.h file > > (since my goal in a follow-up patch is to delete the -message.h file). > > > > > so that > > > this is a lot less verbose (and probably a bit less error prone). > > > > Agree about full mojo enum traits being verbose (I had to type it all out, I'd > > not done one before so it was good practice for me, but yeah, verbose). > sammc@ > > has been thinking about how we (mojo) can make them better. > > > > > But I suppose > > > it's largely an IPC decision, so I'll defer to dcheng. > > > > Hmmmm. While I was writing them, I was thinking about you and your team, and > > how y'all are going to manage them in future, do they add maintenance burden, > > how easy are they change and update, and so on. I'm don't feel it's largely > an > > IPC decision when I consider those other factors as well. > > > > It's good we explore the issues: using ints is KISS to me, others have other > > view-points. > > My vote would be to add IPC_ENUM_TRAITS and use native. It's a lot simpler and > gives us the same benefit, and reduces the risk of introduces a mismatch between > the enum conversion and the enum values (and has no maintenance burden, since > invalid and num locations are always the first/last values). Additionally, the > main benefit that having full mojo type traits is having language agnosticism, > but these enums are only ever used in C++. Since sammc@ and rockot@ are > investigating how to make these cleaner in the future, I'd say we do the > IPC_ENUM_TRAITS + [Native] for now, and update to these once they figure it out. > :) Sounds like a plan to me, and done. > Re just sticking with ints, it's true that it has worked in the past, but it's > not the right thing to do - enums should be passed as enums, not as ints. Also, > as you mentioned, the messages were originally added 5.5 years ago - but IPC > enum validation is only 3.75 years old. :P [1] It's good practice to make sure > this is safe. Thanks for the link, enum validation is more recent thing. I made the enums for our case validating with the limits you suggested, and also brought back the two CHECKs on the utility process side. > [1] https://chromiumcodereview.appspot.com/15841011
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2697463002/diff/300001/extensions/common/exte... File extensions/common/extension_utility_types.h (right): https://codereview.chromium.org/2697463002/diff/300001/extensions/common/exte... extensions/common/extension_utility_types.h:13: #include "ui/gfx/ipc/skia/gfx_skia_param_traits.h" On 2017/03/08 17:56:09, noel gordon wrote: > On 2017/03/08 16:22:24, Devlin wrote: > > needed? > > Not sure, will check this tomorrow. Yeap, needed. I added documentation to the .mojom about it, to do with writing decoded images during unpacking in some special file as a pickle of the DecodeImage type.
Some build funk on the bots, not sure what's up, compiles fine locally for me on win VS2015
My mojo knowledge is limited, so sammc can comment better than I, but the manifest_location_param_traits.cc file looks a bit odd? https://codereview.chromium.org/2697463002/diff/340001/extensions/common/mani... File extensions/common/manifest_location_param_traits.cc (right): https://codereview.chromium.org/2697463002/diff/340001/extensions/common/mani... extensions/common/manifest_location_param_traits.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/2697463002/diff/340001/extensions/common/mani... extensions/common/manifest_location_param_traits.cc:13: #undef EXTENSIONS_COMMON_MANIFEST_LOCATION_PARAM_TRAITS_H_ what does all this do? Why do we repeat it 4x?
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/09 15:52:12, Devlin wrote:
> My mojo knowledge is limited, so sammc can comment better than I, but the
> manifest_location_param_traits.cc file looks a bit odd?
This is to do with IPC I found (not so much mojo). The IPC ENUM macros were new
to me, never used them, and it turned out to be an odd adventure.
Initially, I wrote only the .h first, adding this part:
IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location,
extensions::Manifest::INVALID_LOCATION,
extensions::Manifest::NUM_LOCATIONS)
and hoped that'd be it. Nope, on compile, got undefined references to IPC
ParamTraits routines Read<T> / Write<T> / GetSize<T> / Log<T> with template
parameter T = extensions::Manifest::Location.
Damn: seems I have to write all those routines myself, just like mojo enum
struct_traits!
>
https://codereview.chromium.org/2697463002/diff/340001/extensions/common/mani...
> File extensions/common/manifest_location_param_traits.cc (right):
>
>
https://codereview.chromium.org/2697463002/diff/340001/extensions/common/mani...
> extensions/common/manifest_location_param_traits.cc:1: // Copyright (c) 2017
The
> Chromium Authors. All rights reserved.
> nit: no (c)
Fixed.
>
https://codereview.chromium.org/2697463002/diff/340001/extensions/common/mani...
> extensions/common/manifest_location_param_traits.cc:13: #undef
> EXTENSIONS_COMMON_MANIFEST_LOCATION_PARAM_TRAITS_H_
> what does all this do? Why do we repeat it 4x?
Referred the mojo docs; they said I needed to write the GetSize() param trait
code only. So ok I did that, but still undefined references
to 3 routines (Read / Write / Log) on compile.
I poked around the code base looking at param_traits, and found the clue: a good
example of it which I just looked up
https://cs.chromium.org/chromium/src/cc/ipc/cc_param_traits.cc?q=ipc/param_tr...
Notice the same pattern?
#include "ipc/param_traits_size_macros.h"
#include "ipc/param_traits_write_macros.h"
#include "ipc/param_traits_read_macros.h"
#include "ipc/param_traits_log_macros.h"
Same names as the routines I'm missing. /light bulb goes on.
Reading those includes, if you do this odd-looking thing 4 times, the includes
_auto-generate the ParamTraits<T> C++ code_ needed to send/receive type T over
IPC when T is an enumeration: GetSize, Write, Read, Log.
Once I did that, no more undefined references and chrome compiled :)
Admit it looks pretty wacky, but it writes to code for me. I eventually found
the same pattern all over the code base.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #17 (id:400001) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Build worked everywhere except win, dunno way. Plan B: put the ipc enum in the message file and try again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
^^^ Should work, but does not in windows. /me puts on x-ray goggles ◕‿◕
A) Add IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location,
extensions::Manifest::INVALID_LOCATION,
extensions::Manifest::NUM_LOCATIONS)
to the extensions_utility_message.h. Should work out-of-the-box, but the
windows compilers all complain: multiple definitions of IPC
ParamTraits<Manifest::Location> Write() / Read() pickle code. ¯\(ツ)/¯
B) BUILD.gn //extensions//common:common section
# 2) Due to 1), this auto generates ParamTraits<Manifest::Location> pickle code
"extension_message_generator.cc",
"extension_message_generator.h",
# 3) Weird, already defines struct ParamTraits<Manifest::Location>, who knew?
but the WIN compilers correctly spot the error: multiple definitions.
"extension_messages.cc",
"extension_messages.h",
"extension_paths.cc",
"extension_paths.h",
"extension_resource.cc",
"extension_resource.h",
"extension_set.cc",
"extension_set.h",
"extension_urls.cc",
"extension_urls.h",
# 1) noel@ declares IPC enum ParamTraits<Manifest::Location> pickle code with
the macro IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location ...)
"extension_utility_messages.h",
3) above is the problem, struct ParamTraits<Manifest::Location> IPC pickle has
existed (was renamed a few times) as far back as 2011 but the IPC we are mojo'n
here never used it, it used an _int_ instead ... ಠ-ಠ
-// Tells the utility process to validate and sanitize the extension in a
-// directory.
-IPC_MESSAGE_CONTROL4(ExtensionUtilityMsg_UnpackExtension,
- base::FilePath /* directory_path */,
- std::string /* extension_id */,
- int /* Manifest::Location */, **** used int ****
- int /* InitFromValue flags */)
4) Nice puzzle. Fix seems to be: declare IPC_ENUM_TRAITS_MIN_MAX_VALUE in
extension_messages.h and make the mojo .typemap read that file. Update
extension_messages.cc struct ParamTraits<Manifest::Location> code, make it
validating, make it interwork with crbug.com/692069, etc ... and maybe we'll be
happy koalas ʕᵔᴥᵔʔ ...
On 2017/03/10 02:38:28, noel gordon wrote: > ^^^ Should work, but does not in windows. /me puts on x-ray goggles ◕‿◕ > > A) Add IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location, > extensions::Manifest::INVALID_LOCATION, > extensions::Manifest::NUM_LOCATIONS) > > to the extensions_utility_message.h. Should work out-of-the-box, but the > windows compilers all complain: multiple definitions of IPC > ParamTraits<Manifest::Location> Write() / Read() pickle code. ¯\(ツ)/¯ > > B) BUILD.gn //extensions//common:common section > > # 2) Due to 1), this auto generates ParamTraits<Manifest::Location> pickle code > "extension_message_generator.cc", > "extension_message_generator.h", > # 3) Weird, already defines struct ParamTraits<Manifest::Location>, who knew? > but the WIN compilers correctly spot the error: multiple definitions. > "extension_messages.cc", > "extension_messages.h", > "extension_paths.cc", > "extension_paths.h", > "extension_resource.cc", > "extension_resource.h", > "extension_set.cc", > "extension_set.h", > "extension_urls.cc", > "extension_urls.h", > # 1) noel@ declares IPC enum ParamTraits<Manifest::Location> pickle code with > the macro IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location ...) > "extension_utility_messages.h", > > 3) above is the problem, struct ParamTraits<Manifest::Location> IPC pickle has > existed (was renamed a few times) as far back as 2011 but the IPC we are mojo'n > here never used it, it used an _int_ instead ... ಠ-ಠ > > -// Tells the utility process to validate and sanitize the extension in a > -// directory. > -IPC_MESSAGE_CONTROL4(ExtensionUtilityMsg_UnpackExtension, > - base::FilePath /* directory_path */, > - std::string /* extension_id */, > - int /* Manifest::Location */, **** used int **** > - int /* InitFromValue flags */) > > 4) Nice puzzle. Fix seems to be: declare IPC_ENUM_TRAITS_MIN_MAX_VALUE in > extension_messages.h and make the mojo .typemap read that file. Update > extension_messages.cc struct ParamTraits<Manifest::Location> code, make it > validating, make it interwork with crbug.com/692069, etc ... and maybe we'll be > happy koalas ʕᵔᴥᵔʔ ... You can declare the Mojo enum with [Native] to just reuse ParamTraits if it already exists, FWIW.
On 2017/03/10 02:46:43, dcheng wrote: > On 2017/03/10 02:38:28, noel gordon wrote: > > ^^^ Should work, but does not in windows. /me puts on x-ray goggles ◕‿◕ > > > > A) Add IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location, > > extensions::Manifest::INVALID_LOCATION, > > extensions::Manifest::NUM_LOCATIONS) > > > > to the extensions_utility_message.h. Should work out-of-the-box, but the > > windows compilers all complain: multiple definitions of IPC > > ParamTraits<Manifest::Location> Write() / Read() pickle code. ¯\(ツ)/¯ > > > > B) BUILD.gn //extensions//common:common section > > > > # 2) Due to 1), this auto generates ParamTraits<Manifest::Location> pickle > code > > "extension_message_generator.cc", > > "extension_message_generator.h", > > # 3) Weird, already defines struct ParamTraits<Manifest::Location>, who knew? > > but the WIN compilers correctly spot the error: multiple definitions. > > "extension_messages.cc", > > "extension_messages.h", > > "extension_paths.cc", > > "extension_paths.h", > > "extension_resource.cc", > > "extension_resource.h", > > "extension_set.cc", > > "extension_set.h", > > "extension_urls.cc", > > "extension_urls.h", > > # 1) noel@ declares IPC enum ParamTraits<Manifest::Location> pickle code with > > the macro IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location ...) > > "extension_utility_messages.h", > > > > 3) above is the problem, struct ParamTraits<Manifest::Location> IPC pickle has > > existed (was renamed a few times) as far back as 2011 but the IPC we are > mojo'n > > here never used it, it used an _int_ instead ... ಠ-ಠ > > > > -// Tells the utility process to validate and sanitize the extension in a > > -// directory. > > -IPC_MESSAGE_CONTROL4(ExtensionUtilityMsg_UnpackExtension, > > - base::FilePath /* directory_path */, > > - std::string /* extension_id */, > > - int /* Manifest::Location */, **** used int **** > > - int /* InitFromValue flags */) > > > > 4) Nice puzzle. Fix seems to be: declare IPC_ENUM_TRAITS_MIN_MAX_VALUE in > > extension_messages.h and make the mojo .typemap read that file. Update > > extension_messages.cc struct ParamTraits<Manifest::Location> code, make it > > validating, make it interwork with crbug.com/692069, etc ... and maybe we'll > be > > happy koalas ʕᵔᴥᵔʔ ... > > You can declare the Mojo enum with [Native] to just reuse ParamTraits if it > already exists, FWIW. Oh never mind, I didn't look at the latest CL before replying.
https://codereview.chromium.org/2697463002/diff/420001/extensions/common/exte... File extensions/common/extension_utility_types.h (right): https://codereview.chromium.org/2697463002/diff/420001/extensions/common/exte... extensions/common/extension_utility_types.h:15: using DecodedImages = std::vector<std::tuple<SkBitmap, base::FilePath>>; Nit: this should be in a namespace
https://codereview.chromium.org/2697463002/diff/420001/extensions/common/exte... File extensions/common/extension_utility_messages.h (right): https://codereview.chromium.org/2697463002/diff/420001/extensions/common/exte... extensions/common/extension_utility_messages.h:19: extensions::Manifest::NUM_LOCATIONS) Also, this should be NUM_LOCATIONS - 1, since NUM_LOCATIONS is a sentinel, not an actual valid enum value. For IPC, we usually call this MAX and assign it the last valid value in the enum, but I guess that's not easy to do here, since NUM_LOCATIONS is used for UMA. Oh well.
lgtm
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #18 (id:440001) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #20 (id:500001) has been deleted
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #16 (id:380001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/10 02:47:14, dcheng wrote: > On 2017/03/10 02:46:43, dcheng wrote: > > On 2017/03/10 02:38:28, noel gordon wrote: > > > ^^^ Should work, but does not in windows. /me puts on x-ray goggles ◕‿◕ > > > > > > A) Add IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location, > > > extensions::Manifest::INVALID_LOCATION, > > > extensions::Manifest::NUM_LOCATIONS) > > > > > > to the extensions_utility_message.h. Should work out-of-the-box, but the > > > windows compilers all complain: multiple definitions of IPC > > > ParamTraits<Manifest::Location> Write() / Read() pickle code. ¯\(ツ)/¯ > > > > > > B) BUILD.gn //extensions//common:common section > > > > > > # 2) Due to 1), this auto generates ParamTraits<Manifest::Location> pickle > > code > > > "extension_message_generator.cc", > > > "extension_message_generator.h", > > > # 3) Weird, already defines struct ParamTraits<Manifest::Location>, who > knew? > > > but the WIN compilers correctly spot the error: multiple definitions. > > > "extension_messages.cc", > > > "extension_messages.h", > > > "extension_paths.cc", > > > "extension_paths.h", > > > "extension_resource.cc", > > > "extension_resource.h", > > > "extension_set.cc", > > > "extension_set.h", > > > "extension_urls.cc", > > > "extension_urls.h", > > > # 1) noel@ declares IPC enum ParamTraits<Manifest::Location> pickle code > with > > > the macro IPC_ENUM_TRAITS_MIN_MAX_VALUE(extensions::Manifest::Location > ...) > > > "extension_utility_messages.h", > > > > > > 3) above is the problem, struct ParamTraits<Manifest::Location> IPC pickle > has > > > existed (was renamed a few times) as far back as 2011 but the IPC we are > > mojo'n > > > here never used it, it used an _int_ instead ... ಠ-ಠ > > > > > > -// Tells the utility process to validate and sanitize the extension in a > > > -// directory. > > > -IPC_MESSAGE_CONTROL4(ExtensionUtilityMsg_UnpackExtension, > > > - base::FilePath /* directory_path */, > > > - std::string /* extension_id */, > > > - int /* Manifest::Location */, **** used int **** > > > - int /* InitFromValue flags */) > > > > > > 4) Nice puzzle. Fix seems to be: declare IPC_ENUM_TRAITS_MIN_MAX_VALUE in > > > extension_messages.h and make the mojo .typemap read that file. Update > > > extension_messages.cc struct ParamTraits<Manifest::Location> code, make it > > > validating, make it interwork with crbug.com/692069, etc ... and maybe we'll > > be > > > happy koalas ʕᵔᴥᵔʔ ... > > > > You can declare the Mojo enum with [Native] to just reuse ParamTraits if it > > already exists, FWIW. > > Oh never mind, I didn't look at the latest CL before replying. No worries: turns out it was not as simple as expected, viz., just declaring IPC_ENUM_TRAITS_MIN_MAX_VALUE in a message file should work out-of-the-box. But it did not in this case (due to other factors which I will attempt to explain later).
On 2017/03/10 02:48:47, dcheng wrote: > https://codereview.chromium.org/2697463002/diff/420001/extensions/common/exte... > File extensions/common/extension_utility_types.h (right): > > https://codereview.chromium.org/2697463002/diff/420001/extensions/common/exte... > extensions/common/extension_utility_types.h:15: using DecodedImages = > std::vector<std::tuple<SkBitmap, base::FilePath>>; > Nit: this should be in a namespace Done in patch #21, also forward declared SkBitmap and base::FilePath in that file.
On 2017/03/10 02:51:05, dcheng wrote: > https://codereview.chromium.org/2697463002/diff/420001/extensions/common/exte... > File extensions/common/extension_utility_messages.h (right): > > https://codereview.chromium.org/2697463002/diff/420001/extensions/common/exte... > extensions/common/extension_utility_messages.h:19: > extensions::Manifest::NUM_LOCATIONS) > Also, this should be NUM_LOCATIONS - 1, since NUM_LOCATIONS is a sentinel, not > an actual valid enum value. > > For IPC, we usually call this MAX and assign it the last valid value in the > enum, Think this code might pre-date the use of MAX. > but I guess that's not easy to do here, since NUM_LOCATIONS is used for > UMA. Oh well. I also think the limit should be NUM_LOCATIONS - 1, but as you note, NUM_LOCATIONS is used in UMA.
The pre-exiting ParamTraits<extensions::Manifest::Location> defined in extension_messages.cc caused the multiple definitions error that the windows compilers complained about after I added the IPC_ENUM_TRAITS_MIN_MAX_VALUE. Patch #16 Some other IPC message uses these param traits for the location, ExtensionMsg_Loaded_Params [1] [1] https://cs.chromium.org/search/?q=extension_messages.cc+ExtensionMsg_Loaded_P... Removing those custom traits allowed IPC_ENUM_TRAITS_MIN_MAX_VALUE I added to be both the declarer and definer of them. Declare them in a .h file, and then whatever .h file you put them in has to processed 4 times to produce the GetSize, Write, Read, and Log param traits code. That was patches #17 #18. Compiled on all bots, but the bots where red still, complaining in the post-compile step, where ninja noticed that there were a lot of is_dirty targets after compilation. See any red bot in those patches reference: an example here ninja explain: output obj/extensions/common/mojo/extension_unpacker.mojom.o older than most recent input gen/third_party/WebKit/public/web/window_features.mojom.h (1489168308 vs 1489168316) ninja explain: obj/extensions/common/mojo/extension_unpacker.mojom.o is dirty ninja explain: obj/extensions/common/mojo/extension_unpacker.mojom.o is dirty ninja explain: obj/chrome/chrome_framework_shared_library/Chromium Framework is dirty ninja explain: Chromium Framework.framework/Versions/A/Chromium Framework is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: Chromium Framework.framework is dirty ninja explain: Chromium.app/Contents/Versions/59.0.3038.0/Chromium Framework.framework is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/verify_chrome_framework_order.inputdeps.stamp is dirty ninja explain: obj/chrome/run_verify_chrome_framework_order.stamp is dirty ninja explain: obj/chrome/verify_chrome_framework_order.stamp is dirty ninja explain: Chromium Framework.framework is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/verify_chrome_framework_order.stamp is dirty ninja explain: obj/chrome/chrome_versioned_bundle_data.inputdeps.stamp is dirty ninja explain: obj/chrome/chrome_app.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/verify_chrome_framework_order.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/extensions/common/mojo/extension_unpacker.mojom.o is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_app.stamp is dirty ninja explain: browser_tests is dirty ninja explain: obj/chrome/test/browser_tests_run.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/extensions/common/mojo/extension_unpacker.mojom.o is dirty ninja explain: chrome_app_unittests is dirty ninja explain: obj/chrome/test/chrome_app_unittests_run.stamp is dirty ninja explain: obj/extensions/common/mojo/extension_unpacker.mojom.o is dirty ninja explain: extensions_unittests is dirty ninja explain: obj/extensions/extensions_unittests_run.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/extensions/common/mojo/extension_unpacker.mojom.o is dirty ninja explain: interactive_ui_tests is dirty ninja explain: obj/chrome/test/interactive_ui_tests_run.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/extensions/common/mojo/extension_unpacker.mojom.o is dirty ninja explain: sync_integration_tests is dirty ninja explain: obj/chrome/test/sync_integration_tests_run.stamp is dirty ninja explain: browser_tests is dirty ninja explain: obj/chrome/test/tab_capture_end2end_tests_run.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: obj/extensions/common/mojo/extension_unpacker.mojom.o is dirty ninja explain: obj/chrome/chrome_framework.stamp is dirty ninja explain: unit_tests is dirty ninja explain: obj/chrome/test/unit_tests_run.stamp is dirty
So why all these is_dirty targets? Turns out that the BUILD.on files that use the //extensions/common:common target had either an incorrect dependency, or no direct dependency, on that target. Ninja was right to complain and adding the mojo revealed the problem, under-specified BUILD files. So I made the BUILD.gn files users of //extensions/common:common (and its :mojo therefore), have a dependency on that target. Result: patch set #20, no more is-dirty target complaints from ninja, and all green bots.
On 2017/03/13 12:55:38, noel gordon wrote: > On 2017/03/10 02:51:05, dcheng wrote: > > > but I guess that's not easy to do here, since NUM_LOCATIONS is used for > > UMA. Oh well. > > I also think the limit should be NUM_LOCATIONS - 1, but as you note, > NUM_LOCATIONS is used in UMA. So after fixing all the BUID.gn file hoohah, next question: should it be NUM_LOCATIONS - 1, or NUM_LOCATIONS? The custom param traits I removed had this [1] [1] https://cs.chromium.org/chromium/src/extensions/common/extension_messages.cc?... if (!ReadParam(m, iter, &val) || val < Manifest::INVALID_LOCATION || val >= Manifest::NUM_LOCATIONS) return false; ... So Manifest::NUM_LOCATIONS was considered invalid, so I think it should be NUM_LOCATIONS - 1. I'm uploading a patch to do that (Patch set #22) Please let me know if it looks right.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/13 13:32:10, noel gordon wrote: > So Manifest::NUM_LOCATIONS was considered invalid, so I think it should be > NUM_LOCATIONS - 1. I'm uploading a patch to do that (Patch set #22) Please let > me know if it looks right. Catc^h^h^h^hPatch 22 uploaded.
On 2017/03/13 13:40:23, noel gordon wrote: > On 2017/03/13 13:32:10, noel gordon wrote: > > > So Manifest::NUM_LOCATIONS was considered invalid, so I think it should be > > NUM_LOCATIONS - 1. I'm uploading a patch to do that (Patch set #22) Please > let > > me know if it looks right. > > Catc^h^h^h^hPatch 22 uploaded. Also to pre-empt any question about why put the manifest location param traits in them in their own file? Well the answer is mojo can parse and generate the code it needs to support your [Native] enum from that input quickly. Put it in the extensions-message.h file, and the mojo code generation step becomes noticeably slower (about 1 second, perhaps because the message file is relatively large, it's ~1000 lines long).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Convert utility process extension Unpacker IPC to mojo BUG=691410 ========== to ========== Convert utility process extension Unpacker IPC to mojo Add ExtensionUnpacker mojom service used to unzip and unpack an extension. Expose to the browser via the utility process policy file. Update clients to invoke the service using a mojo utility client; remove all dependencies on UtilityProcessHostClient. Add [Native] IPC enum param traits, via mojo typemap, to handle extensions::Manifest::Location. Set the wire limit for the enum value to one less than Manifest::NUM_LOCATIONS (for compat with the pre-existing custom traits removed in this patch since this patch now auto-generates these traits). In the extensions/utility/utility_handler.cc, retain the CHECKs on the Manifest::Location enum limits while bug 692069 is still being investigated. Move DecodeImages declarations from the extension messages file into its own file (extension_utility_types.h) to decouple those declarations from this messages file (a follow-up patch deletes the messages file). Update all BUILD.gn for using clients to directly depend on the //extensions/common:common target (and hence, its :mojo) to fix post-compile step failures (warning about under-specified BUILD files with "targets is_dirty errors"). Add TestContentBrowserClient for the extension test harness and add a test overlay file in test/data (to expose the new mojo to extension tests). Covered by existing unittest: zipfile_installer_unittest.cc and sandboxed_unpacker_unittest.cc. BUG=691410 ==========
On 2017/03/09 01:03:32, dcheng wrote: > lgtm > > https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... > File extensions/utility/utility_handler.cc (right): > > https://codereview.chromium.org/2697463002/diff/280001/extensions/utility/uti... > extensions/utility/utility_handler.cc:59: CHECK_NE(location, > Manifest::INVALID_LOCATION); > On 2017/03/08 17:56:09, noel gordon wrote: > > On 2017/03/08 16:22:24, Devlin wrote: > > > On 2017/03/08 13:44:26, noel gordon wrote: > > > > On 2017/03/07 09:45:14, dcheng wrote: > > > > > I would suggest DCHECK_NE here, as we should be able to trust what the > > > browser > > > > > process sends us. > > > > > > > > See https://codereview.chromium.org/2711513003 > > > > > > > > Turns out this CHECK recently fired in the field a bit too often. The CL > > added > > > > CHECKs on the browser-side, the underlying issue is still being > > investigated. > > > > > > > > While those browser-side CHECK's are there, I'm ok with DCHECK_NE here as > > you > > > > suggested. But note: we'd need to change it back to CHECK_NE if those > > > > browser-side CHECKs are removed. > > > > > > > > Maybe safer to just leave it as a CHECK_NE for now, thoughts? > > > > > > As Noel points out, this has been a problem lately and we're looking into > it. > > > I'd like to keep the CHECK in place, and I'll update/remove it once we solve > > the > > > issue. > > > > My vote is leave the CHECK. > > OK, I'm OK with that as long as we figure out what's going on and can make this > a debug assertion after that. Note there is a bug (692069) covering the problem already, and Devlin@ mentioned he'd deal with any CHECK/DCHECK clean-up (just above) once the problem is solved.
Description was changed from
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages file (a follow-up patch deletes
the messages file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty errors").
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (to expose the new mojo to
extension tests).
Covered by existing unittest: zipfile_installer_unittest.cc and
sandboxed_unpacker_unittest.cc.
BUG=691410
==========
to
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages file (a follow-up patch deletes
the messages file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty errors").
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (to expose the mojo to the
extension tests system).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
The CQ bit was checked by noel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, rdevlin.cronin@chromium.org, jochen@chromium.org, dcheng@chromium.org, tibell@chromium.org Link to the patchset: https://codereview.chromium.org/2697463002/#ps580001 (title: "Set the IPC enum traits limit to extensions::Manifest::NUM_LOCATIONS - 1.")
The CQ bit was unchecked by noel@chromium.org
Description was changed from
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages file (a follow-up patch deletes
the messages file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty errors").
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (to expose the mojo to the
extension tests system).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
to
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages file (a follow-up patch deletes
the messages file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty" errors).
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (needed to expose the mojo
to the extension test system).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
noel@chromium.org changed reviewers: + ben@chromium.org
+ben for approval to add the typemap file and DEPS.
noel@chromium.org changed reviewers: + rockot@chromium.org - ben@chromium.org
-ben, +rockot
Description was changed from
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages file (a follow-up patch deletes
the messages file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty" errors).
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (needed to expose the mojo
to the extension test system).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
to
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file (a follow-up patch deletes
the messages file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty" errors).
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (needed to expose the mojo
to the extension test system).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
Description was changed from
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file (a follow-up patch deletes
the messages file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty" errors).
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (needed to expose the mojo
to the extension test system).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
to
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file (a follow-up patch deletes
the messages-file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty" errors).
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (needed to expose the mojo
to the extension test system).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
LGTM with uber-nitty terminology nit, at least in the CL description and some code comment(s): the term "service" gets overloaded a lot, and since services are a real thing and not the same thing as mojom interfaces, I think it's important to not call mojom interfaces "services".
Description was changed from
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojom service used to unzip and unpack an
extension. Expose to the browser via the utility process policy
file. Update clients to invoke the service using a mojo utility
client; remove all dependencies on UtilityProcessHostClient.
Add [Native] IPC enum param traits, via mojo typemap, to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch now auto-generates these traits).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file (a follow-up patch deletes
the messages-file).
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files with "targets is_dirty" errors).
Add TestContentBrowserClient for the extension test harness and
add a test overlay file in test/data (needed to expose the mojo
to the extension test system).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
to
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojo, used to (unzip and) unpack a chrome
extension, and expose it to the browser via the utility process
policy file. Make callers use a utility process mojo client.
Add [Native] IPC enum param traits via a mojo typemap to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch auto-generates them: manifest_location_param_traits.cc).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file. A follow-up patch deletes
the messages-file so DecodeImages needs a new home.
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files via "targets is_dirty" errors, see review comment #169).
Add TestContentBrowserClient to the extension test harness, and
also an overlay file in test/data (needed to expose the mojo in
the extension test harness to the unit tests listed below).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
Removed "service" from the CL description. Agree the term is overloaded. Indeed, searching the CL patch diff here turns up many references to some "service" thing that pre-dates mojo and it services (the real ones). Even the utility process mojo client I use only provides access to a mojo Interface with, for example utility_process_mojo_client_->service()->Unzip(...) though in fairness, the utility_process_mojo_client code pre-dates mojo "services". > services are a real thing and not the same thing as mojom interfaces Perhaps our documentation should pose that statement as a question, and provide the answers as to why [1]. [1] Not having dealt with a mojo service as yet, I don't think I know the correct answers either.
Yep, I want to get rid of the "utility process mojo client" - it's not really an abstraction we want hanging around. Clients shouldn't even think of the term "utility process", they should just be connecting to services. Soon... On Wed, Mar 15, 2017 at 8:42 PM, <noel@chromium.org> wrote: > Removed "service" from the CL description. Agree the term is overloaded. > Indeed, searching the CL patch diff here turns up many references to some > "service" thing that pre-dates mojo and it services (the real ones). Even > the > utility process mojo client I use only provides access to a mojo Interface > with, > for example > > utility_process_mojo_client_->service()->Unzip(...) > > though in fairness, the utility_process_mojo_client code pre-dates mojo > "services". > > > services are a real thing and not the same thing as mojom interfaces > > Perhaps our documentation should pose that statement as a question, and > provide > the answers as to why [1]. > > [1] Not having dealt with a mojo service as yet, I don't think I know the > correct answers either. > > > > https://codereview.chromium.org/2697463002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 580001, "attempt_start_ts": 1489650830136330,
"parent_rev": "9e099d4c84b02fe2a6ee52883090a974ceb8cfd1", "commit_rev":
"c8702c4cdecf0859c34a5ae564c89463ac727967"}
Message was sent while issue was closed.
Description was changed from
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojo, used to (unzip and) unpack a chrome
extension, and expose it to the browser via the utility process
policy file. Make callers use a utility process mojo client.
Add [Native] IPC enum param traits via a mojo typemap to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch auto-generates them: manifest_location_param_traits.cc).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file. A follow-up patch deletes
the messages-file so DecodeImages needs a new home.
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files via "targets is_dirty" errors, see review comment #169).
Add TestContentBrowserClient to the extension test harness, and
also an overlay file in test/data (needed to expose the mojo in
the extension test harness to the unit tests listed below).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
==========
to
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojo, used to (unzip and) unpack a chrome
extension, and expose it to the browser via the utility process
policy file. Make callers use a utility process mojo client.
Add [Native] IPC enum param traits via a mojo typemap to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch auto-generates them: manifest_location_param_traits.cc).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file. A follow-up patch deletes
the messages-file so DecodeImages needs a new home.
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files via "targets is_dirty" errors, see review comment #169).
Add TestContentBrowserClient to the extension test harness, and
also an overlay file in test/data (needed to expose the mojo in
the extension test harness to the unit tests listed below).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
Review-Url: https://codereview.chromium.org/2697463002
Cr-Commit-Position: refs/heads/master@{#457374}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8702c4cdecf0859c34a5ae564c8...
==========
Message was sent while issue was closed.
Committed patchset #22 (id:580001) as https://chromium.googlesource.com/chromium/src/+/c8702c4cdecf0859c34a5ae564c8...
Message was sent while issue was closed.
Description was changed from
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojo, used to (unzip and) unpack a chrome
extension, and expose it to the browser via the utility process
policy file. Make callers use a utility process mojo client.
Add [Native] IPC enum param traits via a mojo typemap to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch auto-generates them: manifest_location_param_traits.cc).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file. A follow-up patch deletes
the messages-file so DecodeImages needs a new home.
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files via "targets is_dirty" errors, see review comment #169).
Add TestContentBrowserClient to the extension test harness, and
also an overlay file in test/data (needed to expose the mojo in
the extension test harness to the unit tests listed below).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
Review-Url: https://codereview.chromium.org/2697463002
Cr-Commit-Position: refs/heads/master@{#457374}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8702c4cdecf0859c34a5ae564c8...
==========
to
==========
Convert utility process extension Unpacker IPC to mojo
Add ExtensionUnpacker mojo, used to (unzip and) unpack a chrome
extension, and expose it to the browser via the utility process
policy file. Make callers use a utility process mojo client.
Add [Native] IPC enum param traits via a mojo typemap to handle
extensions::Manifest::Location. Set the wire limit for the enum
value to one less than Manifest::NUM_LOCATIONS (for compat with
the pre-existing custom traits removed in this patch since this
patch auto-generates them: manifest_location_param_traits.cc).
In the extensions/utility/utility_handler.cc, retain the CHECKs
on the Manifest::Location enum limits while bug 692069 is still
being investigated.
Move DecodeImages declarations from the extension messages-file
into its own file (extension_utility_types.h) to decouple those
declarations from this messages-file. A follow-up patch deletes
the messages-file so DecodeImages needs a new home.
Update all BUILD.gn for using clients to directly depend on the
//extensions/common:common target (and hence, its :mojo) to fix
post-compile step failures (warning about under-specified BUILD
files via "targets is_dirty" errors, see review comment #169).
Add TestContentBrowserClient to the extension test harness, and
also an overlay file in test/data (needed to expose the mojo in
the extension test harness to the unit tests listed below).
Covered by existing unit tests:
{sandboxed_unpacker, zipfile_installer}_unittest.cc
BUG=691410
Review-Url: https://codereview.chromium.org/2697463002
Cr-Commit-Position: refs/heads/master@{#457374}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8702c4cdecf0859c34a5ae564c8...
==========
On 2017/03/16 03:43:56, Ken Rockot wrote: > Yep, I want to get rid of the "utility process mojo client" - it's not > really an abstraction we want hanging around. Clients shouldn't even think > of the term "utility process", they should just be connecting to services. > Soon... Nod. Can discuss in upcoming s13n meeting. |
