Improve ShowWebModalDialogViews() API to prevent crashes |
||
Issue descriptionThis comes out from a CL review discussion at https://chromium-review.googlesource.com/c/528015 Today constrained_window::ShowWebModalDialogViews(..., content::WebContents*) impl has some code like this: web_modal::WebContentsModalDialogManager::FromWebContents(web_contents) ->delegate() ->GetWebContentsModalDialogHost() ->GetHostView()); The problem is that ShowWebModalDialogViews() is a public API and can take in any valid WebContents, which doesn't necessarily have a WebContentsModalDialogManager attached to it. Note that if not done properly, a WebContentsModalDialogManager may not have a delegate either. This leave rooms for crashes in some undertested scenario, e.g. the WebContents is created for an extension etc. Here'are wittman@'s comments about this issue: We could do null checks in the code snippets above, but that would have the effect of implicitly making decisions with security and privacy implications on the user's behalf, decisions that we've already recognized that we don't have the information to answer ourselves. In analogous fashion to the guidance to use CHECK() if the consequence of a failed assertion would be a security vulnerability, it's better to crash than to make a wrong decision with security implications. Crashing also provides visibility into the fact that there is a problem -- using a null check would just silently hide it. IMO a better approach to the general issue would be to either provide a more structured mechanism to create WebContents within Chrome that attaches a WebContentsModalDialogManager, or update the web_modal functionality to supply some WebContentsModalDialogManager-of-last-resort to handle cases where no manager was registered.
,
Sep 13
Archiving old bugs that haven't been actively assigned in over 180 days. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!
,
Sep 13
Archiving old bugs that haven't been actively assigned in over 180 days. If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks! |
||
►
Sign in to add a comment |
||
Comment 1 by dtapu...@chromium.org
, Dec 7 2017