Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2880723002:
Favicon url is used only if it is valid (Closed)
|
Created:
3 years, 7 months ago by gambard Modified:
3 years, 7 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, ios-reviews+web_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFavicon url is used only if it is valid
The javascript extracting the favicon urls from a page returns valid and invalid
urls.
The CRWWebController should only return the valid urls, as returning
invalid url leads to a crash.
BUG=720666
Review-Url: https://codereview.chromium.org/2880723002
Cr-Commit-Position: refs/heads/master@{#472722}
Committed: https://chromium.googlesource.com/chromium/src/+/2a03a5de9275d45001da304069313dc7810a63ec
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add the default favicon in native code #
Total comments: 10
Patch Set 3 : Address comments #Patch Set 4 : Change the comment #
Messages
Total messages: 28 (10 generated)
Description was changed from ========== Adds favicon url to be downloaded only if valid The javascript extracting the favicon urls from a page returns only the valid url. It used to also return invalid url, leading to a crash. BUG=720666 ========== to ========== Favicon url is used only if it is valid The javascript extracting the favicon urls from a page returns valid and invalid urls. The CRWWebController should only return the valid urls, as returning invalid url leads to a crash. BUG=720666 ==========
gambard@chromium.org changed reviewers: + eugenebut@chromium.org
PTAL.
The CQ bit was checked by gambard@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: This issue passed the CQ dry run.
Gauthier, is it possible to write a test for prevent this regression in the future? https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2566: GURL url = GURL(href); WebContentsObserver also has DidUpdateFaviconURL. Does content throw away invalid URLs as well? I think we should do the same thing as other platforms, but also update WebStateObserver::FaviconUrlUpdated comments to explicitly say whether or not the result will contain invalid URLs.
I tried to add tests but the function is way too buried inside the class... https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... ios/web/web_state/ui/crw_web_controller.mm:2566: GURL url = GURL(href); On 2017/05/12 15:04:08, Eugene But wrote: > WebContentsObserver also has DidUpdateFaviconURL. Does content throw away > invalid URLs as well? I think we should do the same thing as other platforms, > but also update WebStateObserver::FaviconUrlUpdated comments to explicitly say > whether or not the result will contain invalid URLs. There is a check if the URL is empty https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?sq=... So I guess there might be a check to see if it is valid?
On 2017/05/12 15:43:39, gambard wrote: > I tried to add tests but the function is way too buried inside the class... Look at CRWWebControllerTitleTest.TitleChange for inspiration. You can load html with favicon links and then wait until FaviconUrlUpdated callback gets called (like with TitleWasSet). This will not be exactly a unittest and rather an integration test, but it's the best what we can currently do with WebController. If I have time today, I will write a test for this callback. And if I won't I will let you know. > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > ios/web/web_state/ui/crw_web_controller.mm:2566: GURL url = GURL(href); > On 2017/05/12 15:04:08, Eugene But wrote: > > WebContentsObserver also has DidUpdateFaviconURL. Does content throw away > > invalid URLs as well? I think we should do the same thing as other platforms, > > but also update WebStateObserver::FaviconUrlUpdated comments to explicitly say > > whether or not the result will contain invalid URLs. > > There is a check if the URL is empty > https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?sq=... > So I guess there might be a check to see if it is valid? Thank you for checking that. URL can be non-empty, but still invalid. For example I think customscheme://foo will not be a valid URL, but it's certainly not empty. Would it be better to check for empty url in WebController and check for invalid URL in the embedder? WDYT?
On 2017/05/12 17:10:07, Eugene But wrote: > On 2017/05/12 15:43:39, gambard wrote: > > I tried to add tests but the function is way too buried inside the class... > Look at CRWWebControllerTitleTest.TitleChange for inspiration. You can load html > with favicon links and then wait until FaviconUrlUpdated callback gets called > (like with TitleWasSet). This will not be exactly a unittest and rather an > integration test, but it's the best what we can currently do with WebController. > If I have time today, I will write a test for this callback. And if I won't I > will let you know. > > > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > > File ios/web/web_state/ui/crw_web_controller.mm (right): > > > > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > > ios/web/web_state/ui/crw_web_controller.mm:2566: GURL url = GURL(href); > > On 2017/05/12 15:04:08, Eugene But wrote: > > > WebContentsObserver also has DidUpdateFaviconURL. Does content throw away > > > invalid URLs as well? I think we should do the same thing as other > platforms, > > > but also update WebStateObserver::FaviconUrlUpdated comments to explicitly > say > > > whether or not the result will contain invalid URLs. > > > > There is a check if the URL is empty > > > https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?sq=... > > So I guess there might be a check to see if it is valid? > Thank you for checking that. URL can be non-empty, but still invalid. For > example I think customscheme://foo will not be a valid URL, but it's certainly > not empty. Would it be better to check for empty url in WebController and check > for invalid URL in the embedder? WDYT? Thanks for writing the tests! I checked on a mac version, https://cs.chromium.org/chromium/src/components/favicon/core/favicon_handler.... contains the invalid url. I think in this case the invalid icon is a touch icon so the desktop version might not try to download it. So I think they might hit the same bug even if I did not find any crash report. Looking at the code, I my opinion it would make more sense if the "candidate urls" are valid, so the fix should also be done in content. However I don't have a strong opinion on this.
On 2017/05/15 12:30:22, gambard wrote: > On 2017/05/12 17:10:07, Eugene But wrote: > > On 2017/05/12 15:43:39, gambard wrote: > > > I tried to add tests but the function is way too buried inside the class... > > Look at CRWWebControllerTitleTest.TitleChange for inspiration. You can load > html > > with favicon links and then wait until FaviconUrlUpdated callback gets called > > (like with TitleWasSet). This will not be exactly a unittest and rather an > > integration test, but it's the best what we can currently do with > WebController. > > If I have time today, I will write a test for this callback. And if I won't I > > will let you know. > > > > > > > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > > > File ios/web/web_state/ui/crw_web_controller.mm (right): > > > > > > > > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > > > ios/web/web_state/ui/crw_web_controller.mm:2566: GURL url = GURL(href); > > > On 2017/05/12 15:04:08, Eugene But wrote: > > > > WebContentsObserver also has DidUpdateFaviconURL. Does content throw away > > > > invalid URLs as well? I think we should do the same thing as other > > platforms, > > > > but also update WebStateObserver::FaviconUrlUpdated comments to explicitly > > say > > > > whether or not the result will contain invalid URLs. > > > > > > There is a check if the URL is empty > > > > > > https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?sq=... > > > So I guess there might be a check to see if it is valid? > > Thank you for checking that. URL can be non-empty, but still invalid. For > > example I think customscheme://foo will not be a valid URL, but it's certainly > > not empty. Would it be better to check for empty url in WebController and > check > > for invalid URL in the embedder? WDYT? > > Thanks for writing the tests! > I checked on a mac version, > https://cs.chromium.org/chromium/src/components/favicon/core/favicon_handler.... > contains the invalid url. I think in this case the invalid icon is a touch icon > so the desktop version might not try to download it. > So I think they might hit the same bug even if I did not find any crash report. > Looking at the code, I my opinion it would make more sense if the "candidate > urls" are valid, so the fix should also be done in content. However I don't have > a strong opinion on this. Agree. If you fix this for //ios/web can you also fix this for //content then?
On 2017/05/15 14:42:53, Eugene But wrote: > On 2017/05/15 12:30:22, gambard wrote: > > On 2017/05/12 17:10:07, Eugene But wrote: > > > On 2017/05/12 15:43:39, gambard wrote: > > > > I tried to add tests but the function is way too buried inside the > class... > > > Look at CRWWebControllerTitleTest.TitleChange for inspiration. You can load > > html > > > with favicon links and then wait until FaviconUrlUpdated callback gets > called > > > (like with TitleWasSet). This will not be exactly a unittest and rather an > > > integration test, but it's the best what we can currently do with > > WebController. > > > If I have time today, I will write a test for this callback. And if I won't > I > > > will let you know. > > > > > > > > > > > > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > > > > File ios/web/web_state/ui/crw_web_controller.mm (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > > > > ios/web/web_state/ui/crw_web_controller.mm:2566: GURL url = GURL(href); > > > > On 2017/05/12 15:04:08, Eugene But wrote: > > > > > WebContentsObserver also has DidUpdateFaviconURL. Does content throw > away > > > > > invalid URLs as well? I think we should do the same thing as other > > > platforms, > > > > > but also update WebStateObserver::FaviconUrlUpdated comments to > explicitly > > > say > > > > > whether or not the result will contain invalid URLs. > > > > > > > > There is a check if the URL is empty > > > > > > > > > > https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?sq=... > > > > So I guess there might be a check to see if it is valid? > > > Thank you for checking that. URL can be non-empty, but still invalid. For > > > example I think customscheme://foo will not be a valid URL, but it's > certainly > > > not empty. Would it be better to check for empty url in WebController and > > check > > > for invalid URL in the embedder? WDYT? > > > > Thanks for writing the tests! > > I checked on a mac version, > > > https://cs.chromium.org/chromium/src/components/favicon/core/favicon_handler.... > > contains the invalid url. I think in this case the invalid icon is a touch > icon > > so the desktop version might not try to download it. > > So I think they might hit the same bug even if I did not find any crash > report. > > Looking at the code, I my opinion it would make more sense if the "candidate > > urls" are valid, so the fix should also be done in content. However I don't > have > > a strong opinion on this. > Agree. If you fix this for //ios/web can you also fix this for //content then? Ok, thanks. I will see how to do this in content.
On 2017/05/15 14:58:53, gambard wrote: > On 2017/05/15 14:42:53, Eugene But wrote: > > On 2017/05/15 12:30:22, gambard wrote: > > > On 2017/05/12 17:10:07, Eugene But wrote: > > > > On 2017/05/12 15:43:39, gambard wrote: > > > > > I tried to add tests but the function is way too buried inside the > > class... > > > > Look at CRWWebControllerTitleTest.TitleChange for inspiration. You can > load > > > html > > > > with favicon links and then wait until FaviconUrlUpdated callback gets > > called > > > > (like with TitleWasSet). This will not be exactly a unittest and rather an > > > > integration test, but it's the best what we can currently do with > > > WebController. > > > > If I have time today, I will write a test for this callback. And if I > won't > > I > > > > will let you know. > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > > > > > File ios/web/web_state/ui/crw_web_controller.mm (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880723002/diff/1/ios/web/web_state/ui/crw_we... > > > > > ios/web/web_state/ui/crw_web_controller.mm:2566: GURL url = GURL(href); > > > > > On 2017/05/12 15:04:08, Eugene But wrote: > > > > > > WebContentsObserver also has DidUpdateFaviconURL. Does content throw > > away > > > > > > invalid URLs as well? I think we should do the same thing as other > > > > platforms, > > > > > > but also update WebStateObserver::FaviconUrlUpdated comments to > > explicitly > > > > say > > > > > > whether or not the result will contain invalid URLs. > > > > > > > > > > There is a check if the URL is empty > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?sq=... > > > > > So I guess there might be a check to see if it is valid? > > > > Thank you for checking that. URL can be non-empty, but still invalid. For > > > > example I think customscheme://foo will not be a valid URL, but it's > > certainly > > > > not empty. Would it be better to check for empty url in WebController and > > > check > > > > for invalid URL in the embedder? WDYT? > > > > > > Thanks for writing the tests! > > > I checked on a mac version, > > > > > > https://cs.chromium.org/chromium/src/components/favicon/core/favicon_handler.... > > > contains the invalid url. I think in this case the invalid icon is a touch > > icon > > > so the desktop version might not try to download it. > > > So I think they might hit the same bug even if I did not find any crash > > report. > > > Looking at the code, I my opinion it would make more sense if the "candidate > > > urls" are valid, so the fix should also be done in content. However I don't > > have > > > a strong opinion on this. > > Agree. If you fix this for //ios/web can you also fix this for //content then? > > Ok, thanks. I will see how to do this in content. Cool. I think it's ok to make fixes in separate CLs to make cherrypicking easier. But this CL needs changes as it will break recently landed tests.
Looking at the javascript code, I think we should move the code adding the default favicon url in the CRWWebController or at least add another |if(empty) then addDefaultURL()|. WDYT?
On 2017/05/16 08:28:10, gambard wrote: > Looking at the javascript code, I think we should move the code adding the > default favicon url in the CRWWebController or at least add another |if(empty) > then addDefaultURL()|. > WDYT? Adding default favicon url in the CRWWebController (not in JS) sounds like an improvement.
On 2017/05/16 15:35:58, Eugene But wrote: > On 2017/05/16 08:28:10, gambard wrote: > > Looking at the javascript code, I think we should move the code adding the > > default favicon url in the CRWWebController or at least add another |if(empty) > > then addDefaultURL()|. > > WDYT? > Adding default favicon url in the CRWWebController (not in JS) sounds like an > improvement. Ok, so the idea would be to remove it from the JS __gCrWeb.common.getFavicons and add it only in the CRWWebController. sgtm
Thanks, PTAL.
Generally LG, just have minor comments. https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/favic... File ios/web/web_state/favicon_callbacks_inttest.mm (right): https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/favic... ios/web/web_state/favicon_callbacks_inttest.mm:168: GURL("https://chromium.test")); Should this be "https://chromium.test/test/test.html" ? https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2574: if (!hasFavicon) { Do you need this variable? Do you want to use |if (urls.empty())| instead and then remove |if (!urls.empty())| check? https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2577: NSURL* originURL = base::mac::ObjCCastStrict<NSURL>(origin); Could you please convert this to GURL and use GURL API instead. In Chromium we avoid using NSURL unless it should be passed to SDK methods. https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2577: NSURL* originURL = base::mac::ObjCCastStrict<NSURL>(origin); Do you want to move comments from JS which explained this code?
Thanks, PTAL. https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/favic... File ios/web/web_state/favicon_callbacks_inttest.mm (right): https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/favic... ios/web/web_state/favicon_callbacks_inttest.mm:168: GURL("https://chromium.test")); On 2017/05/17 14:18:18, Eugene But wrote: > Should this be "https://chromium.test/test/test.html" ? The idea to put https://chromium.test/test/test.html for the other one is to test that the CRWWebController strips the path. This test is also to make sure it works with no path. https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2574: if (!hasFavicon) { On 2017/05/17 14:18:19, Eugene But wrote: > Do you need this variable? Do you want to use |if (urls.empty())| instead and > then remove |if (!urls.empty())| check? The javascript code added the default favicon only if it was non apple touch. I did the same behavior here (i.e. the urls might not be empty). https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2577: NSURL* originURL = base::mac::ObjCCastStrict<NSURL>(origin); On 2017/05/17 14:18:19, Eugene But wrote: > Do you want to move comments from JS which explained this code? Done. https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2577: NSURL* originURL = base::mac::ObjCCastStrict<NSURL>(origin); On 2017/05/17 14:18:19, Eugene But wrote: > Could you please convert this to GURL and use GURL API instead. In Chromium we > avoid using NSURL unless it should be passed to SDK methods. Done.
lgtm https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2574: if (!hasFavicon) { On 2017/05/17 16:05:37, gambard wrote: > On 2017/05/17 14:18:19, Eugene But wrote: > > Do you need this variable? Do you want to use |if (urls.empty())| instead and > > then remove |if (!urls.empty())| check? > > The javascript code added the default favicon only if it was non apple touch. I > did the same behavior here (i.e. the urls might not be empty). Thanks for explanation. Could you please specify that in the comments.
Thanks! https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2880723002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2574: if (!hasFavicon) { On 2017/05/17 18:30:18, Eugene But wrote: > On 2017/05/17 16:05:37, gambard wrote: > > On 2017/05/17 14:18:19, Eugene But wrote: > > > Do you need this variable? Do you want to use |if (urls.empty())| instead > and > > > then remove |if (!urls.empty())| check? > > > > The javascript code added the default favicon only if it was non apple touch. > I > > did the same behavior here (i.e. the urls might not be empty). > Thanks for explanation. Could you please specify that in the comments. Done.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2880723002/#ps60001 (title: "Change the comment")
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": 60001, "attempt_start_ts": 1495092060854330,
"parent_rev": "8d40bcabdcd220045cf57a0a2754298bbb818fc4", "commit_rev":
"2a03a5de9275d45001da304069313dc7810a63ec"}
Message was sent while issue was closed.
Description was changed from ========== Favicon url is used only if it is valid The javascript extracting the favicon urls from a page returns valid and invalid urls. The CRWWebController should only return the valid urls, as returning invalid url leads to a crash. BUG=720666 ========== to ========== Favicon url is used only if it is valid The javascript extracting the favicon urls from a page returns valid and invalid urls. The CRWWebController should only return the valid urls, as returning invalid url leads to a crash. BUG=720666 Review-Url: https://codereview.chromium.org/2880723002 Cr-Commit-Position: refs/heads/master@{#472722} Committed: https://chromium.googlesource.com/chromium/src/+/2a03a5de9275d45001da30406931... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2a03a5de9275d45001da30406931... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
