Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
316493002:
[Mac] Add interactive App Shim test. (Closed)
|
Created:
6 years, 6 months ago by jackhou1 Modified:
6 years, 6 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] Add interactive App Shim test.
This test creates shims in the user data dir and actually starts them up and
expects them to connect to the IPC socket.
BUG=168080, 386024
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276368
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277743
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277955
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address comments #
Total comments: 23
Patch Set 3 : Address comments #
Total comments: 12
Patch Set 4 : Address comments #Patch Set 5 : Address comments #Patch Set 6 : Sync and rebase. Fix a couple of comments. #Patch Set 7 : Disable quarantine on app shims. #
Total comments: 4
Patch Set 8 : RemoveQuarantineAttribute on both bundle and exe #Patch Set 9 : Sync and rebase #Patch Set 10 : Don't run test on component builds. #
Messages
Total messages: 51 (0 generated)
https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2013 -> 2014 https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:5: // General end-to-end test for app shims. nit: maybe move this down to the class declaration https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:29: #include "chrome/test/base/interactive_test_utils.h" did you end up using anything in here? https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:32: #include "ui/events/test/cocoa_test_event_utils.h" nit: import this one - the rest are agnostic I think https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:72: web_app::WebAppShortcutCreator shortcut_creator( nit: perhaps a comment: ~use the shortcut creator directly rather than web_app::CreatePlatformShortcuts() since that is disabled in tests. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:85: content::RunAllPendingInMessageLoop(); I think this will spin on the CPU, which isn't nice. That is, the UI message loop will be empty while the file thread is busy making shortcuts, so this will usually return immediately and choke the CPU. I think we might need to add test-only a base::Closure data member to WebAppShortcutCreator and at the end of WebAppShortcutCreator::CreateShortcuts(..) check the callback and post that callback to the UI thread. Here, you'd need add a base::RunLoop scoped_ptr data member to the AppShimInteractiveTest class. The callback would be something like void AppShimInteractiveTest::OnShortcutCreated() { DCHECK_CURRENTLY_ON(... UI); shortcut_created_ = true; if (run_loop_) run_loop_->Quit(); } void AppShimInteractiveTest::WaitForShortcutCreated() { if (shortcut_created_) return; run_loop_.reset(new base::RunLoop); run_loop->Run(); // Blocks until shortcut_created is set false. } https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:112: content::RunAllPendingInMessageLoop(); Same thing here - you can't spin the UI thread like this. Here I'd maybe make AppShimInteractiveTest implement AppLifetimeMonitor::Observer::OnAppStart(Profile*, app_id), then do a similar thing with the run loop above. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:124: content::RunAllPendingInMessageLoop(); For this one, you could create a scoped_nsobject child which asks to receive NSWorkspaceDidTerminateApplicationNotification via [[[NSWorkspace sharedWorkspace] notificationCenter] addObserver:foo selector:@selector(onApplicationTerminated:) name:NSWorkspaceDidTerminateApplicationNotification object:nil but a better option might be to get the PID from [NSRunningApplication processIdentifier] and use base::WaitForSingleProcess(), unless that prevents the UI thread doing other things we need. https://codereview.chromium.org/316493002/diff/1/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/316493002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.h:17: extern bool g_app_shims_enable_internal_for_test; This needs a comment https://codereview.chromium.org/316493002/diff/1/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/316493002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:929: if (AppShimsDisabledForTest()) should this one be guarded too? (or perhaps comment here why not) Or maybe better -- move the g_app_shims_enable_internal_for_test check into AppShimsDisabledForTest() {}
https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/06/03 07:32:39, tapted wrote: > nit: 2013 -> 2014 Done. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:5: // General end-to-end test for app shims. On 2014/06/03 07:32:39, tapted wrote: > nit: maybe move this down to the class declaration Done. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:29: #include "chrome/test/base/interactive_test_utils.h" On 2014/06/03 07:32:39, tapted wrote: > did you end up using anything in here? Nope. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:32: #include "ui/events/test/cocoa_test_event_utils.h" On 2014/06/03 07:32:39, tapted wrote: > nit: import this one - the rest are agnostic I think Done. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:72: web_app::WebAppShortcutCreator shortcut_creator( On 2014/06/03 07:32:39, tapted wrote: > nit: perhaps a comment: ~use the shortcut creator directly rather than > web_app::CreatePlatformShortcuts() since that is disabled in tests. This WebAppShortcutCreator is only used to get the path. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:85: content::RunAllPendingInMessageLoop(); On 2014/06/03 07:32:39, tapted wrote: > I think this will spin on the CPU, which isn't nice. That is, the UI message > loop will be empty while the file thread is busy making shortcuts, so this will > usually return immediately and choke the CPU. > > I think we might need to add test-only a base::Closure data member to > WebAppShortcutCreator and at the end of > WebAppShortcutCreator::CreateShortcuts(..) check the callback and post that > callback to the UI thread. > > Here, you'd need add a base::RunLoop scoped_ptr data member to the > AppShimInteractiveTest class. The callback would be something like > > void AppShimInteractiveTest::OnShortcutCreated() { > DCHECK_CURRENTLY_ON(... UI); > shortcut_created_ = true; > if (run_loop_) > run_loop_->Quit(); > } > > void AppShimInteractiveTest::WaitForShortcutCreated() { > if (shortcut_created_) > return; > run_loop_.reset(new base::RunLoop); > run_loop->Run(); // Blocks until shortcut_created is set false. > } > I do actually call web_app::UpdateAllShortcuts (rather than calling WebAppShortcutCreator::UpdateShortcuts directly). I've changed this to use FilePathWatcher. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:112: content::RunAllPendingInMessageLoop(); On 2014/06/03 07:32:39, tapted wrote: > Same thing here - you can't spin the UI thread like this. Here I'd maybe make > AppShimInteractiveTest implement > AppLifetimeMonitor::Observer::OnAppStart(Profile*, app_id), then do a similar > thing with the run loop above. The shim is not launched until after the app starts. But I can watch NSWorkspaceDidLaunchApplicationNotification. https://codereview.chromium.org/316493002/diff/1/apps/app_shim/app_shim_inter... apps/app_shim/app_shim_interactive_uitest_mac.mm:124: content::RunAllPendingInMessageLoop(); On 2014/06/03 07:32:39, tapted wrote: > For this one, you could create a scoped_nsobject child which asks to receive > > NSWorkspaceDidTerminateApplicationNotification via > > [[[NSWorkspace sharedWorkspace] notificationCenter] addObserver:foo > selector:@selector(onApplicationTerminated:) > name:NSWorkspaceDidTerminateApplicationNotification > object:nil > > > > but a better option might be to get the PID from [NSRunningApplication > processIdentifier] and use base::WaitForSingleProcess(), unless that prevents > the UI thread doing other things we need. Done. https://codereview.chromium.org/316493002/diff/1/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/316493002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.h:17: extern bool g_app_shims_enable_internal_for_test; On 2014/06/03 07:32:39, tapted wrote: > This needs a comment Done. https://codereview.chromium.org/316493002/diff/1/chrome/browser/web_applicati... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/316493002/diff/1/chrome/browser/web_applicati... chrome/browser/web_applications/web_app_mac.mm:929: if (AppShimsDisabledForTest()) On 2014/06/03 07:32:39, tapted wrote: > should this one be guarded too? (or perhaps comment here why not) > > Or maybe better -- move the g_app_shims_enable_internal_for_test check into > AppShimsDisabledForTest() {} Done.
https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:56: content::BrowserThread::FILE, nit: indent 2 more spaces https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:81: content::BrowserThread::UI, nit: indent https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:119: -(id) initForNotification:(NSString*)name; nit: spacing on all these should be like - (id)initFor... https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:129: addObserver:self nit: min 4 spaces indent https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:141: notificationReceived_ = YES; I think this will quit once any application is started - how hard is it to extract the application from the notification object and return early from here if it's not the shim? https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:176: new WindowedFilePathWatcher(shim_path); I think this is watching the .app root directory? I'm not sure it will guarantee all the contents of the directory have been copied in at that point, which might make the launch flaky. Typically only renames (of files or directories) are atomic on POSIX systems (and then only if the destination path already exists). Perhaps you could watch for the temp directory being deleted? (except the ScopedTempDir path isn't currently exposed). That is, in Observe, check each time whether PathExists(tempdir) and only stop the run loop when it's gone. https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:212: NSArray* running_shim = [NSRunningApplication git cl format might format this line differently too https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:220: [observer wait]; I *think* this is not racy, but it might need a comment. Something along the lines of ~"When the shim process receives a terminate request, it first sends NSTerminateLater, then waits for the IPC socket to be closed. This only only happens after ExtensionAppShimHandler::OnShimQuit has closed all windows for the app." https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:223: EXPECT_FALSE(HasAppShimHost(profile(), app->id())); add a matching EXPECT_TRUE(HasAppShimHost../window) for this block? https://codereview.chromium.org/316493002/diff/20001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/316493002/diff/20001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.h:19: // internal shim bundles, i.e. it does not create new shims in ~/Applications. oh cool - I actually didn't notice this the first time. Perhaps worth mentioning in the CL description (i.e. that it creates a shim in the test user data dir), since it currently just says it launches them :)
https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:56: content::BrowserThread::FILE, On 2014/06/04 09:36:03, tapted wrote: > nit: indent 2 more spaces Done. https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:81: content::BrowserThread::UI, On 2014/06/04 09:36:03, tapted wrote: > nit: indent Done. https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:119: -(id) initForNotification:(NSString*)name; On 2014/06/04 09:36:03, tapted wrote: > nit: spacing on all these should be like > > - (id)initFor... Done. https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:129: addObserver:self On 2014/06/04 09:36:03, tapted wrote: > nit: min 4 spaces indent Done. https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:141: notificationReceived_ = YES; On 2014/06/04 09:36:03, tapted wrote: > I think this will quit once any application is started - how hard is it to > extract the application from the notification object and return early from here > if it's not the shim? Done. https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:176: new WindowedFilePathWatcher(shim_path); On 2014/06/04 09:36:03, tapted wrote: > I think this is watching the .app root directory? I'm not sure it will guarantee > all the contents of the directory have been copied in at that point, which might > make the launch flaky. Typically only renames (of files or directories) are > atomic on POSIX systems (and then only if the destination path already exists). > > Perhaps you could watch for the temp directory being deleted? (except the > ScopedTempDir path isn't currently exposed). That is, in Observe, check each > time whether PathExists(tempdir) and only stop the run loop when it's gone. It's guaranteed but in a non-obvious way. Added a comment. https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:212: NSArray* running_shim = [NSRunningApplication On 2014/06/04 09:36:03, tapted wrote: > git cl format might format this line differently too Ran git cl format. It's ok with this, but it puts -> at the start of lines instead of the end. https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:220: [observer wait]; On 2014/06/04 09:36:03, tapted wrote: > I *think* this is not racy, but it might need a comment. Something along the > lines of ~"When the shim process receives a terminate request, it first sends > NSTerminateLater, then waits for the IPC socket to be closed. This only only > happens after ExtensionAppShimHandler::OnShimQuit has closed all windows for the > app." I think this is only fired when the shim actually terminates. The documentation says: "Posted when an app finishes executing." https://developer.apple.com/library/mac/documentation/cocoa/reference/applica... https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:223: EXPECT_FALSE(HasAppShimHost(profile(), app->id())); On 2014/06/04 09:36:03, tapted wrote: > add a matching EXPECT_TRUE(HasAppShimHost../window) for this block? Looks like I need to wait for the shim to connect with ExtensionAppShimHandler. More WindowedSomethingObservers! https://codereview.chromium.org/316493002/diff/20001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/316493002/diff/20001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.h:19: // internal shim bundles, i.e. it does not create new shims in ~/Applications. On 2014/06/04 09:36:03, tapted wrote: > oh cool - I actually didn't notice this the first time. Perhaps worth mentioning > in the CL description (i.e. that it creates a shim in the test user data dir), > since it currently just says it launches them :) Done.
I think it all lg, just need to do one more pass over the full diff. But here are some comments https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:176: new WindowedFilePathWatcher(shim_path); On 2014/06/05 02:57:11, jackhou1 wrote: > On 2014/06/04 09:36:03, tapted wrote: > > I think this is watching the .app root directory? I'm not sure it will > guarantee > > all the contents of the directory have been copied in at that point, which > might > > make the launch flaky. Typically only renames (of files or directories) are > > atomic on POSIX systems (and then only if the destination path already > exists). > > > > Perhaps you could watch for the temp directory being deleted? (except the > > ScopedTempDir path isn't currently exposed). That is, in Observe, check each > > time whether PathExists(tempdir) and only stop the run loop when it's gone. > > It's guaranteed but in a non-obvious way. Added a comment. Ah - cool makes sense. FilePathWatcher's documentation about what happens on what thread isn't super clear :/ https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:220: [observer wait]; On 2014/06/05 02:57:11, jackhou1 wrote: > On 2014/06/04 09:36:03, tapted wrote: > > I *think* this is not racy, but it might need a comment. Something along the > > lines of ~"When the shim process receives a terminate request, it first sends > > NSTerminateLater, then waits for the IPC socket to be closed. This only only > > happens after ExtensionAppShimHandler::OnShimQuit has closed all windows for > the > > app." > > I think this is only fired when the shim actually terminates. The documentation > says: > "Posted when an app finishes executing." > https://developer.apple.com/library/mac/documentation/cocoa/reference/applica... Yep - I just meant it's subtle that [NSRunnignApplicaiton terminate] on the line above does not immediately terminate like it would "by default". So a comment about the shim sending NSTerminateLater would make it clear that this will not be racy. https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:80: observed_ = true; since this memory read on the UIThread, it shouldn't be written on the FileThread, but I think this line can just be moved to StopRunLoop. To be more explicit, you could rename. Something like Observe -> ObserveOnFileThread, StopRunLoop -> ObserveOnUIThread, but the names are probably fine as is https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:103: WindowedAppShimLaunchObserver(const std::string app_id) nit: reference&
https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/20001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:220: [observer wait]; On 2014/06/05 04:08:01, tapted wrote: > On 2014/06/05 02:57:11, jackhou1 wrote: > > On 2014/06/04 09:36:03, tapted wrote: > > > I *think* this is not racy, but it might need a comment. Something along the > > > lines of ~"When the shim process receives a terminate request, it first > sends > > > NSTerminateLater, then waits for the IPC socket to be closed. This only only > > > happens after ExtensionAppShimHandler::OnShimQuit has closed all windows for > > the > > > app." > > > > I think this is only fired when the shim actually terminates. The > documentation > > says: > > "Posted when an app finishes executing." > > > https://developer.apple.com/library/mac/documentation/cocoa/reference/applica... > > Yep - I just meant it's subtle that [NSRunnignApplicaiton terminate] on the line > above does not immediately terminate like it would "by default". So a comment > about the shim sending NSTerminateLater would make it clear that this will not > be racy. Done. https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:80: observed_ = true; On 2014/06/05 04:08:01, tapted wrote: > since this memory read on the UIThread, it shouldn't be written on the > FileThread, but I think this line can just be moved to StopRunLoop. > > To be more explicit, you could rename. Something like Observe -> > ObserveOnFileThread, StopRunLoop -> ObserveOnUIThread, but the names are > probably fine as is Done. https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:103: WindowedAppShimLaunchObserver(const std::string app_id) On 2014/06/05 04:08:01, tapted wrote: > nit: reference& Done.
lgtm with those changes https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:104: : app_mode_id_(app_id) { observed_ needs an initializer https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:120: // Remove self and pass through. nit: ...pass through to the default handler. https://codereview.chromium.org/316493002/diff/40001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/316493002/diff/40001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.h:17: // Whether to enable update and launch of of app shims in tests. (Normally shims nit: repeated "of" https://codereview.chromium.org/316493002/diff/40001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/316493002/diff/40001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:48: '../apps/app_shim/app_shim_interactive_uitest_mac.mm', nit: sorting
https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... File apps/app_shim/app_shim_interactive_uitest_mac.mm (right): https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:104: : app_mode_id_(app_id) { On 2014/06/05 05:20:12, tapted wrote: > observed_ needs an initializer Done. https://codereview.chromium.org/316493002/diff/40001/apps/app_shim/app_shim_i... apps/app_shim/app_shim_interactive_uitest_mac.mm:120: // Remove self and pass through. On 2014/06/05 05:20:12, tapted wrote: > nit: ...pass through to the default handler. Done. https://codereview.chromium.org/316493002/diff/40001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_mac.h (right): https://codereview.chromium.org/316493002/diff/40001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_mac.h:17: // Whether to enable update and launch of of app shims in tests. (Normally shims On 2014/06/05 05:20:12, tapted wrote: > nit: repeated "of" Done. https://codereview.chromium.org/316493002/diff/40001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/316493002/diff/40001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:48: '../apps/app_shim/app_shim_interactive_uitest_mac.mm', On 2014/06/05 05:20:12, tapted wrote: > nit: sorting Done.
thakis, please review for OWNERS: chrome/common/mac/app_mode_common.h chrome/common/mac/app_mode_common.mm
Oops thakis is still OOO. rsesek, could you please review these for for OWNERS: chrome/common/mac/app_mode_common.h chrome/common/mac/app_mode_common.mm
LGTM
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/316493002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/316493002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/316493002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/316493002/100001
Message was sent while issue was closed.
Change committed as 276368
Message was sent while issue was closed.
Appears to have broken http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%.... Reverting for now. If you feel that was a flake, please re-land.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/329253002/ by rouslan@chromium.org. The reason for reverting is: Appears to have broken AppShimInteractiveTest.Launch as seen in http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%.... Reverting. Please test on Mac with address sanitation enabled locally (or use the mac_asan try-bot) before re-landing. Sorry for the inconvenience. .
tapted, PTAL This test seems to be hitting http://crbug.com/271347 on 10.7. (Or something similar that also crashes on _qtn_proc_apply_to_self). It appears to be something to do with quarantine. The changes in Patch 6 are: - Fix the RemoveQuarantineAttribute() call. - Remove LSFileQuarantineEnabled from the plist. I'm not entirely sure why this fixes the test, but it consistently fails without the above changes, and consistently passes with them. Unfortunately, there's no 10.7 trybot.
slgtm but rsesek should take a look in case of any security implications.. https://codereview.chromium.org/316493002/diff/120001/chrome/app/app_mode-Inf... File chrome/app/app_mode-Info.plist (left): https://codereview.chromium.org/316493002/diff/120001/chrome/app/app_mode-Inf... chrome/app/app_mode-Info.plist:29: <key>LSFileQuarantineEnabled</key> This seems weird - documentation suggests this should only affect files the app_mode_loader creates, and app_mode_loader shouldn't be creating any files. But since the crash seems to be ticking some obscure guts of OSX libraries, it's possible. And, I think it's fine, but there might be security implications for this, so we should get rsesek to have another look. https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... chrome/browser/web_applications/web_app_mac.mm:571: .Append("Contents").Append("MacOS").Append("app_mode_loader")); Cool - I think this makes more sense. (although when I tried this from the command line (with xattr) in http://crbug.com/271347 it didn't seem to help. But part of the diagnosis there was also that OSX was caching "stuff" somewhere, so it's quite possible that ensuring that the attribute is gone the "first" time it's run will avoid tickling this bug in OSX)
rsesek, could you PTAL at changes in Patch 7 for security.
thakis, please review for OWNERS: chrome/app/app_mode-Info.plist
Ping rsesek, thakis BTW, mac_asan_64 failed but not on any shim related tests.
https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... chrome/browser/web_applications/web_app_mac.mm:571: .Append("Contents").Append("MacOS").Append("app_mode_loader")); On 2014/06/12 05:19:22, tapted wrote: > Cool - I think this makes more sense. (although when I tried this from the > command line (with xattr) in http://crbug.com/271347 it didn't seem to help. But > part of the diagnosis there was also that OSX was caching "stuff" somewhere, so > it's quite possible that ensuring that the attribute is gone the "first" time > it's run will avoid tickling this bug in OSX) I thought the quarantine bit was set on the bundle, not the executable itself?
https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... chrome/browser/web_applications/web_app_mac.mm:571: .Append("Contents").Append("MacOS").Append("app_mode_loader")); On 2014/06/13 17:58:41, rsesek wrote: > On 2014/06/12 05:19:22, tapted wrote: > > Cool - I think this makes more sense. (although when I tried this from the > > command line (with xattr) in http://crbug.com/271347 it didn't seem to help. > But > > part of the diagnosis there was also that OSX was caching "stuff" somewhere, > so > > it's quite possible that ensuring that the attribute is gone the "first" time > > it's run will avoid tickling this bug in OSX) > > I thought the quarantine bit was set on the bundle, not the executable itself? If I comment out the RemoveQuarantineAttribute and build a new shim, xattr does not list anything for the shim, but lists com.apple.quarantine for the executable.
On 2014/06/14 01:16:37, jackhou1 wrote: > https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... > File chrome/browser/web_applications/web_app_mac.mm (right): > > https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... > chrome/browser/web_applications/web_app_mac.mm:571: > .Append("Contents").Append("MacOS").Append("app_mode_loader")); > On 2014/06/13 17:58:41, rsesek wrote: > > On 2014/06/12 05:19:22, tapted wrote: > > > Cool - I think this makes more sense. (although when I tried this from the > > > command line (with xattr) in http://crbug.com/271347 it didn't seem to help. > > But > > > part of the diagnosis there was also that OSX was caching "stuff" somewhere, > > so > > > it's quite possible that ensuring that the attribute is gone the "first" > time > > > it's run will avoid tickling this bug in OSX) > > > > I thought the quarantine bit was set on the bundle, not the executable itself? > > If I comment out the RemoveQuarantineAttribute and build a new shim, xattr does > not list anything for the shim, but lists com.apple.quarantine for the > executable. There might be more to this - poking around the Internet suggests that it can appear on directories, and the various guides suggest ones should remove the attribute recursively. The only proper documentation I've been able to find is from the 10.5 release notes https://developer.apple.com/library/mac/releasenotes/Carbon/RN-LaunchServices... It says, "When the Launch Services API is used to open a quarantined file and the file appears to be an application, script, or other executable file type," Here I guess we are both an application, and an executable file type. So, perhaps it should be done on both the .app folder and the directory? Or, we should add a recursive option to base::mac::RemoveQuarantineAttribute. I'm pretty sure this (and the unittests) are the only callsites for RemoveQuarantineAttribute. Maybe "the Launch Services API" encountering inconsistency is what's tickling this obscure bug :/
On 2014/06/16 06:40:07, tapted wrote: > On 2014/06/14 01:16:37, jackhou1 wrote: > > > https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... > > File chrome/browser/web_applications/web_app_mac.mm (right): > > > > > https://codereview.chromium.org/316493002/diff/120001/chrome/browser/web_appl... > > chrome/browser/web_applications/web_app_mac.mm:571: > > .Append("Contents").Append("MacOS").Append("app_mode_loader")); > > On 2014/06/13 17:58:41, rsesek wrote: > > > On 2014/06/12 05:19:22, tapted wrote: > > > > Cool - I think this makes more sense. (although when I tried this from the > > > > command line (with xattr) in http://crbug.com/271347 it didn't seem to > help. > > > But > > > > part of the diagnosis there was also that OSX was caching "stuff" > somewhere, > > > so > > > > it's quite possible that ensuring that the attribute is gone the "first" > > time > > > > it's run will avoid tickling this bug in OSX) > > > > > > I thought the quarantine bit was set on the bundle, not the executable > itself? > > > > If I comment out the RemoveQuarantineAttribute and build a new shim, xattr > does > > not list anything for the shim, but lists com.apple.quarantine for the > > executable. > > There might be more to this - poking around the Internet suggests that it can > appear on directories, and the various guides suggest ones should remove the > attribute recursively. > > The only proper documentation I've been able to find is from the 10.5 release > notes > https://developer.apple.com/library/mac/releasenotes/Carbon/RN-LaunchServices... > > It says, "When the Launch Services API is used to open a quarantined file and > the file appears to be an application, script, or other executable file type," > Here I guess we are both an application, and an executable file type. > > So, perhaps it should be done on both the .app folder and the directory? Or, we > should add a recursive option to base::mac::RemoveQuarantineAttribute. I'm > pretty sure this (and the unittests) are the only callsites for > RemoveQuarantineAttribute. Maybe "the Launch Services API" encountering > inconsistency is what's tickling this obscure bug :/ Yeah, it doesn't hurt to remove it from both. Done.
LGTM
rslgtm
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/316493002/140001
Message was sent while issue was closed.
Change committed as 277743
On 2014/06/17 12:35:11, I haz the power (commit-bot) wrote: > Change committed as 277743 FTR: This was reverted https://codereview.chromium.org/333403007/
Turns out this is was failing in debug builds because the shims don't work with shared libraries (in addition to the quarantine issue). The tests are now disabled in component builds. I'm committing this with NOTRY=true since it consistently passes try bots.
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/316493002/180001
Message was sent while issue was closed.
Change committed as 277955 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
