Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1491043002:
Don't swap processes for frames embedded in DevTools pages. (Closed)
|
Created:
5 years ago by alexmos Modified:
5 years ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't swap processes for frames embedded in DevTools pages.
With --site-per-process and --isolate-extensions, cross-process
transfers were broken inside DevTools pages, since DevTools changes
the WebContents being navigated as it executes OpenURLFromTab, and the
RFHM that's eventually navigated can't find its cross-site transfer
request, since it exists on a different RFHM in a different
WebContents. This breaks DevTools extensions. See bug for more
details.
This CL disables process transfers in devtools pages to fix this.
According to dgozman@, this should be safe, since only extensions are
ever embedded in devtools pages.
BUG=564216, 532666
Committed: https://crrev.com/28fae10b3759493f90a201b9841fe35f86b0e0f3
Cr-Commit-Position: refs/heads/master@{#362990}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 2
Patch Set 3 : Address Charlie's comment #Patch Set 4 : Actually put the CHECK in the right place this time. #Patch Set 5 : Fix conflicts in chromium.fyi.json #Patch Set 6 : Another chromium.fyi.json conflict #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Don't swap processes for frames embedded in DevTools pages. BUG=564216 ========== to ========== Don't swap processes for frames embedded in DevTools pages. With --site-per-process and --isolate-extensions, cross-process transfers were broken inside DevTools pages, since DevTools changes the WebContents being navigated as it executes OpenURLFromTab, and the RFHM that's eventually navigated can't find its cross-site transfer request, since it exists on a different RFHM in a different WebContents. This breaks DevTools extensions. See bug for more details. This CL disables process transfers in devtools pages to fix this. According to dgozman@, this should be safe, since only extensions are ever embedded in devtools pages. BUG=564216 ==========
Description was changed from ========== Don't swap processes for frames embedded in DevTools pages. With --site-per-process and --isolate-extensions, cross-process transfers were broken inside DevTools pages, since DevTools changes the WebContents being navigated as it executes OpenURLFromTab, and the RFHM that's eventually navigated can't find its cross-site transfer request, since it exists on a different RFHM in a different WebContents. This breaks DevTools extensions. See bug for more details. This CL disables process transfers in devtools pages to fix this. According to dgozman@, this should be safe, since only extensions are ever embedded in devtools pages. BUG=564216 ========== to ========== Don't swap processes for frames embedded in DevTools pages. With --site-per-process and --isolate-extensions, cross-process transfers were broken inside DevTools pages, since DevTools changes the WebContents being navigated as it executes OpenURLFromTab, and the RFHM that's eventually navigated can't find its cross-site transfer request, since it exists on a different RFHM in a different WebContents. This breaks DevTools extensions. See bug for more details. This CL disables process transfers in devtools pages to fix this. According to dgozman@, this should be safe, since only extensions are ever embedded in devtools pages. BUG=564216, 532666 ==========
alexmos@chromium.org changed reviewers: + creis@chromium.org
Charlie, what do you think? dgozman@ confirmed that only extensions can be embedded in devtools pages, so is it ok to just always skip process transfers from devtools pages? I did also check that the devtools extension in the test won't work from another process without additional work (if we skipped the DCHECK), as it won't have access to DevTools APIs. This CL fixes three tests in --site-per-process and --isolate-extensions: DevToolsExperimentalExtensionTest.TestDevToolsExperimentalExtensionAPI DevToolsExtensionTest.TestDevToolsExtensionAPI DevToolsExtensionTest.TestDevToolsExtensionMessaging
Thanks! LGTM with nit. https://codereview.chromium.org/1491043002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1491043002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1577: if (rfh->GetSiteInstance()->GetSiteURL().SchemeIs(kChromeDevToolsScheme)) Maybe we can add a CHECK that the dest_url's scheme isn't HTTP or HTTPS? Just want to make sure we aren't opening a hole if DevTools later (or accidentally) hosts web content. (For example, if the extension somehow navigated itself to a web page.)
https://codereview.chromium.org/1491043002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1491043002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1577: if (rfh->GetSiteInstance()->GetSiteURL().SchemeIs(kChromeDevToolsScheme)) On 2015/12/02 19:56:07, Charlie Reis wrote: > Maybe we can add a CHECK that the dest_url's scheme isn't HTTP or HTTPS? Just > want to make sure we aren't opening a hole if DevTools later (or accidentally) > hosts web content. (For example, if the extension somehow navigated itself to a > web page.) Done. Good idea!
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1491043002/#ps90001 (title: "Another chromium.fyi.json conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491043002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491043002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by alexmos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491043002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491043002/90001
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491043002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491043002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by alexmos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491043002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491043002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491043002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491043002/90001
Message was sent while issue was closed.
Committed patchset #6 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== Don't swap processes for frames embedded in DevTools pages. With --site-per-process and --isolate-extensions, cross-process transfers were broken inside DevTools pages, since DevTools changes the WebContents being navigated as it executes OpenURLFromTab, and the RFHM that's eventually navigated can't find its cross-site transfer request, since it exists on a different RFHM in a different WebContents. This breaks DevTools extensions. See bug for more details. This CL disables process transfers in devtools pages to fix this. According to dgozman@, this should be safe, since only extensions are ever embedded in devtools pages. BUG=564216, 532666 ========== to ========== Don't swap processes for frames embedded in DevTools pages. With --site-per-process and --isolate-extensions, cross-process transfers were broken inside DevTools pages, since DevTools changes the WebContents being navigated as it executes OpenURLFromTab, and the RFHM that's eventually navigated can't find its cross-site transfer request, since it exists on a different RFHM in a different WebContents. This breaks DevTools extensions. See bug for more details. This CL disables process transfers in devtools pages to fix this. According to dgozman@, this should be safe, since only extensions are ever embedded in devtools pages. BUG=564216, 532666 Committed: https://crrev.com/28fae10b3759493f90a201b9841fe35f86b0e0f3 Cr-Commit-Position: refs/heads/master@{#362990} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/28fae10b3759493f90a201b9841fe35f86b0e0f3 Cr-Commit-Position: refs/heads/master@{#362990} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
