Remove StoragePartition check from URLRequestChromeJob::Start if possible |
|||||
Issue descriptionCheckStoragePartitionMatches was added to URLRequestChromeJob::Start in r252364 to support signin via inline iframes. The inline iframe based signin was removed by rogerta@ in r365712 in favor of a <webview>-based approach, but this check was inadvertently left in place. We think that the StoragePartition check is likely no longer necessary, and it's causing lots of problems: - It was broken soon after landing due to a bug in a cleanup CL (r257556). - It made a refactor tricky to reason about in r394944. - It doesn't work for PlzNavigate, which we're debating how to handle in https://codereview.chromium.org/2002633002. If the <webview>-based code does not require this check, then we should remove CheckStoragePartitionMatches entirely. rogerta@ suggests: "The best way to check if the code is still required: when performing a sign in to chrome: 1/ make sure the renderer process used to show the gaia sign in page is not shared with any other existing renderer process 2/ make sure that the cookie jar of that gaia page is not shared with the profile's main cookie jar 3/ make sure sign in to chrome works"
,
May 25 2016
@creis, yes, that sounds correct.
,
May 25 2016
I'm experimenting with a CL to remove it here: https://codereview.chromium.org/2007223003/ I'm able to sign in via chrome://chrome-signin, which suggests things probably still work for (3). I'm running try jobs now. I can't think of a way that the check would matter today. Our checks in ChildProcessSecurityPolicyImpl::CanCommitURL would prevent WebUI pages from committing in non-WebUI processes, so the original motivation for the check seems stale to me: // The embedder could put some webui pages in separate storage partition. // RenderProcessHostImpl::IsSuitableHost would guard against top level pages // being in the same process. We do an extra check to guard against an // exploited renderer pretending to add them as a subframe. We skip this check // for resources.
,
May 25 2016
Looking promising in https://codereview.chromium.org/2007223003/. If it looks good to signin folks, we can proceed with it.
,
May 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1fa5e60dc33595d1bc56f0452370ee66feb544db commit 1fa5e60dc33595d1bc56f0452370ee66feb544db Author: creis <creis@chromium.org> Date: Thu May 26 16:24:45 2016 Remove obsolete StoragePartition check from old iframe signin path. BUG= 614808 TEST=Signin still works. Review-Url: https://codereview.chromium.org/2007223003 Cr-Commit-Position: refs/heads/master@{#396193} [modify] https://crrev.com/1fa5e60dc33595d1bc56f0452370ee66feb544db/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/1fa5e60dc33595d1bc56f0452370ee66feb544db/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/1fa5e60dc33595d1bc56f0452370ee66feb544db/content/browser/webui/url_data_manager_backend.cc
,
May 26 2016
,
Jun 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b1fa12663f7169c0868dee60c978acfaa6fd1e2 commit 4b1fa12663f7169c0868dee60c978acfaa6fd1e2 Author: caseq <caseq@chromium.org> Date: Wed Jun 01 01:00:00 2016 Revert of Remove obsolete StoragePartition check from old iframe signin path. (patchset #2 id:20001 of https://codereview.chromium.org/2007223003/ ) Reason for revert: This broke DevTools, please see bug for more details. BUG= 616282 Original issue's description: > Remove obsolete StoragePartition check from old iframe signin path. > > BUG= 614808 > TEST=Signin still works. > > Committed: https://crrev.com/1fa5e60dc33595d1bc56f0452370ee66feb544db > Cr-Commit-Position: refs/heads/master@{#396193} TBR=anthonyvd@chromium.org,dbeam@chromium.org,creis@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 614808 Review-Url: https://codereview.chromium.org/2022363002 Cr-Commit-Position: refs/heads/master@{#397006} [modify] https://crrev.com/4b1fa12663f7169c0868dee60c978acfaa6fd1e2/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/4b1fa12663f7169c0868dee60c978acfaa6fd1e2/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/4b1fa12663f7169c0868dee60c978acfaa6fd1e2/content/browser/webui/url_data_manager_backend.cc
,
Jun 2 2016
I'm working to get most of this relanded, without breaking DevTools. The race condition affecting DevTools (found in issue 616282 ) can be fixed separately in issue 616641 .
,
Jun 2 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f5a831f23ed6fdfed5507e5ce9766a7df5d4203c commit f5a831f23ed6fdfed5507e5ce9766a7df5d4203c Author: creis <creis@chromium.org> Date: Thu Jun 02 22:24:19 2016 Remove obsolete StoragePartition check from old iframe signin path. For now, leave the UI thread hop in place for DevTools URLs, due to issue 616641 . BUG= 614808 , 616282 , 616641 TEST=Signin and Mac DevTools still work. Review-Url: https://codereview.chromium.org/2033563002 Cr-Commit-Position: refs/heads/master@{#397530} [modify] https://crrev.com/f5a831f23ed6fdfed5507e5ce9766a7df5d4203c/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/f5a831f23ed6fdfed5507e5ce9766a7df5d4203c/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/f5a831f23ed6fdfed5507e5ce9766a7df5d4203c/content/browser/webui/url_data_manager_backend.cc [modify] https://crrev.com/f5a831f23ed6fdfed5507e5ce9766a7df5d4203c/content/public/browser/content_browser_client.h
,
Jun 3 2016
This seems to have stuck, so I'll mark it fixed. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by creis@chromium.org
, May 25 2016