Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2736863003:
Test that no crash happens with the CWS error page.
|
Created:
3 years, 9 months ago by arthursonzogni Modified:
3 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, Charlie Reis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTest that no crash happens with the CWS error page.
A renderer kill could happens when the Chrome Web Store page is loaded
inside an iframe. This test assert that it doesn't happens.
BUG=622385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed comments #
Total comments: 6
Patch Set 3 : Nit #Patch Set 4 : Rebase. #Patch Set 5 : Prevent crash of the renderer. #
Total comments: 1
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by arthursonzogni@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...
arthursonzogni@chromium.org changed reviewers: + lazyboy@chromium.org, nasko@chromium.org
Hi Nasko, Here is a test that will fails with --browser-side-navigation when you will apply: https://codereview.chromium.org/2736863003 ( Implement error page commit policy in PlzNavigate) N.B. it rely on https://codereview.chromium.org/2727633005 ( PlzNavigate: Enforce frame-src CSP on the browser ) to work. The problem might be avoided if you rewrite the URL of the error pages, or only the one that cannot be committed. I tried something like this before discovering that there was no problem in: https://codereview.chromium.org/2697713005/#ps100001 --- Hi @lazyboy, I would like to commit this CL. Do you think you could review this CL? I am not sure if it is the right place to put this test. Let me know if there is some other place.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/07 15:28:21, arthursonzogni wrote: > Hi Nasko, > > Here is a test that will fails with --browser-side-navigation when you will > apply: > https://codereview.chromium.org/2736863003 ( Implement error page commit policy > in PlzNavigate) > > N.B. it rely on > https://codereview.chromium.org/2727633005 ( PlzNavigate: Enforce frame-src CSP > on the browser ) > to work. > > The problem might be avoided if you rewrite the URL of the error pages, or only > the one that > cannot be committed. I tried something like this before discovering that there > was no problem in: > https://codereview.chromium.org/2697713005/#ps100001 > > --- > > Hi @lazyboy, > > I would like to commit this CL. Do you think you could review this CL? > I am not sure if it is the right place to put this test. Let me know if there is > some other place. The placement of the test seems fine to me.
https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:394: // This problem is similar to crbug.com/622385. It happens when an iframe error nit: https://crbug.com... https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:396: // embedder reside in the same process. In this case, a renderer is killed with nit: s/embedder/parent document/, s/a renderer/the renderer process/ https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:397: // the RFH_CAN_COMMIT_URL_BLOCKED IPC. This test asserts that no crash happens. RFH_CAN_COMMIT_URL_BLOCKED is not an IPC, but an error code. https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:402: ui_test_utils::NavigateToURL(browser(), url); How does this test assert that there is no crash? I don't see this test monitoring explicitly for a renderer process going away. https://codereview.chromium.org/2736863003/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/webstore_blocked_by_frame_src.html (right): https://codereview.chromium.org/2736863003/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/webstore_blocked_by_frame_src.html:7: <iframe src="https://chrome.google.com/webstore"> </iframe> Instead of hardcoding this URL in the HTML file, why not navigate the iframe from the browser process, where we can use the actual string constant that the browser code will be using to check. If left like that, change in the CWS URL will mean that this test will no longer be valid. Also, that means that you can just use chrome/test/data/iframe_blank.html or some other file.
Also, adding creis@ for visibility.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by arthursonzogni@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...
Thanks @lazyboy and @nasko! I applied Nasko's suggestions in the last patch. https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:394: // This problem is similar to crbug.com/622385. It happens when an iframe error On 2017/03/09 05:07:22, nasko (slow) wrote: > nit: https://crbug.com... Done. https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:396: // embedder reside in the same process. In this case, a renderer is killed with On 2017/03/09 05:07:22, nasko (slow) wrote: > nit: s/embedder/parent document/, s/a renderer/the renderer process/ Done. https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:397: // the RFH_CAN_COMMIT_URL_BLOCKED IPC. This test asserts that no crash happens. On 2017/03/09 05:07:22, nasko (slow) wrote: > RFH_CAN_COMMIT_URL_BLOCKED is not an IPC, but an error code. Done. https://codereview.chromium.org/2736863003/diff/1/chrome/browser/extensions/p... chrome/browser/extensions/process_management_browsertest.cc:402: ui_test_utils::NavigateToURL(browser(), url); On 2017/03/09 05:07:22, nasko (slow) wrote: > How does this test assert that there is no crash? I don't see this test > monitoring explicitly for a renderer process going away. Currently, there is no crash but a DCHECK I put a week ago that is triggered in the browser process before the crash: [194887:194887:0309/145431.362148:FATAL:navigation_request.cc(594)] Check failed: render_frame_host->CanCommitURL(common_params_.url). I the next patch, it is checked that the renderer is still alive. FYI, when the test fails, it fails even if I remove the DCHECK, but this time because of the renderer has been killed. https://codereview.chromium.org/2736863003/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/webstore_blocked_by_frame_src.html (right): https://codereview.chromium.org/2736863003/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/webstore_blocked_by_frame_src.html:7: <iframe src="https://chrome.google.com/webstore"> </iframe> On 2017/03/09 05:07:22, nasko (slow) wrote: > Instead of hardcoding this URL in the HTML file, why not navigate the iframe > from the browser process, where we can use the actual string constant that the > browser code will be using to check. If left like that, change in the CWS URL > will mean that this test will no longer be valid. Yes, good idea. I applied your suggestion, but the problem is that if I try content::NavigateIframeToURL, then it is waiting for a navigation to happen, but in the current code, there is still no error page and so content::NavigateIframeToURL is waiting forever. I made something that rely on the 'load' event in the next patch. That works in every case. > Also, that means that you can just use chrome/test/data/iframe_blank.html or > some other file. Yes, modulo the CSP. I still need one new file.
https://codereview.chromium.org/2736863003/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/webstore_blocked_by_frame_src.html (right): https://codereview.chromium.org/2736863003/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/webstore_blocked_by_frame_src.html:7: <iframe src="https://chrome.google.com/webstore"> </iframe> On 2017/03/09 15:40:52, arthursonzogni wrote: > On 2017/03/09 05:07:22, nasko (slow) wrote: > > Instead of hardcoding this URL in the HTML file, why not navigate the iframe > > from the browser process, where we can use the actual string constant that the > > browser code will be using to check. If left like that, change in the CWS URL > > will mean that this test will no longer be valid. > Yes, good idea. > I applied your suggestion, but the problem is that if I try > content::NavigateIframeToURL, then it is waiting for a navigation to happen, but > in the current code, there is still no error page and so > content::NavigateIframeToURL is waiting forever. You could use the TestNavigationManager and WaitForRequestStart(), which could be good enough for you to know the request was made. You don't need to wait for the full commit, though it might be better longer term. > I made something that rely on the 'load' event in the next patch. That works in > every case. > > > Also, that means that you can just use chrome/test/data/iframe_blank.html or > > some other file. > Yes, modulo the CSP. I still need one new file. Oh yeah, good point :). https://codereview.chromium.org/2736863003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2736863003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:399: // asserts that the renderer is still alive after the navigation. nit: "renderer process". One of the main goals I have when reviewing comments is for them to be very explicit and precise. Just using the word "renderer" makes is ambiguous - is it the process or the RenderFrame? Let's try to never use just the word "renderer" by itself, but be more precise. https://codereview.chromium.org/2736863003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:415: "var iframe = document.getElementById('test'); iframe.onload = " nit: I'd put the iframe.onload on the new line and combine it with the "function() {" part. It makes it more clear that we are assigning a function to the onload event handler. https://codereview.chromium.org/2736863003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:419: &message)); Doesn't ExecuteScriptAndExtractString basically return after executing the statement in its argument? How does it wait if the load would take long time?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Nasko. I fixed the nit and answered some comments below: https://codereview.chromium.org/2736863003/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/webstore_blocked_by_frame_src.html (right): https://codereview.chromium.org/2736863003/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/webstore_blocked_by_frame_src.html:7: <iframe src="https://chrome.google.com/webstore"> </iframe> On 2017/03/09 17:18:10, nasko (slow) wrote: > On 2017/03/09 15:40:52, arthursonzogni wrote: > > On 2017/03/09 05:07:22, nasko (slow) wrote: > > > Instead of hardcoding this URL in the HTML file, why not navigate the iframe > > > from the browser process, where we can use the actual string constant that > the > > > browser code will be using to check. If left like that, change in the CWS > URL > > > will mean that this test will no longer be valid. > > Yes, good idea. > > I applied your suggestion, but the problem is that if I try > > content::NavigateIframeToURL, then it is waiting for a navigation to happen, > but > > in the current code, there is still no error page and so > > content::NavigateIframeToURL is waiting forever. > > You could use the TestNavigationManager and WaitForRequestStart(), which could > be good enough for you to know the request was made. You don't need to wait for > the full commit, though it might be better longer term. If a crash happens, it will be in RenderFrameHostImpl::OnDidCommitProvisionnalLoad. So the crash occurs after WaitForRequestStart(). I need to wait for the full commit. https://codereview.chromium.org/2736863003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/process_management_browsertest.cc (right): https://codereview.chromium.org/2736863003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:399: // asserts that the renderer is still alive after the navigation. On 2017/03/09 17:18:11, nasko (slow) wrote: > nit: "renderer process". One of the main goals I have when reviewing comments is > for them to be very explicit and precise. Just using the word "renderer" makes > is ambiguous - is it the process or the RenderFrame? Let's try to never use just > the word "renderer" by itself, but be more precise. Done. https://codereview.chromium.org/2736863003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:415: "var iframe = document.getElementById('test'); iframe.onload = " On 2017/03/09 17:18:11, nasko (slow) wrote: > nit: I'd put the iframe.onload on the new line and combine it with the > "function() {" part. It makes it more clear that we are assigning a function to > the onload event handler. Done. https://codereview.chromium.org/2736863003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/process_management_browsertest.cc:419: &message)); On 2017/03/09 17:18:11, nasko (slow) wrote: > Doesn't ExecuteScriptAndExtractString basically return after executing the > statement in its argument? How does it wait if the load would take long time? It is waiting that the domAutomationController send something. It calls DOMMessageQueue::WaitForMessage(...) So, it will wait as long as it takes.
Description was changed from ========== Test that no crash happens with the CWS error page. A renderer kill could happens when the Chrome Web Store page is loaded inside an iframe. This test assert that it doesn't happens. BUG=622385 ========== to ========== Test that no crash happens with the CWS error page. A renderer kill could happens when the Chrome Web Store page is loaded inside an iframe. This test assert that it doesn't happens. BUG=622385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #5 (id:100001) has been deleted
Hi Nasko, I rebased the code, so * https://codereview.chromium.org/2736863003 => Implement error page commit policy * https://codereview.chromium.org/2727633005 => PlzNavigate: Enforce frame-src CSP are applied. As expected, the test doesn't pass anymore. In the last patch, I applied what I wanted to do in: https://codereview.chromium.org/2697713005/#ps100001 The test passes. Do you think rewriting the url of the error page is the right thing to do(for the moment)?
I think we know the crash will occur and it is there for a good reason - prevent people from iframing the CWS. Fixing error pages is going to be a full project on its own, so I don't think we should be landing this test. https://codereview.chromium.org/2736863003/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2736863003/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:622: common_params_.url = GURL(kUnreachableWebDataURL); This is not something we should do. Charlie, Alex and I discussed what the right thing for error pages to is, but it is not a simple fix, rather a project on its own.
Patchset #6 (id:140001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
