Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1225693009:
Show extension uninstall dialog in browser window. (Closed)
DescriptionShow extension uninstall dialog in browser window.
This fixes crash when chrome.management.uninstall() is called from background
page. The logic is copied from CreateAppShortcutFunctionDelegate(). The dialog
will show in the browser instead of in a new window.
BUG=505236
R=kalman,mek
Committed: https://crrev.com/5fd713ee7244a4949e120c9ababdb71fda3109f5
Cr-Commit-Position: refs/heads/master@{#338410}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 13
Patch Set 5 : #
Total comments: 1
Messages
Total messages: 19 (3 generated)
wjywbs@gmail.com changed reviewers: + mek@chromium.org
PTAL, thanks. (Should I file a security bug next time to get it fixed earlier? Does crash in extensions code count as a security bug?)
I don't know what you mean by "fixing it earlier", but it's ideal to file with the crash report template and track down a crash ID if you have one. https://codereview.chromium.org/1225693009/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:82: profile, chrome::HOST_DESKTOP_TYPE_NATIVE); Looks like you're not supposed to hard-code this enum, see: https://sites.google.com/a/chromium.org/dev/developers/design-documents/aura/... https://codereview.chromium.org/1225693009/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:85: profile, browser->window()->GetNativeWindow(), this)); I think it's better to at least try to show it in the WebContent's native window, even though it's probably equivalent to the active window given it requires a user action. Then fall back to the active window if necessary.
https://codereview.chromium.org/1225693009/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:82: profile, chrome::HOST_DESKTOP_TYPE_NATIVE); On 2015/07/08 20:40:40, kalman wrote: > Looks like you're not supposed to hard-code this enum, see: > https://sites.google.com/a/chromium.org/dev/developers/design-documents/aura/... Done. https://codereview.chromium.org/1225693009/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:85: profile, browser->window()->GetNativeWindow(), this)); On 2015/07/08 20:40:41, kalman wrote: > I think it's better to at least try to show it in the WebContent's native > window, even though it's probably equivalent to the active window given it > requires a user action. Then fall back to the active window if necessary. Done.
https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:95: } Is this entire block of code necessary? I would have thought that a background contents would return null for GetTopLevelNativeWindow(), since it doesn't have one.
https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:95: } On 2015/07/08 23:10:04, kalman wrote: > Is this entire block of code necessary? I would have thought that a background > contents would return null for GetTopLevelNativeWindow(), since it doesn't have > one. No, GetTopLevelNativeWindow() does not return null for background contents, and this is why the crash happened. It returns a valid pointer, but calling window->GetRootWindow() on this pointer will return null. https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/na...
https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:95: } On 2015/07/09 00:27:12, wjywbs wrote: > On 2015/07/08 23:10:04, kalman wrote: > > Is this entire block of code necessary? I would have thought that a background > > contents would return null for GetTopLevelNativeWindow(), since it doesn't > have > > one. > > No, GetTopLevelNativeWindow() does not return null for background contents, and > this is why the crash happened. It returns a valid pointer, but calling > window->GetRootWindow() on this pointer will return null. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/na... ok, could you check for a null root window then?
https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:95: } On 2015/07/09 16:30:42, kalman wrote: > > ok, could you check for a null root window then? The window->GetRootWindow() function is only defined in aura, but not on Mac. Should I add a "#if defined(USE_AURA)" instead?
https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:95: } On 2015/07/09 16:54:25, wjywbs wrote: > On 2015/07/09 16:30:42, kalman wrote: > > > > ok, could you check for a null root window then? > > The window->GetRootWindow() function is only defined in aura, but not on Mac. > Should I add a "#if defined(USE_AURA)" instead? ahhh damn it. Alright! This all finally reminded me of a very similar situation we had with the media galleries API. It did two things: 1. The lack of a place to display the dialogue is detected by the lack of a "dialogue manager". A bit of a hack but apparently it works: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... 2. In the case where there *isn't* a place to show it (including from a background page), trace the caller back to the tab that initiated the call to uninstall() in the first place: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... i.e. you can access source_tab_id(). Could you try something like that? I would pull that function I mentioned in (1) into a function on at least ChromeUIThreadExtensionFunction, "GetVisibleWebContents()". It would be nice to pull this into UIThreadExtensionFunction but at this point I don't want to add a dependency from the extensions module into the web_modal component.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:95: } On 2015/07/09 17:13:39, kalman wrote: > ahhh damn it. Alright! This all finally reminded me of a very similar situation > we had with the media galleries API. It did two things: > > 1. The lack of a place to display the dialogue is detected by the lack of a > "dialogue manager". A bit of a hack but apparently it works: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > 2. In the case where there *isn't* a place to show it (including from a > background page), trace the caller back to the tab that initiated the call to > uninstall() in the first place: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > i.e. you can access source_tab_id(). Could you try something like that? > > I would pull that function I mentioned in (1) into a function on at least > ChromeUIThreadExtensionFunction, "GetVisibleWebContents()". It would be nice to > pull this into UIThreadExtensionFunction but at this point I don't want to add a > dependency from the extensions module into the web_modal component. Done.
Sorry about the huge number of comments here, but you're doing a great cleanup, and sometime later, we should be able to move over a bunch of other code to use your new function. The comments should be pretty straightforward to do this really. https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc (right): https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:83: ChromeExtensionFunctionDetails(function).GetVisibleWebContents(); I'd slightly restructure this, to avoid the cast from BrowserContext->Profile which already happens: ChromeExtensionFunctionDetails details(function); content::WebContents* web_contents = details.GetVisibleWebContents(); ... ... chrome::FindBrowserWithProfile(details.GetProfile(), ...); ... https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:90: chrome::FindBrowserWithProfile(profile, chrome::GetActiveDesktop()); You could move this into GetNativeWindowForUI? https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_function_details.cc (right): https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:123: WebContents::FromRenderFrameHost(function_->render_frame_host()); You should be able to use GetSenderWebContents() here. It knows how to pull out the WebContents from render_frame_host. You should also null-check it and return nullptr if there is no sender. https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:124: web_modal::WebContentsModalDialogManager* web_contents_modal_dialog_manager = Add comment: // Hack: use the existence of a WebContentsModalDialogManager to decide whether // the sender web contents is visible. I'd also prefer to early-return rather than putting the rest of the function body behind this check. https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:139: contents = window->web_contents(); Likewise, early-return (even once moved to the end): if (window) return window->web_contents(); https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:142: int source_tab_id = function_->source_tab_id(); Tiny request: put the checks for the tab above the check for the app window. It's more likely to pass, so it saves looking up an app window probably unnecessarily. https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:148: profile->IsOffTheRecord(), NULL, just use "true" here, not "profile->IsOffTheRecord()". But do comment it: ... true, // include_incognito ... https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:149: NULL, &contents, NULL); always use nullptr not NULL now. https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:150: } and early-return. https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_function_details.h (right): https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.h:65: // Gets the web contents if it's not from a background page. Otherwise this Actually can we call this method something with a weaker guarantee than "GetVisibleWebContents"? It's not really clear what a "visible web contents" means to this function in particular. "Sender" is obvious. "Associated" is a really bad name which is why we want to get rid of that function. In fact, I'd even go so far as to make this return a NativeWindow, like "GetNativeWindowForUI", and document it as finding a UI surface to display any UI (like a permission prompt) for the extension calling this function. You could also move the active browser check into here.
https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_function_details.cc (right): https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:135: extensions::AppWindow* window = Actually, perhaps you should be using WindowControllerList here? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... like: // Get the WindowControllerList from somewhere. Where? WindowControllerList::CurrentWindowForFunction(function_)->window()->GetNativeWindow(); but I'd (or you'd) need to double check that CurrentWindowForFunction does what you'd expect. I *think* it does. This would let you pick up devtools windows as well, I believe.
All comments are addressed. Code is rebased, but I'm still compiling. https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_function_details.cc (right): https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.cc:135: extensions::AppWindow* window = On 2015/07/10 17:57:03, kalman wrote: > Actually, perhaps you should be using WindowControllerList here? > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > like: > > // Get the WindowControllerList from somewhere. Where? > WindowControllerList::CurrentWindowForFunction(function_)->window()->GetNativeWindow(); > > but I'd (or you'd) need to double check that CurrentWindowForFunction does what > you'd expect. I *think* it does. This would let you pick up devtools windows as > well, I believe. Turns out that CurrentWindowForFunction() can properly find extension's window but not app's window, but I used CurrentWindowForFunction() in GetNativeWindowForUI(). https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_function_details.h (right): https://codereview.chromium.org/1225693009/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_function_details.h:65: // Gets the web contents if it's not from a background page. Otherwise this On 2015/07/10 17:02:25, kalman wrote: > Actually can we call this method something with a weaker guarantee than > "GetVisibleWebContents"? It's not really clear what a "visible web contents" > means to this function in particular. "Sender" is obvious. "Associated" is a > really bad name which is why we want to get rid of that function. > > In fact, I'd even go so far as to make this return a NativeWindow, like > "GetNativeWindowForUI", and document it as finding a UI surface to display any > UI (like a permission prompt) for the extension calling this function. You could > also move the active browser check into here. I renamed this to be "GetOriginWebContents" because media gallery only needs WebContents but not NativeWindow. I added the "GetNativeWindowForUI" function.
lgtm https://codereview.chromium.org/1225693009/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function_details.h (right): https://codereview.chromium.org/1225693009/diff/100001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function_details.h:66: content::WebContents* GetOriginWebContents(); I like it.
The CQ bit was checked by wjywbs@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225693009/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5fd713ee7244a4949e120c9ababdb71fda3109f5 Cr-Commit-Position: refs/heads/master@{#338410} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
