Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2697713005: DCHECK: Browser asking renderer to commit URLs it is not allowed to. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 1 week ago by arthursonzogni
Modified:
7 months, 4 weeks ago
Reviewers:
clamy
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, mlamouri+watch-content_chromium.org, alexmos, nasko
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

DCHECK: Browser asking renderer to commit URLs it is not allowed to. This CL adds a DCHECK when the browser ask the renderer to commit an URL and then kill the renderer when it does. This CL was previously related with a problem when the renderer tries to commit an error page to the chrome webstore inside an iframe. It turns out that there was no more problems with PlzNavigate since the ChromeWebStore error page uses a different process. BUG=588314, 622385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2697713005 Cr-Commit-Position: refs/heads/master@{#452808} Committed: https://chromium.googlesource.com/chromium/src/+/4ba80de54a09324ae69bde773f194df0eec6e8b3

Patch Set 1 #

Patch Set 2 : I forgot to remove the change in render_frame_impl #

Total comments: 4

Patch Set 3 : Addressed comments clamy@ #

Patch Set 4 : Move things into the NavigationRequest. #

Total comments: 2

Patch Set 5 : Move implementation of CanCommitURL. #

Patch Set 6 : Turns the condition into a DCHECK. #

Patch Set 7 : git cl try #

Messages

Total messages: 63 (50 generated)
arthursonzogni
Hi Camille, Please, could you take a look?
8 months ago (2017-02-16 08:55:38 UTC) #13
clamy
Thanks! A few comments below. Also, it seems quite a few tests are failing on ...
8 months ago (2017-02-16 13:45:59 UTC) #14
arthursonzogni
Thanks Camille. I have the same failure with and without this patch, but I only ...
8 months ago (2017-02-17 09:33:20 UTC) #17
clamy
https://codereview.chromium.org/2697713005/diff/20001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2697713005/diff/20001/content/browser/frame_host/render_frame_host_impl.cc#newcode2774 content/browser/frame_host/render_frame_host_impl.cc:2774: CommonNavigationParams new_common_params; On 2017/02/17 09:33:19, arthursonzogni wrote: > On ...
8 months ago (2017-02-21 15:32:25 UTC) #32
arthursonzogni
Thanks Camille. It works % these two things that might be unrelated. * I found ...
8 months ago (2017-02-22 16:25:03 UTC) #33
clamy
Thanks! Current version lgtm with one comment. The bug looks weird, might not be PlzNavigate ...
8 months ago (2017-02-22 16:40:16 UTC) #34
arthursonzogni
> The bug looks weird, might not be PlzNavigate related? I don't know, it is ...
8 months ago (2017-02-22 17:31:13 UTC) #35
clamy
On 2017/02/22 17:31:13, arthursonzogni wrote: > > The bug looks weird, might not be PlzNavigate ...
7 months, 4 weeks ago (2017-02-23 12:09:52 UTC) #36
arthursonzogni
On 2017/02/23 12:09:52, clamy wrote: > On 2017/02/22 17:31:13, arthursonzogni wrote: > > > The ...
7 months, 4 weeks ago (2017-02-23 14:23:43 UTC) #37
arthursonzogni
On 2017/02/23 14:23:43, arthursonzogni wrote: > On 2017/02/23 12:09:52, clamy wrote: > > On 2017/02/22 ...
7 months, 4 weeks ago (2017-02-23 14:51:08 UTC) #38
arthursonzogni
So, there is no more problems with PlzNavigate when an error page to the ChromeWebStore ...
7 months, 4 weeks ago (2017-02-23 17:05:44 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697713005/180001
7 months, 4 weeks ago (2017-02-24 13:36:09 UTC) #60
commit-bot: I haz the power
7 months, 4 weeks ago (2017-02-24 13:40:16 UTC) #63
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/4ba80de54a09324ae69bde773f19...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa