Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862093002:
Remove DCHECK that crashes.
|
Created:
3 years, 7 months ago by Avi (use Gerrit) Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, mlamouri+watch-content_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove DCHECK that crashes.
BUG=718570
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #
Dependent Patchsets: Messages
Total messages: 18 (7 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Description was changed from ========== Remove DCHECK that crashes. BUG=718570 ========== to ========== Remove DCHECK that crashes. BUG=718570 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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.
avi@chromium.org changed reviewers: + nasko@chromium.org
creis@chromium.org changed reviewers: + creis@chromium.org
Ah, great, you have repro steps on the bug! Please add a test for that case, since we clearly don't have test coverage for it today. And while I'm all for getting rid of the otherwise dead parameter, it's pretty concerning to me that frame_url doesn't match last_committed_url_ here. Let's understand whether there's a deeper (possibly security) bug here before removing this.
On 2017/05/04 23:26:18, Charlie Reis wrote: > Ah, great, you have repro steps on the bug! Please add a test for that case, > since we clearly don't have test coverage for it today. What test are you thinking about?
On 2017/05/04 23:29:35, Avi (ping after 24h) wrote: > On 2017/05/04 23:26:18, Charlie Reis wrote: > > Ah, great, you have repro steps on the bug! Please add a test for that case, > > since we clearly don't have test coverage for it today. > > What test are you thinking about? I mean a test that makes the current DCHECK fail (i.e., a simplified version of https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_onbeforeunload plus reload).
On 2017/05/04 23:31:09, Charlie Reis wrote: > On 2017/05/04 23:29:35, Avi (ping after 24h) wrote: > > On 2017/05/04 23:26:18, Charlie Reis wrote: > > > Ah, great, you have repro steps on the bug! Please add a test for that > case, > > > since we clearly don't have test coverage for it today. > > > > What test are you thinking about? > > I mean a test that makes the current DCHECK fail (i.e., a simplified version of > https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_onbeforeunload plus > reload). Right, but you add a test that fails so that you can fix it. The ultimate fix is to remove the DCHECK and the un-needed parameter. You want: 1. Figure out how the mismatch occurs 2. Fix whatever random piece of nav is broken ... When we drop the DCHECK, because we really don't need that IPC parameter, we drop the possibility of writing a test to it, which is what you're asking. When we figure out what piece of nav is broken, we can write a test to that, but writing a test to this DCHECK doesn't make sense.
On 2017/05/04 23:34:22, Avi (ping after 24h) wrote: > On 2017/05/04 23:31:09, Charlie Reis wrote: > > On 2017/05/04 23:29:35, Avi (ping after 24h) wrote: > > > On 2017/05/04 23:26:18, Charlie Reis wrote: > > > > Ah, great, you have repro steps on the bug! Please add a test for that > > case, > > > > since we clearly don't have test coverage for it today. > > > > > > What test are you thinking about? > > > > I mean a test that makes the current DCHECK fail (i.e., a simplified version > of > > https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_onbeforeunload > plus > > reload). > > Right, but you add a test that fails so that you can fix it. The ultimate fix is > to remove the DCHECK and the un-needed parameter. > > You want: > 1. Figure out how the mismatch occurs > 2. Fix whatever random piece of nav is broken > ... > > When we drop the DCHECK, because we really don't need that IPC parameter, we > drop the possibility of writing a test to it, which is what you're asking. When > we figure out what piece of nav is broken, we can write a test to that, but > writing a test to this DCHECK doesn't make sense. My point is that this isn't an invalid DCHECK. It's there for a good reason: we assume that the RenderFrameHost has an accurate representation of what page is loaded in the RenderFrameImpl stored in last_committed_url_. Apparently that is not correct in this case, so we should have a test exercising the path that we didn't know about before. Maybe it's a bug, or maybe we just need to know it's possible when writing future code. Independently, I'm ok with keeping or removing the DCHECK. We could keep it because it might have caught a real bug that affects other code, and could catch more bugs in the future. Or we could remove it because it's awkward to include the frame_url in the IPC solely for this and because it may have now served its purpose. We can answer that question when we know more about whether there's a real bug here. Either way, I think we should have a test for the case that caused it to fail, to help us understand what the code is doing and avoid adding incorrect DCHECKs or logic in the future.
On 2017/05/04 23:44:25, Charlie Reis wrote: > It's there for a good reason: we > assume that the RenderFrameHost has an accurate representation of what page is > loaded in the RenderFrameImpl stored in last_committed_url_. Yes and no. Yes, we assume that last_committed_url_ is valid. No, it wasn't added for that. Nasko ran across the fact that the url was being sent via IPC, thought it was weird, and added the DCHECK a week ago. https://codereview.chromium.org/2846983002 > Either way, I think we should have a test for the case that caused it to fail Step one is yes, understanding what's going on here. Beyond that I don't know.
On 2017/05/04 23:56:27, Avi (ping after 24h) wrote: > On 2017/05/04 23:44:25, Charlie Reis wrote: > > It's there for a good reason: we > > assume that the RenderFrameHost has an accurate representation of what page is > > loaded in the RenderFrameImpl stored in last_committed_url_. > > Yes and no. > > Yes, we assume that last_committed_url_ is valid. No, it wasn't added for that. > Nasko ran across the fact that the url was being sent via IPC, thought it was > weird, and added the DCHECK a week ago. > https://codereview.chromium.org/2846983002 Thanks for the context-- I didn't realize that part. > > Either way, I think we should have a test for the case that caused it to fail > > Step one is yes, understanding what's going on here. Beyond that I don't know. We agree on step one-- let's understand it. I don't understand your objection to the test, though. If we'd had the test last week, then Nasko's CL to add the DCHECK wouldn't have made it through the try jobs, and we would have understood why. I want to prevent making the same mistake in the future.
On 2017/05/05 00:00:52, Charlie Reis wrote: > On 2017/05/04 23:56:27, Avi (ping after 24h) wrote: > > On 2017/05/04 23:44:25, Charlie Reis wrote: > > > It's there for a good reason: we > > > assume that the RenderFrameHost has an accurate representation of what page > is > > > loaded in the RenderFrameImpl stored in last_committed_url_. > > > > Yes and no. > > > > Yes, we assume that last_committed_url_ is valid. No, it wasn't added for > that. > > Nasko ran across the fact that the url was being sent via IPC, thought it was > > weird, and added the DCHECK a week ago. > > https://codereview.chromium.org/2846983002 > > Thanks for the context-- I didn't realize that part. > > > > Either way, I think we should have a test for the case that caused it to > fail > > > > Step one is yes, understanding what's going on here. Beyond that I don't know. > > We agree on step one-- let's understand it. > > I don't understand your objection to the test, though. If we'd had the test > last week, then Nasko's CL to add the DCHECK wouldn't have made it through the > try jobs, and we would have understood why. I want to prevent making the same > mistake in the future. I'm slightly cranky. This DCHECK firing is directly in the way of my beforeunload patch, which I was revving for your final approval but is now blocked on this issue. Whee. I'll be looking into this bug because it's directly blocking me now.
On 2017/05/05 00:04:08, Avi (ping after 24h) wrote: > On 2017/05/05 00:00:52, Charlie Reis wrote: > > On 2017/05/04 23:56:27, Avi (ping after 24h) wrote: > > > On 2017/05/04 23:44:25, Charlie Reis wrote: > > > > It's there for a good reason: we > > > > assume that the RenderFrameHost has an accurate representation of what > page > > is > > > > loaded in the RenderFrameImpl stored in last_committed_url_. > > > > > > Yes and no. > > > > > > Yes, we assume that last_committed_url_ is valid. No, it wasn't added for > > that. > > > Nasko ran across the fact that the url was being sent via IPC, thought it > was > > > weird, and added the DCHECK a week ago. > > > https://codereview.chromium.org/2846983002 > > > > Thanks for the context-- I didn't realize that part. > > > > > > Either way, I think we should have a test for the case that caused it to > > fail > > > > > > Step one is yes, understanding what's going on here. Beyond that I don't > know. > > > > We agree on step one-- let's understand it. > > > > I don't understand your objection to the test, though. If we'd had the test > > last week, then Nasko's CL to add the DCHECK wouldn't have made it through the > > try jobs, and we would have understood why. I want to prevent making the same > > mistake in the future. > > I'm slightly cranky. This DCHECK firing is directly in the way of my > beforeunload patch, which I was revving for your final approval but is now > blocked on this issue. Whee. > > I'll be looking into this bug because it's directly blocking me now. I see. If you want to remove the DCHECK and add the test without fully investigating why it's happening, I'm ok with following up on the investigation later, to unblock your other CL.
On 2017/05/05 00:08:21, Charlie Reis wrote: > On 2017/05/05 00:04:08, Avi (ping after 24h) wrote: > > On 2017/05/05 00:00:52, Charlie Reis wrote: > > > On 2017/05/04 23:56:27, Avi (ping after 24h) wrote: > > > > On 2017/05/04 23:44:25, Charlie Reis wrote: > > > > > It's there for a good reason: we > > > > > assume that the RenderFrameHost has an accurate representation of what > > page > > > is > > > > > loaded in the RenderFrameImpl stored in last_committed_url_. > > > > > > > > Yes and no. > > > > > > > > Yes, we assume that last_committed_url_ is valid. No, it wasn't added for > > > that. > > > > Nasko ran across the fact that the url was being sent via IPC, thought it > > was > > > > weird, and added the DCHECK a week ago. > > > > https://codereview.chromium.org/2846983002 > > > > > > Thanks for the context-- I didn't realize that part. > > > > > > > > Either way, I think we should have a test for the case that caused it to > > > fail > > > > > > > > Step one is yes, understanding what's going on here. Beyond that I don't > > know. > > > > > > We agree on step one-- let's understand it. > > > > > > I don't understand your objection to the test, though. If we'd had the test > > > last week, then Nasko's CL to add the DCHECK wouldn't have made it through > the > > > try jobs, and we would have understood why. I want to prevent making the > same > > > mistake in the future. > > > > I'm slightly cranky. This DCHECK firing is directly in the way of my > > beforeunload patch, which I was revving for your final approval but is now > > blocked on this issue. Whee. > > > > I'll be looking into this bug because it's directly blocking me now. > > I see. If you want to remove the DCHECK and add the test without fully > investigating why it's happening, I'm ok with following up on the investigation > later, to unblock your other CL. I'll take a quick look to see if there's an easy answer. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
