Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2947413002:
Restrict CM API interface request and message dispatch. (Closed)
DescriptionRestrict CM API interface request and message dispatch.
Add safeguards to ensure that interface requests as well as RPCs are only
dispatched, and thus serviced, when coming from the document corresponding to
the last committed navigation in the currently active render frame.
BUG=736357
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2947413002
Cr-Commit-Position: refs/heads/master@{#484608}
Committed: https://chromium.googlesource.com/chromium/src/+/afef048df9416f31f2d90535c3ca946ac3f43b80
Patch Set 1 : With fix. #
Total comments: 6
Patch Set 2 : Reworked tests, added error handler in renderer. #
Total comments: 36
Patch Set 3 : Addressed comments, and used an associated interface. #
Total comments: 2
Patch Set 4 : Addressed comments from rockot@. #
Total comments: 12
Patch Set 5 : Rebase. #Patch Set 6 : Fix missing include. #Patch Set 7 : Address comments from vasilii@. #Patch Set 8 : Rebase. #Patch Set 9 : Update CredentialManagerClient browsertest to use associated interfaces. #
Total comments: 2
Patch Set 10 : Address nit from clamy@. #Messages
Total messages: 56 (26 generated)
Description was changed from ========== Bug repro. BUG=None ========== to ========== Disconnect existing CM API client, if any, on DidFinishNavigation. BUG=736357 ==========
engedy@chromium.org changed reviewers: + dcheng@chromium.org, mkwst@chromium.org, vasilii@chromium.org
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Daniel, Vasilii, Mike, please take a look. See the bug description for context.
Thanks for fixing this quickly! Assuming that the basic scenarios work, LGTM. 1. Log in with CM API, make few navigations, sign out, verify that autosign-in is suppressed. 2. Open a CM API site, navigate somewhere else, navigate to a CM API site. https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:717: if (web_contents->GetMainFrame() != render_frame_host) I don't mind this check to be on the safe side. Out of curiosity, do you know a scenario which at least in theory would fail here? https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:268: WaitForPasswordStore(); It's excessive. https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:289: EXPECT_EQ(example_url.host(), signin_form.origin.host()); Do I understand right that this condition is flaky without the fix?
I agree that this will work, but it makes me feel like we need a standard way of doing this. Web manifests (https://chromium-review.googlesource.com/c/482739/20/content/browser/manifest...) are another main-frame only IPC that also need to care if the document changes. At the same time, it seems a bit sad to have to keep doing these one-off things... LGTM from an ipc standpoint I guess =/
LGTM. As I mentioned on Friday, it might be worth adding an `origin` field to the `CredentialInfo` in a future patch, and CHECKing in Blink. That would further harden against this specific kind of origin confusion, but also make it less likely that we'd be exploitably confused in the future.
Hey everyone, please take a final look. I reworked tests entirely -- there is no more non-determinism unaccounted for anymore. They now have more the characteristics of white box tests, but otherwise I could not find a good way to wait long enough in the test to ensure that undesired events do *not* happen. Also added some logic to the renderer side so that it recreates the Mojo connection if the existing one breaks. This is now needed so that the second document loaded in the same RF/RFH can use the CM API (given that we now terminate the connection on DidFinishNavigation). Note that this would allow spamming Mojo InterfaceRequests under certain scenarios, but I *think* it should not cause any grave issues. Please let me know if you smell anything fishy around here. https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:717: if (web_contents->GetMainFrame() != render_frame_host) On 2017/06/23 18:58:28, vasilii wrote: > I don't mind this check to be on the safe side. Out of curiosity, do you know a > scenario which at least in theory would fail here? I have added browser tests to exercise this scenario (see tests with preestablish_mojo_pipe == false). https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:268: WaitForPasswordStore(); On 2017/06/23 18:58:28, vasilii wrote: > It's excessive. Reworked tests, N/A anymore. https://codereview.chromium.org/2947413002/diff/40001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:289: EXPECT_EQ(example_url.host(), signin_form.origin.host()); On 2017/06/23 18:58:28, vasilii wrote: > Do I understand right that this condition is flaky without the fix? Reworked tests -- they are not flaky anymore in any scenario (be it pass/failure, with/without the fix).
One more thing: let me know if you think that any aspect of this CL could use unit tests. > 1. Log in with CM API, make few navigations, sign out, verify that autosign-in > is suppressed. Not sure I understand this. How would this behavior be affected by this change? > 2. Open a CM API site, navigate somewhere else, navigate to a CM API site. I have this now as a browser test.
https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:463: if (!navigation_handle->IsSameDocument()) How about combining all the non-same document cleanup together? https://codereview.chromium.org/2947413002/diff/60001/components/password_man... File components/password_manager/content/browser/credential_manager_impl.cc (right): https://codereview.chromium.org/2947413002/diff/60001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:58: binding_.Close(); I'm confused how we could get here--shouldn't the Disconnect() call below have closed it before we get another binding request?
https://codereview.chromium.org/2947413002/diff/60001/components/password_man... File components/password_manager/content/browser/credential_manager_impl.cc (right): https://codereview.chromium.org/2947413002/diff/60001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:58: binding_.Close(); On 2017/06/28 19:04:49, dcheng (OOO Jun 25 - Jul 1) wrote: > I'm confused how we could get here--shouldn't the Disconnect() call below have > closed it before we get another binding request? I was trying to be on the safe side here -- my thinking was that we could get there if the Mojo pipe is broken for reasons other than either end closing it purposely (some IPC error?). Essentially, my thinking was that we could DCHECK(binding_.encountered_error()) here, but that flag is not exposed. BindingStateBase::BindInternal DCHECK's that it's not bound, hence the if(), although I'm not sure if anything exploded in production if we just called Bind on a bound Binding anyway. WDYT?
https://codereview.chromium.org/2947413002/diff/60001/components/password_man... File components/password_manager/content/browser/credential_manager_impl.cc (right): https://codereview.chromium.org/2947413002/diff/60001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:58: binding_.Close(); On 2017/06/28 19:26:34, engedy wrote: > On 2017/06/28 19:04:49, dcheng (OOO Jun 25 - Jul 1) wrote: > > I'm confused how we could get here--shouldn't the Disconnect() call below have > > closed it before we get another binding request? > > I was trying to be on the safe side here -- my thinking was that we could get > there if the Mojo pipe is broken for reasons other than either end closing it > purposely (some IPC error?). > Essentially, my thinking was that we could DCHECK(binding_.encountered_error()) > here, but that flag is not exposed. > > BindingStateBase::BindInternal DCHECK's that it's not bound, hence the if(), > although I'm not sure if anything exploded in production if we just called Bind > on a bound Binding anyway. WDYT? One common alternative is to add an explicit connection error handler that calls Close() on the binding to reset the internal state on a connection error (if either end of the pipe is closed for whatever reason). This is typically done if there's other interesting work that should be reset when the message pipe is closed (other than just closing the binding itself). I don't have a strong preference about whether we use the connection error handler or use the CL as-is, but long-term, this definitely seems like something that makes more sense to be frame-scoped (once frame is 1:1 with document): does this seem reasonable?
The solution looks good. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:464: ukm_source_id_.reset(); As a side effect that line now runs on Android. I presume it's correct but worth a check. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.h:138: // Exposed for testing. Missing #if defined(UNIT_TEST) https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:31: // destroyed. Is the second sentence trying to say more than "RenderFrameDeleted means that RenderFrameHost is being deleted". If not then I'd just drop it as confusing. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:41: return; Note that if destroyed_ is true then run_loop_.Run() exits immediately. You still can have this condition, of course. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:333: for (const auto preestablish_mojo_pipe : {false, true}) { const bool https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:333: for (const auto preestablish_mojo_pipe : {false, true}) { I'd prefer to split this test into two. The body can go to a method taking preestablish_mojo_pipe as a parameter. My concern is that the loop iteration may produce unwanted side effects. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:364: base::RunLoop().RunUntilIdle(); I'm not a big fan of RunUntilIdle() in the browser tests. The test may time out. I think NavigateToURL already waits for everything you need. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:371: // CredentialManagerImpl is retrieved is synchronized to FrameHost Is this request itself coming from IPC? https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:379: EXPECT_FALSE(client->has_binding_for_credential_manager()); Is it the check that would fail before the CL? https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:391: prompt_observer.WaitForSavePrompt(); You may really crash here. WaitForSavePrompt() can be used only on WebContents(). That is, a specially created tab inside PasswordManagerBrowserTestBase with a fake UI controller. Instead you can wait for password store and check prompt_observer.IsShowingSavePrompt() https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:454: // Ensure that the navigator.credentials.store() call is never serviced. Why can't the same scenario as in the previous test happen? (RenderFrameHost gets a store() message right before deletion) https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:462: base::RunLoop().RunUntilIdle(); Unnecessary I think. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:495: BubbleObserver prompt_observer(active_web_contents()); Please use WebContents() for the reasons stated above. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:507: base::RunLoop().RunUntilIdle(); ditto https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:522: ASSERT_EQ(old_rfh, active_web_contents()->GetMainFrame()); Do you want to check the connection?
@Vasilii, @Daniel: addressed all comments, please take another look. @Ken, please a close look at the associated interface parts. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:463: if (!navigation_handle->IsSameDocument()) On 2017/06/28 19:04:49, dcheng (OOO Jun 25 - Jul 1) wrote: > How about combining all the non-same document cleanup together? Hmm, that line wasn't there before the last rebase. :-) I'd keep this separate for now to facilitate merging the CL back to branches. I can do the clean-up in a follow-up CL. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:464: ukm_source_id_.reset(); On 2017/06/29 11:59:58, vasilii wrote: > As a side effect that line now runs on Android. I presume it's correct but worth > a check. Rebase artifact. After discussing with Dominic, we decided it belongs to the outside of the #if-!def. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.h:138: // Exposed for testing. On 2017/06/29 11:59:59, vasilii wrote: > Missing #if defined(UNIT_TEST) Done. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:31: // destroyed. On 2017/06/29 11:59:59, vasilii wrote: > Is the second sentence trying to say more than "RenderFrameDeleted means that > RenderFrameHost is being deleted". If not then I'd just drop it as confusing. Yes, there is a subtle difference between the `Deleted` event and the RFH::~dtor being called. Luckily, the two coincides. But no longer applicable, as I found this exact class among test utils. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:41: return; On 2017/06/29 11:59:59, vasilii wrote: > Note that if destroyed_ is true then run_loop_.Run() exits immediately. > You still can have this condition, of course. N/A, removed this class. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:333: for (const auto preestablish_mojo_pipe : {false, true}) { On 2017/06/29 12:00:00, vasilii wrote: > const bool Done. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:333: for (const auto preestablish_mojo_pipe : {false, true}) { On 2017/06/29 11:59:59, vasilii wrote: > I'd prefer to split this test into two. The body can go to a method taking > preestablish_mojo_pipe as a parameter. > My concern is that the loop iteration may produce unwanted side effects. Ack. Done this one, and also the one below. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:364: base::RunLoop().RunUntilIdle(); On 2017/06/29 11:59:59, vasilii wrote: > I'm not a big fan of RunUntilIdle() in the browser tests. The test may time out. > I think NavigateToURL already waits for everything you need. Done. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:371: // CredentialManagerImpl is retrieved is synchronized to FrameHost On 2017/06/29 11:59:59, vasilii wrote: > Is this request itself coming from IPC? As discussed offline, made this an associated interface and updated comment. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:379: EXPECT_FALSE(client->has_binding_for_credential_manager()); On 2017/06/29 11:59:59, vasilii wrote: > Is it the check that would fail before the CL? Among others, yes. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:391: prompt_observer.WaitForSavePrompt(); On 2017/06/29 11:59:59, vasilii wrote: > You may really crash here. > WaitForSavePrompt() can be used only on WebContents(). That is, a specially > created tab inside PasswordManagerBrowserTestBase with a fake UI controller. > > Instead you can wait for password store and check > prompt_observer.IsShowingSavePrompt() Done. (Using WebContents() now.) https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:454: // Ensure that the navigator.credentials.store() call is never serviced. On 2017/06/29 11:59:59, vasilii wrote: > Why can't the same scenario as in the previous test happen? (RenderFrameHost > gets a store() message right before deletion) It's channel is disconnected in DidFinishNavigation, and it cannot establish a new one due to the other check in ChromePasswordManagerClient::BindCredentialManager. Does this make sense? https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:462: base::RunLoop().RunUntilIdle(); On 2017/06/29 11:59:59, vasilii wrote: > Unnecessary I think. Done. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:495: BubbleObserver prompt_observer(active_web_contents()); On 2017/06/29 11:59:59, vasilii wrote: > Please use WebContents() for the reasons stated above. Done. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:507: base::RunLoop().RunUntilIdle(); On 2017/06/29 12:00:00, vasilii wrote: > ditto Done. https://codereview.chromium.org/2947413002/diff/60001/chrome/browser/password... chrome/browser/password_manager/credential_manager_browsertest.cc:522: ASSERT_EQ(old_rfh, active_web_contents()->GetMainFrame()); On 2017/06/29 12:00:00, vasilii wrote: > Do you want to check the connection? Done. https://codereview.chromium.org/2947413002/diff/60001/components/password_man... File components/password_manager/content/browser/credential_manager_impl.cc (right): https://codereview.chromium.org/2947413002/diff/60001/components/password_man... components/password_manager/content/browser/credential_manager_impl.cc:58: binding_.Close(); On 2017/06/29 06:43:49, dcheng (OOO Jun 25 - Jul 1) wrote: > On 2017/06/28 19:26:34, engedy wrote: > > On 2017/06/28 19:04:49, dcheng (OOO Jun 25 - Jul 1) wrote: > > > I'm confused how we could get here--shouldn't the Disconnect() call below > have > > > closed it before we get another binding request? > > > > I was trying to be on the safe side here -- my thinking was that we could get > > there if the Mojo pipe is broken for reasons other than either end closing it > > purposely (some IPC error?). > > Essentially, my thinking was that we could > DCHECK(binding_.encountered_error()) > > here, but that flag is not exposed. > > > > BindingStateBase::BindInternal DCHECK's that it's not bound, hence the if(), > > although I'm not sure if anything exploded in production if we just called > Bind > > on a bound Binding anyway. WDYT? > > One common alternative is to add an explicit connection error handler that calls > Close() on the binding to reset the internal state on a connection error (if > either end of the pipe is closed for whatever reason). This is typically done if > there's other interesting work that should be reset when the message pipe is > closed (other than just closing the binding itself). > > I don't have a strong preference about whether we use the connection error > handler or use the CL as-is, but long-term, this definitely seems like something > that makes more sense to be frame-scoped (once frame is 1:1 with document): does > this seem reasonable? Yep, added error handler.
engedy@chromium.org changed reviewers: + rockot@chromium.org
+Ken actually. Sorry about Rietveld.
LGTM with optional suggestions https://codereview.chromium.org/2947413002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2947413002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:665: virtual void BindAssociatedInterfaceRequestFromFrame( Please consider making this return a bool and take a ScopedInterfaceEndpointHandle* instead (returns true if it actually takes *handle and binds it, false otherwise). And then call it in RFHI::OnAssociatedInterfaceRequest instead of WebContentsImpl. Also please consider removing the BindSourceInfo argument since it's completely redundant and I doubt you need it in your binder.
Description was changed from ========== Disconnect existing CM API client, if any, on DidFinishNavigation. BUG=736357 ========== to ========== Disconnect existing CM API client, if any, on DidFinishNavigation. BUG=736357 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/2947413002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2947413002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:665: virtual void BindAssociatedInterfaceRequestFromFrame( On 2017/06/30 21:11:24, Ken Rockot(use gerrit already) wrote: > Please consider making this return a bool and take a > ScopedInterfaceEndpointHandle* instead (returns true if it actually takes > *handle and binds it, false otherwise). And then call it in > RFHI::OnAssociatedInterfaceRequest instead of WebContentsImpl. > > Also please consider removing the BindSourceInfo argument since it's completely > redundant and I doubt you need it in your binder. All done, please take a final look.
lgtm
https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:737: // zombie RFH's being swapped out following cross-origin navigations. Hmm... I feel like something at a higher-level is broken if we violate this condition... I don't think we should be forwarding bind requests for swapped out frames at all.
https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/chrome_password_manager_client.cc:737: // zombie RFH's being swapped out following cross-origin navigations. On 2017/07/03 09:23:33, dcheng (OOO Jun 25 - Jul 1) wrote: > Hmm... I feel like something at a higher-level is broken if we violate this > condition... I don't think we should be forwarding bind requests for swapped out > frames at all. Yes, I agree that this is a bit weird, but note the subtle distinction between `swapped out` and `being swapped out`. Currently the ordering of events is as follows: 1) FrameHostMsg_DidCommitProvisionalLoad for provisional RFH 2) RenderFrameHostManager::CommitPending() 3) FrameMsg_SwapOut sent, RFH put on |pending_delete_hosts_| 4) The `unload` handler invoked on old Document 4.1) InterfaceRequest sent, potentially followed by method call 5) FrameHostMsg_SwapOut_ACK sent 6) RFH deleted Obviously this is also orthogonal to using associated interfaces or not. The browser tests with |preestablish_mojo_pipe == false| exercise this scenario, and would fail without this check.
Looks good. Just couple of questions. https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:6: #include "base/run_loop.h" unused https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:88: // Tests the when navigator.credentials.store() is called in an `unload` Tests the when? https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:132: // InterfaceRequests cannot be issued anymore. As I understand, it's all due to the same origin navigation. Can you describe a row of events happening? I think you want to say that FrameHostMsg_DidCommitProvisionalLoad is sent after 'unload' stuff is executed. https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:217: rfh_destruction_observer.WaitUntilDeleted(); I wonder if it's possible that NavigateToURL doesn't wait for RFH being destroyed? Regardless of the answer it's good to have this call to be safe. https://codereview.chromium.org/2947413002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2947413002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:913: auto* browser_client = GetContentClient()->browser(); I'd prefer ContentBrowserClient*
https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:6: #include "base/run_loop.h" On 2017/07/03 13:59:47, vasilii wrote: > unused Done. https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:88: // Tests the when navigator.credentials.store() is called in an `unload` On 2017/07/03 13:59:47, vasilii wrote: > Tests the when? Sorry, typo. Fixed. https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:132: // InterfaceRequests cannot be issued anymore. On 2017/07/03 13:59:47, vasilii wrote: > As I understand, it's all due to the same origin navigation. Can you describe a > row of events happening? > I think you want to say that FrameHostMsg_DidCommitProvisionalLoad is sent after > 'unload' stuff is executed. I have clarified. https://codereview.chromium.org/2947413002/diff/100001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:217: rfh_destruction_observer.WaitUntilDeleted(); On 2017/07/03 13:59:47, vasilii wrote: > I wonder if it's possible that NavigateToURL doesn't wait for RFH being > destroyed? > > Regardless of the answer it's good to have this call to be safe. Yes, it can happen, as the swapped out RFH is put on a |pending_delete_hosts_| list and only destroyed when the SwapOut_ACK message arrives to the browser -- this no longer has to do with the load completing in the new RFH. https://codereview.chromium.org/2947413002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2947413002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:913: auto* browser_client = GetContentClient()->browser(); On 2017/07/03 13:59:47, vasilii wrote: > I'd prefer ContentBrowserClient* Done.
lgtm
@Daniel, do you want to take a final look?
On 2017/07/03 15:45:48, engedy wrote: > @Daniel, do you want to take a final look? LGTM. Thanks for the explanation. However, this really highlights the problem of the credential manager being a web contents observer rather than being tied to a RenderFrameHost. 1) How hard is it to not make it a WebContentsObserver? 2) Can we experiment with not even forwarding interface requests if the sending RenderFrameHost isn't active anymore (i.e. it's being swapped out?) While there might be a few things where we want that to work, having it not be the default seems safer and the right thing to do.
On 2017/07/06 01:23:16, dcheng wrote: > On 2017/07/03 15:45:48, engedy wrote: > > @Daniel, do you want to take a final look? > > LGTM. Thanks for the explanation. > > However, this really highlights the problem of the credential manager being a > web contents observer rather than being tied to a RenderFrameHost. > > 1) How hard is it to not make it a WebContentsObserver? > 2) Can we experiment with not even forwarding interface requests if the sending > RenderFrameHost isn't active anymore (i.e. it's being swapped out?) While there > might be a few things where we want that to work, having it not be the default > seems safer and the right thing to do. Agreed on all points. Specifically for the CM API, I am planning to do something like (1) in a follow-up CL, and (2) is already addressed by this CL in chrome_password_manager_client.cc:752. I have filed crbug.com/739645 to keep track of this issue in general.
Description was changed from ========== Disconnect existing CM API client, if any, on DidFinishNavigation. BUG=736357 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Restrict CM API interface request and message dispatch. Add safeguards to ensure that interface requests as well as RPCs are only dispatched, and thus serviced, when coming from the document corresponding to the last committed navigation in the currently active render frame. BUG=736357 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by engedy@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_...)
The CQ bit was checked by engedy@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 engedy@chromium.org to run a CQ dry run
Patchset #9 (id:200001) has been deleted
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 engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, rockot@chromium.org, dcheng@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2947413002/#ps220001 (title: "Update CredentialManagerClient browsertest to use associated interfaces.")
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...)
engedy@chromium.org changed reviewers: + clamy@chromium.org
@Camille, could you please review the following for OWNER's approval: content/browser/frame_host/render_frame_host_impl.cc content/public/browser/content_browser_client.cc content/public/browser/content_browser_client.h
Thanks! Lgtm with one nit. https://codereview.chromium.org/2947413002/diff/220001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2947413002/diff/220001/content/public/browser... content/public/browser/content_browser_client.h:664: // embedder should try. Should return true if the |handle| was actually taken nit: s/Should return/Returns
Thanks for the quick review! https://codereview.chromium.org/2947413002/diff/220001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2947413002/diff/220001/content/public/browser... content/public/browser/content_browser_client.h:664: // embedder should try. Should return true if the |handle| was actually taken On 2017/07/06 14:19:43, clamy wrote: > nit: s/Should return/Returns Done.
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, clamy@chromium.org, mkwst@chromium.org, dcheng@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/2947413002/#ps240001 (title: "Address nit from clamy@.")
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": 240001, "attempt_start_ts": 1499350964886070,
"parent_rev": "378a9a29ffab72232c5e1d06c8bb70919ba6c35f", "commit_rev":
"afef048df9416f31f2d90535c3ca946ac3f43b80"}
Message was sent while issue was closed.
Description was changed from ========== Restrict CM API interface request and message dispatch. Add safeguards to ensure that interface requests as well as RPCs are only dispatched, and thus serviced, when coming from the document corresponding to the last committed navigation in the currently active render frame. BUG=736357 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Restrict CM API interface request and message dispatch. Add safeguards to ensure that interface requests as well as RPCs are only dispatched, and thus serviced, when coming from the document corresponding to the last committed navigation in the currently active render frame. BUG=736357 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2947413002 Cr-Commit-Position: refs/heads/master@{#484608} Committed: https://chromium.googlesource.com/chromium/src/+/afef048df9416f31f2d90535c3ca... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001) as https://chromium.googlesource.com/chromium/src/+/afef048df9416f31f2d90535c3ca... |
