Memory leak of LogoObserver when DSE has no logo |
|||
Issue descriptionLogoObserverAndroid does memory management by deleting itself in OnObserverRemoved(). However, there is no guarantee that an observer passed to LogoService::GetLogo() will be added, and and therefore no guarantee it will be removed. In particular, if there is no TemplateURLService or DSE (unlikely), or if the user is using a DSE with no logo (e.g. Yahoo!; reasonably likely), then LogoService::GetLogo() will return early, the observer will never be given to the LogoTracker, and it will never delete itself. Although iOS's implementations of both the service (until https://crrev.com/c/605207) and observer (in internal code) are different, the same bug exists, mutatis mutandis.
,
Aug 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7dd0374286ae7e4199d1b87c7a20875db1c84200 commit 7dd0374286ae7e4199d1b87c7a20875db1c84200 Author: Chris Pickel <sfiera@chromium.org> Date: Wed Aug 09 14:25:03 2017 Fix leak of LogoObserver LogoObserverAndroid does memory management by deleting itself in OnObserverRemoved(). However, there was no guarantee that an observer passed to LogoService::GetLogo() would be added, and and therefore no guarantee it would be removed. In particular, if there was no TemplateURLService or DSE (unlikely), or if the user was using a DSE with no logo (e.g. Yahoo!; reasonably likely), then LogoService::GetLogo() would return early, the observer would never be given to the LogoTracker, and it would never delete itself. It's still the case that the caller of GetLogo() will never get a response in this case. I think it would be preferable to get a "no logo" response. BUG= 753784 Change-Id: I1b0510da4d53bc15beadc550ce5dc73d9f0d5c1f Reviewed-on: https://chromium-review.googlesource.com/608232 Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Chris Pickel <sfiera@chromium.org> Cr-Commit-Position: refs/heads/master@{#492967} [modify] https://crrev.com/7dd0374286ae7e4199d1b87c7a20875db1c84200/components/search_provider_logos/logo_service.cc
,
Aug 11 2017
,
Aug 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c3a10125a20e7ae88ba2385128d274902b7323a5 commit c3a10125a20e7ae88ba2385128d274902b7323a5 Author: Chris Pickel <sfiera@chromium.org> Date: Thu Aug 24 11:46:24 2017 Share LogoService implementation with iOS iOS inherits LogoService::GetLogo(), but still implements its own methods for caching logos. In order to avoid launching non-Google DSE logos on iOS without launch review, add an #ifdef around the code that launches it on Android. BUG=583290, 753784 Change-Id: I91e5b7da8641418d0e0ca59bd552b6b59fbfae40 Reviewed-on: https://chromium-review.googlesource.com/605207 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Chris Pickel <sfiera@chromium.org> Cr-Commit-Position: refs/heads/master@{#497008} [modify] https://crrev.com/c3a10125a20e7ae88ba2385128d274902b7323a5/components/search_provider_logos/logo_service.cc [modify] https://crrev.com/c3a10125a20e7ae88ba2385128d274902b7323a5/ios/chrome/browser/google/BUILD.gn [modify] https://crrev.com/c3a10125a20e7ae88ba2385128d274902b7323a5/ios/chrome/browser/google/google_logo_service.h [modify] https://crrev.com/c3a10125a20e7ae88ba2385128d274902b7323a5/ios/chrome/browser/google/google_logo_service.mm [modify] https://crrev.com/c3a10125a20e7ae88ba2385128d274902b7323a5/ios/chrome/browser/google/google_logo_service_factory.mm
,
Aug 24 2017
,
Aug 24 2017
Re: the memory leak, I think what we really should is do is replace the OnObserverRemoved() call with explicit ownership, so LogoService::GetLogo() could take a unique_ptr and automatically make sure it will destroy it.
,
Aug 24 2017
I think we want to rethink the LogoService interface more deeply than that. In the current world, the service takes an observer, invokes it 0-2 times, then removes it. That's awkward for at least two of the three platforms that use the service, and I find it just generally kind of weird. |
|||
►
Sign in to add a comment |
|||
Comment 1 by treib@chromium.org
, Aug 9 2017