Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9491009:
Show queued geolocation InfoBars when InfoBar is hidden. (Closed)
|
Created:
8 years, 9 months ago by John Knottenbelt Modified:
8 years, 9 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionShow queued geolocation InfoBars when InfoBar is hidden.
This change should fix a crash in GeolocationBrowserTest.TabDestroyed
browser test.
Instead of showing queued info bars when the
GeolocationConfirmInfobarDelegate is destroyed, show it in response to the
INFOBAR_REMOVED notification. This will enable InfoBarTabHelper to
successfully close all infobars. Consequently, we don't need to listen on
WEB_CONTENTS_DESTROYED notification.
BUG=70588
TEST=GeolocationBrowserTest.TabDestroyed
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128467
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129186
Patch Set 1 #
Total comments: 14
Patch Set 2 : Requested changes. #
Total comments: 13
Patch Set 3 : Requested changes. #Patch Set 4 : Fix the unit tests. #Patch Set 5 : Prevent double notification. #Patch Set 6 : Refactor to make logic clearer. #
Total comments: 10
Patch Set 7 : Marcus requested changes. #
Total comments: 14
Patch Set 8 : Requested changes. #Patch Set 9 : Fix TabDestroyed memory management. #Patch Set 10 : Fix memory leak in TabDestroyed unit test, suppress gtk memory leak. #
Total comments: 4
Patch Set 11 : Requested changes. #
Messages
Total messages: 33 (0 generated)
I'm following Peter's suggested approach in http://code.google.com/p/chromium/issues/detail?id=70588 Please take a look, comments and suggestions very welcome!
thanks john! a few comments below: https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (left): https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:467: registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED, as we chatted, I think we also need this one.. this is owned by GeolocationPermissionContext, which is owned by the Profile which outlives the web contents... should it be destroyed by any reason, we probably want to clean up any pending infobars from it too.. just to be clear, I'm advocating just for keeping this side, in addition to the right hand side of this patch. if you do agree with this, please also update the patch description :) https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:502: content::Source<WebContents>(tab_contents)); as above, we may need to keep this too.. https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:491: InfoBarTabHelper *helper = wrapper->infobar_tab_helper(); nit: s/r *helper/r* helper/
https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (left): https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:467: registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED, On 2012/02/28 17:20:34, bulach wrote: > as we chatted, I think we also need this one.. > this is owned by GeolocationPermissionContext, which is owned by the Profile > which outlives the web contents... should it be destroyed by any reason, we > probably want to clean up any pending infobars from it too.. No, we don't want to keep this. If the web contents is destroyed the infobars on it should all be removed beforehand. That's what the InfoBarTabHelper does in its destructor. Keeping this would imply that somehow a WebContents can reach the shutdown-notification stage with open infobars, which is wrong. https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:465: int type, const content::NotificationSource& source, Nit: While here, one arg per line https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:467: registrar_.Remove(this, chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED, This code will be reached when any infobar is removed, not just when the one you want is removed! You need to ensure that the infobar being removed is actually the one you expect. https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:494: if (!registrar_.IsRegistered( If infobars are disabled, you'll add a listener for a notification that will not fire, because the infobar will never have been added. To solve this, I suggest checking that after AddInfobar(), the helper has added an infobar and it's the one you created; then only register if this is the case. You'll need this infobar pointer anyway to address my comment in Observe() above. I also don't see why IsRegistered() should ever return true here. If it does that means you're adding a second infobar to the tab, which is wrong. So I would instead DCHECK() that IsRegistered() is false.
http://codereview.chromium.org/9491009/diff/1/chrome/browser/geolocation/chro... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): http://codereview.chromium.org/9491009/diff/1/chrome/browser/geolocation/chro... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:25: #include "chrome/common/chrome_notification_types.h" nit: sort, should be before extension.h
clarifying peter's comments below, please let me know your thoughts: https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (left): https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:467: registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED, On 2012/02/28 19:34:59, Peter Kasting wrote: > On 2012/02/28 17:20:34, bulach wrote: > > as we chatted, I think we also need this one.. > > this is owned by GeolocationPermissionContext, which is owned by the Profile > > which outlives the web contents... should it be destroyed by any reason, we > > probably want to clean up any pending infobars from it too.. > > No, we don't want to keep this. > > If the web contents is destroyed the infobars on it should all be removed > beforehand. That's what the InfoBarTabHelper does in its destructor. Keeping > this would imply that somehow a WebContents can reach the shutdown-notification > stage with open infobars, which is wrong. thanks for the details, and sorry I wasn't clear. let me try to explain my thoughts, let me know if they make any sense: this container here may have pending requests that haven't materialized as infobars yet: suppose a page with N frames requesting access to geolocation. initially, it'll have 1 actual infobar, and N-1 requests.. should the WebContents go away, the infobar will be dismissed as you mentioned, but this pending requests will not. I guess the previous code here would take care of this case, no?
https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (left): https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:467: registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED, On 2012/02/29 11:29:29, bulach wrote: > On 2012/02/28 19:34:59, Peter Kasting wrote: > > On 2012/02/28 17:20:34, bulach wrote: > > > as we chatted, I think we also need this one.. > > > > No, we don't want to keep this. > > this container here may have pending requests that haven't materialized as > infobars yet: suppose a page with N frames requesting access to geolocation. > initially, it'll have 1 actual infobar, and N-1 requests.. should the > WebContents go away, the infobar will be dismissed as you mentioned, but this > pending requests will not. The pending requests should be cleared as well, because before the WebContents goes away, it will first tell the InfoBarTabHelper to shut down. This will in turn tell the visible infobars to dismiss. Dismissing the visible geolocation infobar will pump the queue immediately via the INFOBAR_REMOVED notification calling ShowQueuedInfoBar. That will add a new infobar. Then that infobar will be removed by the InfoBarTabHelper, causing the cycle to repeat. The user will never actually see any of these newly-added infobars because none of them will be animated above 0 height before they're removed.
https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:25: #include "chrome/common/chrome_notification_types.h" On 2012/02/28 23:20:58, tfarina wrote: > nit: sort, should be before extension.h Done. https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:465: int type, const content::NotificationSource& source, On 2012/02/28 19:34:59, Peter Kasting wrote: > Nit: While here, one arg per line Done. https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:491: InfoBarTabHelper *helper = wrapper->infobar_tab_helper(); On 2012/02/28 17:20:34, bulach wrote: > nit: s/r *helper/r* helper/ Done.
https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (left): https://chromiumcodereview.appspot.com/9491009/diff/1/chrome/browser/geolocat... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:467: registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED, On 2012/02/29 20:47:54, Peter Kasting wrote: > On 2012/02/29 11:29:29, bulach wrote: > > On 2012/02/28 19:34:59, Peter Kasting wrote: > > > On 2012/02/28 17:20:34, bulach wrote: > > > > as we chatted, I think we also need this one.. > > > > > > No, we don't want to keep this. > > > > this container here may have pending requests that haven't materialized as > > infobars yet: suppose a page with N frames requesting access to geolocation. > > initially, it'll have 1 actual infobar, and N-1 requests.. should the > > WebContents go away, the infobar will be dismissed as you mentioned, but this > > pending requests will not. > > The pending requests should be cleared as well, because before the WebContents > goes away, it will first tell the InfoBarTabHelper to shut down. This will in > turn tell the visible infobars to dismiss. Dismissing the visible geolocation > infobar will pump the queue immediately via the INFOBAR_REMOVED notification > calling ShowQueuedInfoBar. That will add a new infobar. Then that infobar will > be removed by the InfoBarTabHelper, causing the cycle to repeat. > > The user will never actually see any of these newly-added infobars because none > of them will be animated above 0 height before they're removed. got it, thanks for the clarification.
Patch set 2 makes the suggested changes.
https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:93: // content::NotificationObserver (NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED) nit: we usually don't specify in which notification we're interested here. This line of comment is just to give a hint to the reader from where this Observe function was overridden from. https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:484: InfoBarTabHelper* helper = wrapper->infobar_tab_helper(); nit: Move this into the if below and use it in line 497 as well?
LGTM https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:447: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Nit: I don't think this DCHECK is all that important -- a ton of stuff across all Chrome would break badly if these notifications got sent on a non-UI thread. https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:448: DCHECK(type == chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED); Nit: Can be DCHECK_EQ(expected, actual) https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:450: // confirm info bar. We need to check that the infobar delegate Nit: Nix "confirm". Actually, "We will receive this notification for all infobar closures, so we need to check whether this is the geolocation infobar we're tracking" might be clearer? https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:452: InfoBarRemovedDetails* removed_details = Nit: Can inline this into the next line
Thanks for all your help guys! https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:93: // content::NotificationObserver (NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED) On 2012/03/01 16:14:23, tfarina wrote: > nit: we usually don't specify in which notification we're interested here. > This line of comment is just to give a hint to the reader from where this > Observe function was overridden from. Done. https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:447: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/03/01 19:46:54, Peter Kasting wrote: > Nit: I don't think this DCHECK is all that important -- a ton of stuff across > all Chrome would break badly if these notifications got sent on a non-UI thread. Done. https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:447: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/03/01 19:46:54, Peter Kasting wrote: > Nit: I don't think this DCHECK is all that important -- a ton of stuff across > all Chrome would break badly if these notifications got sent on a non-UI thread. Done. https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:448: DCHECK(type == chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED); On 2012/03/01 19:46:54, Peter Kasting wrote: > Nit: Can be DCHECK_EQ(expected, actual) Done. https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:450: // confirm info bar. We need to check that the infobar delegate On 2012/03/01 19:46:54, Peter Kasting wrote: > Nit: Nix "confirm". Actually, "We will receive this notification for all > infobar closures, so we need to check whether this is the geolocation infobar > we're tracking" might be clearer? Done. https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:452: InfoBarRemovedDetails* removed_details = On 2012/03/01 19:46:54, Peter Kasting wrote: > Nit: Can inline this into the next line Done. https://chromiumcodereview.appspot.com/9491009/diff/8001/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:484: InfoBarTabHelper* helper = wrapper->infobar_tab_helper(); On 2012/03/01 16:14:23, tfarina wrote: > nit: Move this into the if below and use it in line 497 as well? Done.
Looks like the change broke some Geolocation unit tests. Will investigate.
In this latest change, I have refactored things a little to make it easier to understand. I have changed ShowQueuedInfoBar so that it doesn't cause any changes to pending_infobar_requests_ and rejigged CancelInfoBarRequestInternal so that it always returns the next infobar.
hi john, a few general comments, I'll look into more details tomorrow: https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:115: void ClearInfoBarRequestsFromTab(int render_process_id, int render_view_id); nit: maybe ClearPendingInfoBarRequestsFromTab, to match with the member? https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:118: bool AlreadyShowingQueuedInfoBar(int render_process_id, int render_view_id); nit: and in this case the could probably remove the "Queued", and perhaps IsShowingInfoBar? https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:289: bool notified; unused? https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:393: if (!helper) { maybe move 388 after this and add a comment saying something along: // We won't be able to create any infobars, shortcut and remove any pending requests.. https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:478: CancelInfoBarRequestInternal(i, false); see below, I think it'd be nicer to ShowQueuedInfoBar from here instead.. https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:565: ShowQueuedInfoBar(render_process_id, render_view_id, helper); hmm.. I'm not sure we want to create the infobar here, feels strange that a "Cancel" method would end up creating a new one...
Thanks for review Marcus, I've made the requested changes. I actually removed CancelInfoBarRequestInternal altogether now, because OnPermissionSet has to do things in a sufficiently careful way to make it not useful for it to call, leaving the only other caller CancelInfoBarRequest, so may as well put the code in here. This change puts the ShowQueuedInfoBar back into the Observe method where it should be. The other nice thing about this is that if there is a PendingInfoBarRequest in the list that has a delegate, then we know that the helper must be alive too, because if it wasn't the Observe() method would have been called and we would have removed it. Nice! https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:115: void ClearInfoBarRequestsFromTab(int render_process_id, int render_view_id); On 2012/03/22 17:01:46, bulach wrote: > nit: maybe ClearPendingInfoBarRequestsFromTab, to match with the member? Done. https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:118: bool AlreadyShowingQueuedInfoBar(int render_process_id, int render_view_id); On 2012/03/22 17:01:46, bulach wrote: > nit: and in this case the could probably remove the "Queued", and perhaps > IsShowingInfoBar? Done. https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:289: bool notified; Removed. On 2012/03/22 17:01:46, bulach wrote: > unused? https://chromiumcodereview.appspot.com/9491009/diff/27001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:393: if (!helper) { On 2012/03/22 17:01:46, bulach wrote: > maybe move 388 after this and add a comment saying something along: > // We won't be able to create any infobars, shortcut and remove any pending > requests.. Done.
lgtm I like this flow, thanks john! a few more suggestions to help clarify it: http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:109: // Removes any pending requests for a given dead tab. nit: I'd just say "for a given tab", this method doesn't care about the state.. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:110: void ClearPendingInfoBarRequestsFromTab(int render_process_id, nit: maybe s/From/For/ ? http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:438: PendingInfoBarRequests requests_to_notify, infobars_to_remove; nit: one per line.. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:449: // may also cause other problems during stack unwinding. nit: not sure if this would clarify, but how about: // The delegate that called us is i, and it's currently in either Accept() // or Cancel(). // This means that the owning InfoBar will call RemoveInfoBar() later on, // and that will trigger a notification we're observing. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:453: // loop because doing so modifies pending_infobar_requests_. and here: // This InfoBar is for the same frame/embedder pair, but in a different tab. // We should remove it now that we've got an answer for it. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:456: } else { // We haven't created an InfoBar yet, just remove the pending request. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:464: // Remove any showing InfoBars. // Remove all InfoBars for the same |requesting_frame| and |embedder|.
Thanks again. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... File chrome/browser/geolocation/chrome_geolocation_permission_context.cc (right): http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:109: // Removes any pending requests for a given dead tab. On 2012/03/23 11:01:46, bulach wrote: > nit: I'd just say "for a given tab", this method doesn't care about the state.. Done. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:110: void ClearPendingInfoBarRequestsFromTab(int render_process_id, On 2012/03/23 11:01:46, bulach wrote: > nit: maybe s/From/For/ ? Done. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:438: PendingInfoBarRequests requests_to_notify, infobars_to_remove; On 2012/03/23 11:01:46, bulach wrote: > nit: one per line.. Done. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:449: // may also cause other problems during stack unwinding. On 2012/03/23 11:01:46, bulach wrote: > nit: not sure if this would clarify, but how about: > // The delegate that called us is i, and it's currently in either Accept() > // or Cancel(). > // This means that the owning InfoBar will call RemoveInfoBar() later on, > // and that will trigger a notification we're observing. Done. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:453: // loop because doing so modifies pending_infobar_requests_. On 2012/03/23 11:01:46, bulach wrote: > and here: > // This InfoBar is for the same frame/embedder pair, but in a different tab. > // We should remove it now that we've got an answer for it. Done. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:456: } else { On 2012/03/23 11:01:46, bulach wrote: > // We haven't created an InfoBar yet, just remove the pending request. Done. http://chromiumcodereview.appspot.com/9491009/diff/32002/chrome/browser/geolo... chrome/browser/geolocation/chrome_geolocation_permission_context.cc:464: // Remove any showing InfoBars. On 2012/03/23 11:01:46, bulach wrote: > // Remove all InfoBars for the same |requesting_frame| and |embedder|. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jknotten@chromium.org/9491009/35001
Change committed as 128467
Looks like this has caused a leak: operator new(unsigned long) (m_replacemalloc/vg_replace_malloc.c:1074) GeolocationInfoBarQueueController::ShowQueuedInfoBar(int, int, InfoBarTabHelper*) (chrome/browser/geolocation/chrome_geolocation_permission_context.cc:564) GeolocationInfoBarQueueController::Observe(int, content::NotificationSource const&, content::NotificationDetails const&) (chrome/browser/geolocation/chrome_geolocation_permission_context.cc:502) NotificationServiceImpl::Notify(int, content::NotificationSource const&, content::NotificationDetails const&) (content/browser/notification_service_impl.cc:125) InfoBarTabHelper::RemoveInfoBarInternal(InfoBarDelegate*, bool) (chrome/browser/infobars/infobar_tab_helper.cc:121) InfoBarTabHelper::RemoveAllInfoBars(bool) (chrome/browser/infobars/infobar_tab_helper.cc:135) InfoBarTabHelper::~InfoBarTabHelper() (chrome/browser/infobars/infobar_tab_helper.cc:32) scoped_ptr<InfoBarTabHelper>::reset(InfoBarTabHelper*) (./base/memory/scoped_ptr.h:160) TabContentsWrapper::~TabContentsWrapper() (chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:160) scoped_ptr<TabContentsWrapper>::reset(TabContentsWrapper*) (./base/memory/scoped_ptr.h:160) TabContentsWrapperTestHarness::SetContents(content::WebContents*) (chrome/browser/ui/tab_contents/test_tab_contents_wrapper.cc:29) content::RenderViewHostTestHarness::DeleteContents() (content/test/test_renderer_host.cc:161) GeolocationPermissionContextTests_TabDestroyed_Test::TestBody() (chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc:567) http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v... I'll revert this patch for now.
Am I right in thinking that the InfoBarDelegate will be destroyed naturally in InfoBar::MaybeDelete() called from InfoBar::Hide() called from InfoBarContainer::HideInfoBar called from InfoBarContainer::Observe in response to NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED ? But InfoBarContainer looks platform specific, and I don't see a Testing version, so I think we're running without an InfoBarContainer. Looking at InfoBarHelper::~InfoBarHelper I see this TODO: // TODO(pkasting): If there is no InfoBarContainer, this leaks all the // InfoBarDelegates. This will be fixed once we call CloseSoon() directly on // Infobars. Can we fix up the GeolocationPermissionContextTests by creating our own InfoBarContainer ? We would then need to remove all the manual calls to InfoBarDelegate::InfoBarClosed(); On 2012/03/23 13:59:12, John Knottenbelt wrote: > Looks like this has caused a leak: > > operator new(unsigned long) (m_replacemalloc/vg_replace_malloc.c:1074) > GeolocationInfoBarQueueController::ShowQueuedInfoBar(int, int, > InfoBarTabHelper*) > (chrome/browser/geolocation/chrome_geolocation_permission_context.cc:564) > GeolocationInfoBarQueueController::Observe(int, content::NotificationSource > const&, content::NotificationDetails const&) > (chrome/browser/geolocation/chrome_geolocation_permission_context.cc:502) > NotificationServiceImpl::Notify(int, content::NotificationSource const&, > content::NotificationDetails const&) > (content/browser/notification_service_impl.cc:125) > InfoBarTabHelper::RemoveInfoBarInternal(InfoBarDelegate*, bool) > (chrome/browser/infobars/infobar_tab_helper.cc:121) > InfoBarTabHelper::RemoveAllInfoBars(bool) > (chrome/browser/infobars/infobar_tab_helper.cc:135) > InfoBarTabHelper::~InfoBarTabHelper() > (chrome/browser/infobars/infobar_tab_helper.cc:32) > scoped_ptr<InfoBarTabHelper>::reset(InfoBarTabHelper*) > (./base/memory/scoped_ptr.h:160) > TabContentsWrapper::~TabContentsWrapper() > (chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:160) > scoped_ptr<TabContentsWrapper>::reset(TabContentsWrapper*) > (./base/memory/scoped_ptr.h:160) > TabContentsWrapperTestHarness::SetContents(content::WebContents*) > (chrome/browser/ui/tab_contents/test_tab_contents_wrapper.cc:29) > content::RenderViewHostTestHarness::DeleteContents() > (content/test/test_renderer_host.cc:161) > GeolocationPermissionContextTests_TabDestroyed_Test::TestBody() > (chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc:567) > > http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%2520Tests%2520... > > I'll revert this patch for now.
On 2012/03/23 17:31:21, John Knottenbelt wrote: > Can we fix up the GeolocationPermissionContextTests by creating our own > InfoBarContainer ? We would then need to remove all the manual calls to > InfoBarDelegate::InfoBarClosed(); Don't do this. Check your patch back in as-is and update the leak suppressions. I thought there was already a suppression present for the geolocation infobar controller as it used to do things. Look around for suppressions relating to infobars and if you don't find one that just needs updating, then create a new one.
Sorry, I just read this message after uploading my latest patch. I didn't create an InfoBarContainer, but changed the test to remove the left over InfoBarDelegate from the TabDestroyed unit test. Please could you take a look and let me know if you still prefer patch 8 + suppression? On 2012/03/23 18:17:20, Peter Kasting wrote: > On 2012/03/23 17:31:21, John Knottenbelt wrote: > > Can we fix up the GeolocationPermissionContextTests by creating our own > > InfoBarContainer ? We would then need to remove all the manual calls to > > InfoBarDelegate::InfoBarClosed(); > > Don't do this. Check your patch back in as-is and update the leak suppressions. > I thought there was already a suppression present for the geolocation infobar > controller as it used to do things. Look around for suppressions relating to > infobars and if you don't find one that just needs updating, then create a new > one.
On 2012/03/23 18:44:44, John Knottenbelt wrote: > Sorry, I just read this message after uploading my latest patch. I didn't create > an InfoBarContainer, but changed the test to remove the left over > InfoBarDelegate from the TabDestroyed unit test. > > Please could you take a look and let me know if you still prefer patch 8 + > suppression? Explicitly removing is OK, but all the other changes you made (in other words, everything relating to Delete()) will just make it harder for me to change this test over once I land the fix. So if you can do a patch that's exactly like patch 8 except for just deleting the extra infobar on the one test, that'd be great.
This patch is patch 8 + fix for leaking delegate in TabDestroyed test. In running locally on valgrind, I found another memory leak in unrelated ThemeServiceGTK::LoadGtkValues() for which I added a suppression for this and filed bug http://code.google.com/p/chromium/issues/detail?id=120118
LGTM https://chromiumcodereview.appspot.com/9491009/diff/36001/chrome/browser/geol... File chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/36001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc:43: FRIEND_TEST_ALL_PREFIXES(GeolocationPermissionContextTests, TabDestroyed); This doesn't go above the public section, it goes in the private section. https://chromiumcodereview.appspot.com/9491009/diff/36001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc:607: } // namespace Don't extend the namespace to cover the unit tests, or we can't friend them from any other source files. If you need the helper class in the same namespace then remove both from the namespace.
https://chromiumcodereview.appspot.com/9491009/diff/36001/chrome/browser/geol... File chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc (right): https://chromiumcodereview.appspot.com/9491009/diff/36001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc:43: FRIEND_TEST_ALL_PREFIXES(GeolocationPermissionContextTests, TabDestroyed); On 2012/03/26 19:12:22, Peter Kasting wrote: > This doesn't go above the public section, it goes in the private section. Done. https://chromiumcodereview.appspot.com/9491009/diff/36001/chrome/browser/geol... chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc:607: } // namespace I removed the namespace. The issue is that the FRIEND_TEST macro looks up the class in the anonymous namespace and there is no documented way to forward declare the test class outside of the namespace. I could do: class GeolocationPermissionContextTests_TabDestroyed_Test; or class GTEST_TEST_CLASS_NAME_(GeolocationPermissionContextTests, TabDestroyed); but I think this is relies too much on the internal implementation of gtest. On 2012/03/26 19:12:22, Peter Kasting wrote: > Don't extend the namespace to cover the unit tests, or we can't friend them from > any other source files. If you need the helper class in the same namespace then > remove both from the namespace.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jknotten@chromium.org/9491009/41001
Try job failure for 9491009-41001 (retry) on win_rel for steps "ui_tests, browser_tests". It's a second try, previously, steps "ui_tests, browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jknotten@chromium.org/9491009/41001
Change committed as 129186 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
