Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
265743003:
Change a DCHECK to CHECK in chrome::ExtractSearchTermsFromURL(). (Closed)
|
Created:
6 years, 7 months ago by kmadhusu Modified:
6 years, 7 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChange a DCHECK to CHECK in chrome::ExtractSearchTermsFromURL() to help debug a crash in release builds.
TBR=samarth@chromium.org
BUG=367204
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267722
Patch Set 1 #
Total comments: 1
Patch Set 2 : Change DCHECK -> CHECK #Messages
Total messages: 16 (0 generated)
Please review. Thanks.
Is there a unit test that would exercise this? If not, can there be one? https://codereview.chromium.org/265743003/diff/1/chrome/browser/search/search.cc File chrome/browser/search/search.cc (right): https://codereview.chromium.org/265743003/diff/1/chrome/browser/search/search... chrome/browser/search/search.cc:408: // page. Nit: Add an explanation of when this can be NULL (e.g. "when testing", "if window x has closed", etc. -- ideally not just "when function foo() returns NULL").
On 2014/05/01 20:21:02, Peter Kasting wrote: > Is there a unit test that would exercise this? If not, can there be one? > > https://codereview.chromium.org/265743003/diff/1/chrome/browser/search/search.cc > File chrome/browser/search/search.cc (right): > > https://codereview.chromium.org/265743003/diff/1/chrome/browser/search/search... > chrome/browser/search/search.cc:408: // page. > Nit: Add an explanation of when this can be NULL (e.g. "when testing", "if > window x has closed", etc. -- ideally not just "when function foo() returns > NULL"). I am unable to repro this crash locally. Therefore, its hard to state the exact condition when this will become NULL. Because of that, I am unable to write a unit test to exercise that workflow. Thanks.
On 2014/05/01 20:39:32, kmadhusu wrote: > I am unable to repro this crash locally. Therefore, its hard to state the exact > condition when this will become NULL. Because of that, I am unable to write a > unit test to exercise that workflow. That worries me, because that calls into question whether this fix is correct, or the callers should ensure this is non-NULL. On the bug, you said you had "several" reasons why this could be NULL. What are they? Can you mentally trace through the code flow enough to figure out what's going on? NULL-checks tend to spread -- if we make this change now, for example, we lock in for all future readers that NULL is legitimate here, which in turn affects how they read code that calls this function.
On 2014/05/01 20:48:07, Peter Kasting wrote: > On 2014/05/01 20:39:32, kmadhusu wrote: > > I am unable to repro this crash locally. Therefore, its hard to state the > exact > > condition when this will become NULL. Because of that, I am unable to write a > > unit test to exercise that workflow. > > That worries me, because that calls into question whether this fix is correct, > or the callers should ensure this is non-NULL. > > On the bug, you said you had "several" reasons why this could be NULL. What are > they? Can you mentally trace through the code flow enough to figure out what's > going on? > I understand your concern. My guess is "InstantService" is NULL for some reason (either the user is trying to open/navigate to the Instant search base page in an Incognito window, corrupted profile, etc). I tried to exercise the above said workflows. But no luck. Will you be okay if I convert the DCHECK to CHECK here? That will confirm us if this is the right fix. > NULL-checks tend to spread -- if we make this change now, for example, we lock > in for all future readers that NULL is legitimate here, which in turn affects > how they read code that calls this function.
On 2014/05/01 20:54:12, kmadhusu wrote: > Will you be okay if I convert the DCHECK to CHECK here? > That will confirm us if this is the right fix. Sure, upgrading a DCHECK to a CHECK temporarily (make sure to switch it back later) is a great way of trying to track down better callstacks. Feel free to add or upgrade other checks in the pipeline where you're wondering about this while you're at it.
On 2014/05/01 20:57:11, Peter Kasting wrote: > On 2014/05/01 20:54:12, kmadhusu wrote: > > Will you be okay if I convert the DCHECK to CHECK here? > > That will confirm us if this is the right fix. > > Sure, upgrading a DCHECK to a CHECK temporarily (make sure to switch it back > later) is a great way of trying to track down better callstacks. > > Feel free to add or upgrade other checks in the pipeline where you're wondering > about this while you're at it. Done. I would like to keep this CL simple. Once I track this down, I will verify other NULL checks.
LGTM
TBR'ing samarth@ to do the OWNER's check. Thanks.
The CQ bit was checked by kmadhusu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/265743003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
The CQ bit was checked by kmadhusu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/265743003/20001
Message was sent while issue was closed.
Change committed as 267722 |
