Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522073002:
[OfflinePages] RequestCoordinatorFactory return null for incognito (Closed)
|
Created:
4 years, 1 month ago by dougarnett Modified:
4 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, dewittj+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, gavinp+prer_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[OfflinePages] RequestCoordinatorFactory return null for incognito profile and check return value at call sites.
BUG=667463
Committed: https://crrev.com/a324d3e5e1dc8dbb297b9a85509cbb3cc3ef5f24
Cr-Commit-Position: refs/heads/master@{#435351}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Added CHECK in NetErrorTabHelper as well #
Total comments: 3
Patch Set 3 : Dropped GetBrowserContextToUse() per Filip #
Total comments: 7
Patch Set 4 : Avoid calling SavePageLater in no Coordinator created #Patch Set 5 : Removed SavePageLater check on this #Patch Set 6 : Check RequestCoordinatorFactory returning null in other call sites #Patch Set 7 : Merge #
Total comments: 8
Patch Set 8 : Updates per fgorski feedback #
Total comments: 6
Patch Set 9 : Dropped DCHECK #Patch Set 10 : Swapping back to DCHECK in net_error_tab_helper and dropping if-return per jianli #Patch Set 11 : Merge #
Messages
Total messages: 71 (35 generated)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + fgorski@chromium.org, jianli@chromium.org, petewil@chromium.org
I also looked at changing net_error_tab_helper.cc to check value returned from factory or IsOffTheRecord and not call SavePageLater but then we just end up with snackbar claiming we are Downloading when we are not so such change would need more work to present error. Seems better to just not offer the Download button so just keeping the scope of this change to fail fast.
On 2016/11/22 21:11:03, dougarnett wrote: > I also looked at changing net_error_tab_helper.cc to check value returned from > factory or IsOffTheRecord and not call SavePageLater but then we just end up > with snackbar claiming we are Downloading when we are not so such change would > need more work to present error. Seems better to just not offer the Download > button so just keeping the scope of this change to fail fast. Should we also add a null check before the call to SavePageLater()?
The CQ bit was checked by dougarnett@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...
On 2016/11/22 21:15:37, Pete Williamson wrote: > On 2016/11/22 21:11:03, dougarnett wrote: > > I also looked at changing net_error_tab_helper.cc to check value returned from > > factory or IsOffTheRecord and not call SavePageLater but then we just end up > > with snackbar claiming we are Downloading when we are not so such change would > > need more work to present error. Seems better to just not offer the Download > > button so just keeping the scope of this change to fail fast. > > Should we also add a null check before the call to SavePageLater()? Ok added a check - see if want you meant.
comments to patchset 1. https://codereview.chromium.org/2522073002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2522073002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:53: DVLOG(1) << "Can't create RequestCoordinator service for incognito mode"; this change is not what you want. Remove GetBrowserContextToUse, which will use nullptr in place of offtherecord... https://codereview.chromium.org/2522073002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:89: content::BrowserContext* RequestCoordinatorFactory::GetBrowserContextToUse( this is the code that needs to go. https://codereview.chromium.org/2522073002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2522073002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:199: CHECK(this); I don't think this is a good strategy. (It does not contribute.) If you fix the factory, this is not necessary.
comments on patch 2 https://codereview.chromium.org/2522073002/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:284: CHECK(request_coordinator); check is guaranteeing a crash in incognito and I am not sure that is what you mean. I would let Jian Li handle this part properly.
https://codereview.chromium.org/2522073002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2522073002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:53: DVLOG(1) << "Can't create RequestCoordinator service for incognito mode"; On 2016/11/22 21:43:37, fgorski wrote: > this change is not what you want. Remove GetBrowserContextToUse, which will use > nullptr in place of offtherecord... Ok, I will revisit that. I tried that originally but didn't work because I hadn't figured out that SavePageLater was still being called with null this pointer. So I expect it will work as long as we also have check for null RequestCoordinator pointer. https://codereview.chromium.org/2522073002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2522073002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:199: CHECK(this); On 2016/11/22 21:43:37, fgorski wrote: > I don't think this is a good strategy. (It does not contribute.) > > If you fix the factory, this is not necessary. I found the Factory returning null was not sufficient, this method was getting called with null this pointer. We can fix the caller to check the Factory result but that doesn't stop a new caller from hitting the same problem if they don't check the factory result. https://codereview.chromium.org/2522073002/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:284: CHECK(request_coordinator); On 2016/11/22 21:45:14, fgorski wrote: > check is guaranteeing a crash in incognito and I am not sure that is what you > mean. > > I would let Jian Li handle this part properly. Yes, that was my original plan. Was trying to respond to Pete comment. Happy to drop or keep. Crash means we find issue earlier and fix rather than someone clicking on button and seeing the Downloading snackbar but nothing happening (which would really need different snackbar message). Not presenting the button in the first place is the clean fix so perhaps an assertion makes more sense here than error UX - if we want to check here. It could be DCHECK (and expect to crash later or for no progress ever downstream for non-debug build).
https://codereview.chromium.org/2522073002/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:284: CHECK(request_coordinator); On 2016/11/22 22:01:16, dougarnett wrote: > On 2016/11/22 21:45:14, fgorski wrote: > > check is guaranteeing a crash in incognito and I am not sure that is what you > > mean. > > > > I would let Jian Li handle this part properly. > > Yes, that was my original plan. Was trying to respond to Pete comment. > Happy to drop or keep. > > Crash means we find issue earlier and fix rather than someone clicking on button > and seeing the Downloading snackbar but nothing happening (which would really > need different snackbar message). Not presenting the button in the first place > is the clean fix so perhaps an assertion makes more sense here than error UX - > if we want to check here. It could be DCHECK (and expect to crash later or for > no progress ever downstream for non-debug build). > The fix for not showing download page later button under incognito has been landed. I don't see adding DCHECK or CHECK here adds some extra value since it will crash at line 287 if for some reason request_coordinator is null.
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2522073002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2522073002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:53: DVLOG(1) << "Can't create RequestCoordinator service for incognito mode"; On 2016/11/22 22:01:16, dougarnett wrote: > On 2016/11/22 21:43:37, fgorski wrote: > > this change is not what you want. Remove GetBrowserContextToUse, which will > use > > nullptr in place of offtherecord... > > Ok, I will revisit that. I tried that originally but didn't work because I > hadn't figured out that SavePageLater was still being called with null this > pointer. So I expect it will work as long as we also have check for null > RequestCoordinator pointer. Done. https://codereview.chromium.org/2522073002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:89: content::BrowserContext* RequestCoordinatorFactory::GetBrowserContextToUse( On 2016/11/22 21:43:37, fgorski wrote: > this is the code that needs to go. > Acknowledged.
https://codereview.chromium.org/2522073002/diff/40001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:284: DCHECK(request_coordinator); Doug, if request_coordinator is null here, you will still crash in 287 (in production)/ A common pattern to deal with that is to check returned service: if (!request_coordinator) return; compare with offline_page_utils or pretty much any other usage of keyed services. Having DCHECK here is OK (e.g. just before return;), but please handle non debug case appropriately too. Use this as a place to start. https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/dow... https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:199: CHECK(this); I am still not OK with this, and it does not seem to be a pattern used in chromium. https://cs.chromium.org/search/?q=CHECK%5C(this%5C)&sq=package:chromium&type=cs
On 2016/11/22 22:06:36, jianli wrote: > https://codereview.chromium.org/2522073002/diff/20001/chrome/browser/net/net_... > File chrome/browser/net/net_error_tab_helper.cc (right): > > https://codereview.chromium.org/2522073002/diff/20001/chrome/browser/net/net_... > chrome/browser/net/net_error_tab_helper.cc:284: CHECK(request_coordinator); > On 2016/11/22 22:01:16, dougarnett wrote: > > On 2016/11/22 21:45:14, fgorski wrote: > > > check is guaranteeing a crash in incognito and I am not sure that is what > you > > > mean. > > > > > > I would let Jian Li handle this part properly. > > > > Yes, that was my original plan. Was trying to respond to Pete comment. > > Happy to drop or keep. > > > > Crash means we find issue earlier and fix rather than someone clicking on > button > > and seeing the Downloading snackbar but nothing happening (which would really > > need different snackbar message). Not presenting the button in the first place > > is the clean fix so perhaps an assertion makes more sense here than error UX - > > if we want to check here. It could be DCHECK (and expect to crash later or for > > no progress ever downstream for non-debug build). > > > > The fix for not showing download page later button under incognito has been > landed. I don't see adding DCHECK or CHECK here adds some extra value since it > will crash at line 287 if for some reason request_coordinator is null. I totally agree that it looks that way (certainly would for Java). But I swear that it was not happening for me (driving me bonkers). I had log messages in SavePageLater that were logging just after log messages from Factory reporting no instance created. Pete suggested just DCHECK. Perhaps another option to this odd situation might be to define SavePageLater() as virtual method so that it will crash for vtable access?
https://codereview.chromium.org/2522073002/diff/40001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:284: DCHECK(request_coordinator); Actually, what I had in mind was not a DCHECK, but if (request_coordinator) { request_coordinator->SavePageLater(...) } So we don't crash retail chrome if this path is somehow executed with a null request_coordinator. It would also be fine to do something like Filip suggests: if (!request_coordinator) return; https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:199: CHECK(this); On 2016/11/22 22:46:33, fgorski wrote: > I am still not OK with this, and it does not seem to be a pattern used in > chromium. > > https://cs.chromium.org/search/?q=CHECK%5C(this%5C)&sq=package:chromium&type=cs This might be better as a DCHECK. The idea is to catch new calling code paths that don't set request_coordinator first.
https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:199: CHECK(this); On 2016/11/22 22:59:32, Pete Williamson wrote: > On 2016/11/22 22:46:33, fgorski wrote: > > I am still not OK with this, and it does not seem to be a pattern used in > > chromium. > > > > > https://cs.chromium.org/search/?q=CHECK%5C(this%5C)&sq=package:chromium&type=cs > > This might be better as a DCHECK. The idea is to catch new calling code paths > that don't set request_coordinator first. I agree with Filip that this was not a good pattern to use in Chrome. If we add this here, do we want to add for other methods? We should rely on crash caused by accessing null pointer.
On 2016/11/22 23:03:55, jianli wrote: > https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... > File components/offline_pages/background/request_coordinator.cc (right): > > https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... > components/offline_pages/background/request_coordinator.cc:199: CHECK(this); > On 2016/11/22 22:59:32, Pete Williamson wrote: > > On 2016/11/22 22:46:33, fgorski wrote: > > > I am still not OK with this, and it does not seem to be a pattern used in > > > chromium. > > > > > > > > > https://cs.chromium.org/search/?q=CHECK%5C(this%5C)&sq=package:chromium&type=cs > > > > This might be better as a DCHECK. The idea is to catch new calling code paths > > that don't set request_coordinator first. > > I agree with Filip that this was not a good pattern to use in Chrome. If we add > this here, do we want to add for other methods? > > We should rely on crash caused by accessing null pointer. I think I will rework to check at all current call sites instead. The interesting point to be aware of though is that I actually hit this check.
The CQ bit was checked by dougarnett@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== [OfflinePages] Fail fast if SavePageLater called with incognito profile We see crashes with Async Loading from error page in incognito profile. It is confusing because it was failing later, at load processing time, when no OfflinePageModel is available in incognito mode. We should have checks higher up the stack but also we should fail faster in case new uses trigger this. The latter is what this CL is addressing. It has the RequestCoordinator factory return null for incognito profile and also adds CHECK on this point in SavePageLater in case the caller does not check the return value from the factory (actually was experiencing SavePageLater called with null this). BUG=667463 ========== to ========== [OfflinePages] Check RequestCoordinatorFactory return value BUG=667463 ==========
Description was changed from ========== [OfflinePages] Check RequestCoordinatorFactory return value BUG=667463 ========== to ========== [OfflinePages] Check RequestCoordinatorFactory return value. I've seen SavePageLater (non-virtual method) get called on null RequestCoordinator value returned from the factory so we need to be sure to always check the factory's return value before using it. BUG=667463 ==========
The CQ bit was checked by dougarnett@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...
PTAL - I checked the other RequestCoordinatorFactory call sites and added checks on its return value if not already handled. I added LOG(WARNING)'s in offline_pages_download_bridge.cc per the style existing in that file for existing checks for Pause/Resume.
Missed the Done's last response ... https://codereview.chromium.org/2522073002/diff/40001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/40001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:284: DCHECK(request_coordinator); On 2016/11/22 22:59:32, Pete Williamson wrote: > Actually, what I had in mind was not a DCHECK, but > if (request_coordinator) { > request_coordinator->SavePageLater(...) > } > > So we don't crash retail chrome if this path is somehow executed with a null > request_coordinator. > > It would also be fine to do something like Filip suggests: > if (!request_coordinator) > return; Done. https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2522073002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:199: CHECK(this); On 2016/11/22 23:03:55, jianli wrote: > On 2016/11/22 22:59:32, Pete Williamson wrote: > > On 2016/11/22 22:46:33, fgorski wrote: > > > I am still not OK with this, and it does not seem to be a pattern used in > > > chromium. > > > > > > > > > https://cs.chromium.org/search/?q=CHECK%5C(this%5C)&sq=package:chromium&type=cs > > > > This might be better as a DCHECK. The idea is to catch new calling code paths > > that don't set request_coordinator first. > > I agree with Filip that this was not a good pattern to use in Chrome. If we add > this here, do we want to add for other methods? > > We should rely on crash caused by accessing null pointer. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks for working on this. It looks much better, but I would document your expectations related to dchecks with explicit comments. Also, I think you can putt << after dcheck to print something with the failure. https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:91: LOG(WARNING) << "SavePageIfNotNavigatedAway has no valid coordinator."; Use DVLOG, as this is expected behavior for incognito. https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:109: LOG(WARNING) << "SavePageIfNotNavigatedAway has no valid coordinator."; ditto https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner_factory.cc (right): https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner_factory.cc:37: DCHECK(model); Will this DCHECK trigger in incognito as well? It is OK to dcheck if you know it will not crash the incognito, provided that you want to crash in any other situation where model could be nullptr... https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:284: DCHECK(request_coordinator); Again. What is the purpose of this dcheck? Please explain and add a comment.
Description was changed from ========== [OfflinePages] Check RequestCoordinatorFactory return value. I've seen SavePageLater (non-virtual method) get called on null RequestCoordinator value returned from the factory so we need to be sure to always check the factory's return value before using it. BUG=667463 ========== to ========== [OfflinePages] RequestCoordinatorFactory return null for incognito profile and check return value at call sites. BUG=667463 ==========
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:91: LOG(WARNING) << "SavePageIfNotNavigatedAway has no valid coordinator."; On 2016/11/23 19:05:30, fgorski wrote: > Use DVLOG, as this is expected behavior for incognito. > Done. https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:109: LOG(WARNING) << "SavePageIfNotNavigatedAway has no valid coordinator."; On 2016/11/23 19:05:30, fgorski wrote: > ditto Done. https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner_factory.cc (right): https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner_factory.cc:37: DCHECK(model); On 2016/11/23 19:05:30, fgorski wrote: > Will this DCHECK trigger in incognito as well? > > It is OK to dcheck if you know it will not crash the incognito, provided that > you want to crash in any other situation where model could be nullptr... We should not reach here in incognito mode because there should be no RequestCoordinator created now in incognito mode to hold this factory. DCHECK will let us crash here rather than later, at archive time if we have no model. https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/120001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:284: DCHECK(request_coordinator); On 2016/11/23 19:05:30, fgorski wrote: > Again. What is the purpose of this dcheck? > Please explain and add a comment. Added brief comment and << string. Basically since incognito mode might not be exercised commonly by devs (eg, recently crashing bug here), we should try to catch this case and fix since otherwise we have a silently not working feature which could be very hard to identify. There are other silently failing cases though so if that is the pattern in chrome, I can remove. That seems fine if we have full test passes over features in incognito to catch these things.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. All my concerns are taken care of, OK to checkin when other reviewers are happy.
dougarnett@chromium.org changed reviewers: + mmenke@chromium.org
Matt, can you do owners review of net_error_tab_helper.cc?
https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:285: DCHECK(request_coordinator) << "No RequestCoordinator for SavePageLater"; I don't understand why we need both a DCHECK and code to handle this situation? If it's expected not to happen, we just need the DCHECK. If it's expected to happen and we should handle it, we need just the if.
https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:285: DCHECK(request_coordinator) << "No RequestCoordinator for SavePageLater"; On 2016/11/28 19:03:19, mmenke wrote: > I don't understand why we need both a DCHECK and code to handle this situation? > If it's expected not to happen, we just need the DCHECK. If it's expected to > happen and we should handle it, we need just the if. This will be null currently for incognito profile. The DCHECK is to allow us to catch an issue up the stack calling it in incognito mode on debug builds vs. silently not doing anything (especially since this is an asynchronous operation subject to gaining network connection). There was a separate fix by jianli to not present the Download Later button in the UI for incognito profile to stop this from happening - which is the proper fix. So I don't need the DCHECK now but it was more about considering a pattern for catching this won't-really-do-anything scenario without near term feedback. Seem worthwhile or should I remove the DCHECK here?
https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:285: DCHECK(request_coordinator) << "No RequestCoordinator for SavePageLater"; On 2016/11/28 19:27:14, dougarnett wrote: > On 2016/11/28 19:03:19, mmenke wrote: > > I don't understand why we need both a DCHECK and code to handle this > situation? > > If it's expected not to happen, we just need the DCHECK. If it's expected to > > happen and we should handle it, we need just the if. > > This will be null currently for incognito profile. The DCHECK is to allow us to > catch an issue up the stack calling it in incognito mode on debug builds vs. > silently not doing anything (especially since this is an asynchronous operation > subject to gaining network connection). > > There was a separate fix by jianli to not present the Download Later button in > the UI for incognito profile to stop this from happening - which is the proper > fix. So I don't need the DCHECK now but it was more about considering a pattern > for catching this won't-really-do-anything scenario without near term feedback. > > Seem worthwhile or should I remove the DCHECK here? If this will currently be NULL in production code, the DCHECK is not correct, and should be removed. If you (Or jianli) later (Or before this lands) changes the code so that request_coordinator should never be non-NULL when this method is called, you should then remove this check and replace it with a DCHECK.
https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:285: DCHECK(request_coordinator) << "No RequestCoordinator for SavePageLater"; On 2016/11/28 19:31:06, mmenke wrote: > On 2016/11/28 19:27:14, dougarnett wrote: > > On 2016/11/28 19:03:19, mmenke wrote: > > > I don't understand why we need both a DCHECK and code to handle this > > situation? > > > If it's expected not to happen, we just need the DCHECK. If it's expected > to > > > happen and we should handle it, we need just the if. > > > > This will be null currently for incognito profile. The DCHECK is to allow us > to > > catch an issue up the stack calling it in incognito mode on debug builds vs. > > silently not doing anything (especially since this is an asynchronous > operation > > subject to gaining network connection). > > > > There was a separate fix by jianli to not present the Download Later button in > > the UI for incognito profile to stop this from happening - which is the proper > > fix. So I don't need the DCHECK now but it was more about considering a > pattern > > for catching this won't-really-do-anything scenario without near term > feedback. > > > > Seem worthwhile or should I remove the DCHECK here? > > If this will currently be NULL in production code, the DCHECK is not correct, > and should be removed. > > If you (Or jianli) later (Or before this lands) changes the code so that > request_coordinator should never be non-NULL when this method is called, you > should then remove this check and replace it with a DCHECK. Dropped the DCHECK
On 2016/11/28 20:34:20, dougarnett wrote: > https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... > File chrome/browser/net/net_error_tab_helper.cc (right): > > https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... > chrome/browser/net/net_error_tab_helper.cc:285: DCHECK(request_coordinator) << > "No RequestCoordinator for SavePageLater"; > On 2016/11/28 19:31:06, mmenke wrote: > > On 2016/11/28 19:27:14, dougarnett wrote: > > > On 2016/11/28 19:03:19, mmenke wrote: > > > > I don't understand why we need both a DCHECK and code to handle this > > > situation? > > > > If it's expected not to happen, we just need the DCHECK. If it's expected > > to > > > > happen and we should handle it, we need just the if. > > > > > > This will be null currently for incognito profile. The DCHECK is to allow us > > to > > > catch an issue up the stack calling it in incognito mode on debug builds vs. > > > silently not doing anything (especially since this is an asynchronous > > operation > > > subject to gaining network connection). > > > > > > There was a separate fix by jianli to not present the Download Later button > in > > > the UI for incognito profile to stop this from happening - which is the > proper > > > fix. So I don't need the DCHECK now but it was more about considering a > > pattern > > > for catching this won't-really-do-anything scenario without near term > > feedback. > > > > > > Seem worthwhile or should I remove the DCHECK here? > > > > If this will currently be NULL in production code, the DCHECK is not correct, > > and should be removed. > > > > If you (Or jianli) later (Or before this lands) changes the code so that > > request_coordinator should never be non-NULL when this method is called, you > > should then remove this check and replace it with a DCHECK. > > Dropped the DCHECK LGTM
lgtm https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:285: DCHECK(request_coordinator) << "No RequestCoordinator for SavePageLater"; On 2016/11/28 20:34:20, dougarnett wrote: > On 2016/11/28 19:31:06, mmenke wrote: > > On 2016/11/28 19:27:14, dougarnett wrote: > > > On 2016/11/28 19:03:19, mmenke wrote: > > > > I don't understand why we need both a DCHECK and code to handle this > > > situation? > > > > If it's expected not to happen, we just need the DCHECK. If it's expected > > to > > > > happen and we should handle it, we need just the if. > > > > > > This will be null currently for incognito profile. The DCHECK is to allow us > > to > > > catch an issue up the stack calling it in incognito mode on debug builds vs. > > > silently not doing anything (especially since this is an asynchronous > > operation > > > subject to gaining network connection). > > > > > > There was a separate fix by jianli to not present the Download Later button > in > > > the UI for incognito profile to stop this from happening - which is the > proper > > > fix. So I don't need the DCHECK now but it was more about considering a > > pattern > > > for catching this won't-really-do-anything scenario without near term > > feedback. > > > > > > Seem worthwhile or should I remove the DCHECK here? > > > > If this will currently be NULL in production code, the DCHECK is not correct, > > and should be removed. > > > > If you (Or jianli) later (Or before this lands) changes the code so that > > request_coordinator should never be non-NULL when this method is called, you > > should then remove this check and replace it with a DCHECK. > > Dropped the DCHECK I rather to stick with DCHECK in order to catch the error earlier. I already landed the patch not to shown DOWNLOAD PAGE LATER button under incognito some days ago. So we should never hit this with nullptr.
https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2522073002/diff/140001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:285: DCHECK(request_coordinator) << "No RequestCoordinator for SavePageLater"; On 2016/11/29 22:05:02, jianli wrote: > On 2016/11/28 20:34:20, dougarnett wrote: > > On 2016/11/28 19:31:06, mmenke wrote: > > > On 2016/11/28 19:27:14, dougarnett wrote: > > > > On 2016/11/28 19:03:19, mmenke wrote: > > > > > I don't understand why we need both a DCHECK and code to handle this > > > > situation? > > > > > If it's expected not to happen, we just need the DCHECK. If it's > expected > > > to > > > > > happen and we should handle it, we need just the if. > > > > > > > > This will be null currently for incognito profile. The DCHECK is to allow > us > > > to > > > > catch an issue up the stack calling it in incognito mode on debug builds > vs. > > > > silently not doing anything (especially since this is an asynchronous > > > operation > > > > subject to gaining network connection). > > > > > > > > There was a separate fix by jianli to not present the Download Later > button > > in > > > > the UI for incognito profile to stop this from happening - which is the > > proper > > > > fix. So I don't need the DCHECK now but it was more about considering a > > > pattern > > > > for catching this won't-really-do-anything scenario without near term > > > feedback. > > > > > > > > Seem worthwhile or should I remove the DCHECK here? > > > > > > If this will currently be NULL in production code, the DCHECK is not > correct, > > > and should be removed. > > > > > > If you (Or jianli) later (Or before this lands) changes the code so that > > > request_coordinator should never be non-NULL when this method is called, you > > > should then remove this check and replace it with a DCHECK. > > > > Dropped the DCHECK > > I rather to stick with DCHECK in order to catch the error earlier. I already > landed the patch not to shown DOWNLOAD PAGE LATER button under incognito some > days ago. So we should never hit this with nullptr. Done.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, fgorski@chromium.org, jianli@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2522073002/#ps180001 (title: "Swapping back to DCHECK in net_error_tab_helper and dropping if-return per jianli")
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dougarnett@chromium.org
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
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dougarnett@chromium.org
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
Failed to apply patch for
chrome/browser/android/offline_pages/offline_page_utils.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/android/offline_pages/offline_page_utils.cc:180
error: chrome/browser/android/offline_pages/offline_page_utils.cc: patch does
not apply
Patch: chrome/browser/android/offline_pages/offline_page_utils.cc
Index: chrome/browser/android/offline_pages/offline_page_utils.cc
diff --git a/chrome/browser/android/offline_pages/offline_page_utils.cc
b/chrome/browser/android/offline_pages/offline_page_utils.cc
index
c4e3fcae56fb6caa97ba6d51ffb9a33cb055a830..8b2b37ab92c92ac5161b3ca8a5ffe8137d98c3ff
100644
--- a/chrome/browser/android/offline_pages/offline_page_utils.cc
+++ b/chrome/browser/android/offline_pages/offline_page_utils.cc
@@ -180,6 +180,8 @@ void OfflinePageUtils::CheckExistenceOfRequestsWithURL(
const base::Callback<void(bool)>& callback) {
RequestCoordinator* request_coordinator =
RequestCoordinatorFactory::GetForBrowserContext(browser_context);
+ if (!request_coordinator)
+ return;
auto request_coordinator_continuation = [](
const std::string& name_space, const GURL& offline_page_url,
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, jianli@chromium.org, mmenke@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2522073002/#ps200001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480526580068430,
"parent_rev": "298780096679e396290ec848eb7abc4a8d8abf8c", "commit_rev":
"0e05c5d283eb44c55ce34c22791ffff27acb9034"}
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] RequestCoordinatorFactory return null for incognito profile and check return value at call sites. BUG=667463 ========== to ========== [OfflinePages] RequestCoordinatorFactory return null for incognito profile and check return value at call sites. BUG=667463 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] RequestCoordinatorFactory return null for incognito profile and check return value at call sites. BUG=667463 ========== to ========== [OfflinePages] RequestCoordinatorFactory return null for incognito profile and check return value at call sites. BUG=667463 Committed: https://crrev.com/a324d3e5e1dc8dbb297b9a85509cbb3cc3ef5f24 Cr-Commit-Position: refs/heads/master@{#435351} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/a324d3e5e1dc8dbb297b9a85509cbb3cc3ef5f24 Cr-Commit-Position: refs/heads/master@{#435351} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
