Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
297293002:
Change HungPluginTabHelper to listen for infobar changes through Observer style. (Closed)
|
Created:
6 years, 7 months ago by tfarina Modified:
6 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, droger Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChange HungPluginTabHelper to listen for infobar changes through Observer style.
Test plan:
1) out/Debug/chrome --no-sandbox --ppapi-flash-path=/opt/google/chrome/PepperFlash/libpepflashplayer.so --ppapi-flash-version=`grep -i version /opt/google/chrome/PepperFlash/manifest.json | awk '{print $2}' | awk -F"\"" '{print $2}'`
2) Go to a site that has flash, e.g., http://www.flash-game.net/
3) Open Task Manager, Shift+Esc
4) Search for "Plug-in: Shockwave Flash" in the list.
5) Select it and click on "End process" button.
6) Go to the website and observe the infobar.
Another way to test it is:
1) Add an infinite loop into ppapi/examples/scripting/post_message.cc
2) Build ppapi_example_post_message
3) out/Debug/chrome --no-sandbox --register-pepper-plugins="/home/tfarina/chromium/src/out/Debug/lib/libppapi_example_post_message.so;application/x-ppapi-post-message-example" ppapi/examples/scripting/post_message.html
BUG=354380
TEST=see above
R=pkasting@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277292
Patch Set 1 #
Total comments: 4
Patch Set 2 : observer installed #
Total comments: 2
Patch Set 3 : review #
Total comments: 2
Patch Set 4 : count? NOT WORKING YET #Patch Set 5 : . #Patch Set 6 : reduce diff #Patch Set 7 : reduce the diff even more #Patch Set 8 : rm infobar_count #Patch Set 9 : number of infobars #
Total comments: 12
Patch Set 10 : review #
Total comments: 8
Patch Set 11 : review fixes #Patch Set 12 : ourself #Patch Set 13 : dtor change #
Messages
Total messages: 31 (0 generated)
Peter, I will need your advice here, in order to have a better fix. I appreciate your patience. David, if you want to take a look too, feel free. fyi. Thanks, https://codereview.chromium.org/297293002/diff/1/chrome/browser/ui/hung_plugi... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/1/chrome/browser/ui/hung_plugi... chrome/browser/ui/hung_plugin_tab_helper.cc:380: infobar_service->AddObserver(this); I can't put this in the ctor, because otherwise, when the user shutdown the browser this happens: [15141:15141:0525/184807:FATAL:observer_list.h(197)] Check failed: ObserverListBase<ObserverType>::size() == 0U (1 vs. 0) #0 0x7f8071eb105e base::debug::StackTrace::StackTrace() #1 0x7f8071f3e465 logging::LogMessage::~LogMessage() #2 0x7f8077339b25 ObserverList<>::~ObserverList() #3 0x7f8077338d35 infobars::InfoBarManager::~InfoBarManager() #4 0x7f80745c70b2 InfoBarService::~InfoBarService() #6 0x7f80745c71fc InfoBarService::~InfoBarService() #7 0x7f8071ffb657 linked_ptr<>::depart() #8 0x7f8071ffac45 linked_ptr<>::~linked_ptr() #9 0x7f8071ffb21c std::pair<>::~pair() #10 0x7f8071ffb1ec std::_Rb_tree_node<>::~_Rb_tree_node() #11 0x7f8071ffb159 __gnu_cxx::new_allocator<>::destroy<>() #12 0x7f8071ffb10c std::_Rb_tree<>::_M_destroy_node() #13 0x7f8071ffb0be std::_Rb_tree<>::_M_erase_aux() #14 0x7f8071ffb03a std::_Rb_tree<>::erase() #15 0x7f8071ffafd5 std::__cxx1998::map<>::erase() #16 0x7f8071ffacff std::__debug::map<>::erase() #17 0x7f8071ffa4c1 base::SupportsUserData::RemoveUserData() #18 0x7f80745c7510 InfoBarService::WebContentsDestroyed() #19 0x7f80745c753c InfoBarService::WebContentsDestroyed() #20 0x7f8065623224 content::WebContentsImpl::~WebContentsImpl() #21 0x7f8065623989 content::WebContentsImpl::~WebContentsImpl() #22 0x7f8076248fd7 TabStripModel::InternalCloseTab() #23 0x7f80762453f8 TabStripModel::InternalCloseTabs() #24 0x7f8076244f17 TabStripModel::CloseAllTabs() https://codereview.chromium.org/297293002/diff/1/chrome/browser/ui/hung_plugi... chrome/browser/ui/hung_plugin_tab_helper.cc:387: void HungPluginTabHelper::CloseBar(PluginState* state) { should I have a call to RemoveObserver() here to balance the call to AddObserver() in ShowBar?
https://codereview.chromium.org/297293002/diff/1/chrome/browser/ui/hung_plugi... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/1/chrome/browser/ui/hung_plugi... chrome/browser/ui/hung_plugin_tab_helper.cc:380: infobar_service->AddObserver(this); You should only be doing this if we're not already an observer. Based on the other code in this file I assume we can be observing for multiple different infobars. https://codereview.chromium.org/297293002/diff/1/chrome/browser/ui/hung_plugi... chrome/browser/ui/hung_plugin_tab_helper.cc:387: void HungPluginTabHelper::CloseBar(PluginState* state) { On 2014/05/25 21:53:30, tfarina wrote: > should I have a call to RemoveObserver() here to balance the call to > AddObserver() in ShowBar? All AddObserver() calls should be balanced with RemoveObserver() calls. I don't think putting such a call here is right unless this is reached when users click the close button on the infobar itself. Otherwise you'll need to do the call e.g. in OnInfoBarRemoved() when we have no more infobars.
Peter, could you take a look at patch set 2? thanks,
https://codereview.chromium.org/297293002/diff/40001/chrome/browser/ui/hung_p... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/40001/chrome/browser/ui/hung_p... chrome/browser/ui/hung_plugin_tab_helper.cc:280: infobar_service->AddObserver(this); Why do we want to add an observer here? We don't seem to be adding any infobars. https://codereview.chromium.org/297293002/diff/40001/chrome/browser/ui/hung_p... chrome/browser/ui/hung_plugin_tab_helper.cc:307: infobar_service->AddObserver(this); Wouldn't we want this in ShowBar() instead of here? Otherwise, can't OnReshowTimer() add a bar that we don't observe? Also, where is the balancing RemoveObserver() call to this? We only seem to have one if the manager is shutting down.
Peter, feedback addressed. ptal.
I feel like you're not thinking very hard about how to do this conversion. You should have already recognized the sorts of objects I've been raising. https://codereview.chromium.org/297293002/diff/60001/chrome/browser/ui/hung_p... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/60001/chrome/browser/ui/hung_p... chrome/browser/ui/hung_plugin_tab_helper.cc:400: infobar_service->RemoveObserver(this); This still doesn't seem right. (1) What if the bar is closed by a different method than CloseBar(), e.g. the user closing the infobar? Then we won't get notified. Seems like this code should instead be in OnInfoBarRemoved(). (2) What if we're observing notificatinos for more than one infobar? Then we don't want to remove our observer yet. Also, is it possible for the class to be destroyed with infobars open? If so, we need to RemoveObserver() in our destructor. If not, the destructor should DCHECK that we're no longer an observer.
On Fri, May 30, 2014 at 3:54 PM, <pkasting@chromium.org> wrote: > I feel like you're not thinking very hard about how to do this conversion. > You > should have already recognized the sorts of objects I've been raising. > > Yes. You are right. Thanks for pushing me again. I will go back to my cave and think more about this. I have figured out how to test this and thus make my life easier. I have described in the CL description how I'm initially testing it. There should be other ways I should also be testing it though. And the by way I'm testing it, line 278 is being hit. -- Thiago Farina To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/297293002/diff/60001/chrome/browser/ui/hung_p... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/60001/chrome/browser/ui/hung_p... chrome/browser/ui/hung_plugin_tab_helper.cc:400: infobar_service->RemoveObserver(this); On 2014/05/30 18:54:47, Peter Kasting wrote: > Also, is it possible for the class to be destroyed with infobars open? Not sure if that answers your question. But the biggest issue (imo) is that InfoBarService is destroyed before HungPluginTabHelper, and possibly before many other TabHelpers. If HPTH was destroyed before, then we could deregister from it in the dtor.
On 2014/05/30 21:14:03, tfarina wrote: > https://codereview.chromium.org/297293002/diff/60001/chrome/browser/ui/hung_p... > File chrome/browser/ui/hung_plugin_tab_helper.cc (right): > > https://codereview.chromium.org/297293002/diff/60001/chrome/browser/ui/hung_p... > chrome/browser/ui/hung_plugin_tab_helper.cc:400: > infobar_service->RemoveObserver(this); > On 2014/05/30 18:54:47, Peter Kasting wrote: > > Also, is it possible for the class to be destroyed with infobars open? > > Not sure if that answers your question. If what answers my question? Your previous comment here an hour ago? No, it does not answer my question. > But the biggest issue (imo) is that > InfoBarService is destroyed before HungPluginTabHelper, and possibly before many > other TabHelpers. Always? So we're guaranteed that we have gotten a manager shutdown message, and thus the destructor can DCHECK that we're not an observer?
I don't think we have a clean way to do this right now (my observer_installed_ was a mess). The two assumptions below would have to be true for this to be a straight conversion: 1) InfoBarService needs to be created before HungPluginTabHelper - OK (so we can call AddObserver() from ctor). 2) InfoBarService needs to be destroyed *AFTER* HungPluginTabHelper - NOT OK (so we could call RemoveObserver() from dtor). NOTE: This would not have been caught if InfoBarManager's |observers_| were not declared with "true". https://chromium.googlesource.com/chromium/chromium/+/trunk/components/infoba...
On 2014/05/30 23:15:19, tfarina wrote: > I don't think we have a clean way to do this right now (my observer_installed_ > was a mess). Don't you simply need to count how many infobars you're observing notifications for?
On 2014/05/30 23:21:40, Peter Kasting wrote: > On 2014/05/30 23:15:19, tfarina wrote: > > I don't think we have a clean way to do this right now (my observer_installed_ > > was a mess). > > Don't you simply need to count how many infobars you're observing notifications > for? There is no good place to unregister it other than the dtor.
On 2014/05/30 23:53:01, tfarina wrote: > On 2014/05/30 23:21:40, Peter Kasting wrote: > > On 2014/05/30 23:15:19, tfarina wrote: > > > I don't think we have a clean way to do this right now (my > observer_installed_ > > > was a mess). > > > > Don't you simply need to count how many infobars you're observing > notifications > > for? > > There is no good place to unregister it other than the dtor. Well, if you're registered, and you haven't gotten a "manager shutting down" notification, then unregistering in the dtor is both safe and necessary. Otherwise, unregistering when removing the last infobar seems like the right way to go. Remember to handle the case when Create() returns a NULL infobar.
On 2014/05/31 00:00:44, Peter Kasting wrote: > On 2014/05/30 23:53:01, tfarina wrote: > > On 2014/05/30 23:21:40, Peter Kasting wrote: > > > On 2014/05/30 23:15:19, tfarina wrote: > > > > I don't think we have a clean way to do this right now (my > > observer_installed_ > > > > was a mess). > > > > > > Don't you simply need to count how many infobars you're observing > > notifications > > > for? > > > > There is no good place to unregister it other than the dtor. > > Well, if you're registered, and you haven't gotten a "manager shutting down" > notification, then unregistering in the dtor is both safe and necessary. > It can't be safe. At this time WebContents and InfoBarService are already destroyed.
On 2014/05/31 00:27:25, tfarina wrote: > On 2014/05/31 00:00:44, Peter Kasting wrote: > > On 2014/05/30 23:53:01, tfarina wrote: > > > On 2014/05/30 23:21:40, Peter Kasting wrote: > > > > On 2014/05/30 23:15:19, tfarina wrote: > > > > > I don't think we have a clean way to do this right now (my > > > observer_installed_ > > > > > was a mess). > > > > > > > > Don't you simply need to count how many infobars you're observing > > > notifications > > > > for? > > > > > > There is no good place to unregister it other than the dtor. > > > > Well, if you're registered, and you haven't gotten a "manager shutting down" > > notification, then unregistering in the dtor is both safe and necessary. > > > It can't be safe. At this time WebContents and InfoBarService are already > destroyed. How was the InfoBarService destroyed without you receiving a "manager is being destroyed" notification? That's the entire point of that notification.
On 2014/05/31 00:28:51, Peter Kasting wrote: > On 2014/05/31 00:27:25, tfarina wrote: > > On 2014/05/31 00:00:44, Peter Kasting wrote: > > > On 2014/05/30 23:53:01, tfarina wrote: > > > > On 2014/05/30 23:21:40, Peter Kasting wrote: > > > > > On 2014/05/30 23:15:19, tfarina wrote: > > > > > > I don't think we have a clean way to do this right now (my > > > > observer_installed_ > > > > > > was a mess). > > > > > > > > > > Don't you simply need to count how many infobars you're observing > > > > notifications > > > > > for? > > > > > > > > There is no good place to unregister it other than the dtor. > > > > > > Well, if you're registered, and you haven't gotten a "manager shutting down" > > > notification, then unregistering in the dtor is both safe and necessary. > > > > > It can't be safe. At this time WebContents and InfoBarService are already > > destroyed. > > How was the InfoBarService destroyed without you receiving a "manager is being > destroyed" notification? That's the entire point of that notification. I put break points in the destructors of InfoBarService, InfoBarManager and HungPluginTabHelper. And from what I could see, InfoBarService dtor is called first, which calls ShutDown(). Then InfoBarManager dtor is called and by the end HungPluginTabHelper dtor is called. Did you expect InfoBarService to receive OnManagerShuttingDown() event? It isn't an InfoBarManager::Observer. Maybe you meant something else?
On 2014/05/31 01:18:27, tfarina wrote: > On 2014/05/31 00:28:51, Peter Kasting wrote: > > On 2014/05/31 00:27:25, tfarina wrote: > > > On 2014/05/31 00:00:44, Peter Kasting wrote: > > > > On 2014/05/30 23:53:01, tfarina wrote: > > > > > On 2014/05/30 23:21:40, Peter Kasting wrote: > > > > > > On 2014/05/30 23:15:19, tfarina wrote: > > > > > > > I don't think we have a clean way to do this right now (my > > > > > observer_installed_ > > > > > > > was a mess). > > > > > > > > > > > > Don't you simply need to count how many infobars you're observing > > > > > notifications > > > > > > for? > > > > > > > > > > There is no good place to unregister it other than the dtor. > > > > > > > > Well, if you're registered, and you haven't gotten a "manager shutting > down" > > > > notification, then unregistering in the dtor is both safe and necessary. > > > > > > > It can't be safe. At this time WebContents and InfoBarService are already > > > destroyed. > > > > How was the InfoBarService destroyed without you receiving a "manager is being > > destroyed" notification? That's the entire point of that notification. > I put break points in the destructors of InfoBarService, InfoBarManager and > HungPluginTabHelper. And from what I could see, InfoBarService dtor is called > first, which calls ShutDown(). Then InfoBarManager dtor is called and by the end > HungPluginTabHelper dtor is called. > > Did you expect InfoBarService to receive OnManagerShuttingDown() event? It isn't > an InfoBarManager::Observer. Maybe you meant something else? HungPluginTabHelper needs to override OnManagerShuttingDown() -- as it did in your early patch sets. Then you should RemoveObserver() in three cases: (1) In OnInfoBarRemoved(), when the last infobar you're managing is removed. This requires keeping a count of the relevant infobars. (2) In OnManagerShuttingDown(), in order to handle all cases where the manager is destroyed before the tab helper. (3) In ~HungPluginTabHelper(), in order to handle all cases where the tab helper is destroyed before the manager. If you are _guaranteed_ that the destruction order in (3) can never happen, then ~HungPluginTabHelper() should instead DCHECK that it's not an observer (by checking whatever variable you use to count how many infobars you're observing for). Is there something non-obvious about this? I feel like this would normally be a piece of cake for you.
Peter, after some difficulties, I think it is ready for another look. I hope I have addressed all your concerns (and tested everything). This time I didn't observe any DCHECKs, both closing the infobar throught the [x] button or closing the browser with the infobar opened. Please, let me know if I'm missing something else or need to debug more. Thanks,
OK, this is better. Not sure why you sent me a screenshot of the infobar though. After reading through this, I think things are even simpler than I described earlier. See comment below. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:266: DCHECK_EQ(0, number_of_infobars_); One of the following is true: (1) This DCHECK doesn't always hold. (2) We don't actually need to observe the "manager shutting down" notification, because we will have gone to zero infobars and removed ourselves as an observer before then. If (2) were not true, then we could get "manager shutting down" while some infobars are still open. But then we wouldn't reset |number_of_infobars_| to zero, so we'd fail this DCHECK. Based on the code in InfoBarManager::ShutDown(), I would think (2) is true, unless somehow ShowBar() can ever be called in response to the manager closing all existing infobars. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:323: number_of_infobars_--; Nit: Prefer predecrement to postdecrement https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:327: InfoBarService::FromWebContents(web_contents()); Nit: Just inline this into the next statement: InfoBarService::FromWebContents(web_contents())->RemoveObserver(this); This will allow removing the {} too. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:355: if (infobar_service) How can this be NULL if we're getting this notification? https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:425: number_of_infobars_++; Nit: Preincrement https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:427: if (number_of_infobars_ == 1) This needs to be placed inside the (state->infobar) conditional check. Otherwise, if we add one infobar, then fail to create the second, we'll try to AddObserver() again. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... File chrome/browser/ui/hung_plugin_tab_helper.h (right): https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.h:83: // The number of infobars we are observing. Nit: We don't actually observe infobars, so how about: "The number of hung plugin infobars open. We use this to determine when we should remove ourselves as an observer of InfoBarManager notifications."
Peter, I made the changes you suggested. I removed OnManagerShuttingDown, because in my testing, from what I could understand, we don't get this notification. I think it is because by the time we (HungPluginTabHelper) were already removed itself as an infobar observer. Let me know if I should get you more information about this. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:323: number_of_infobars_--; On 2014/05/31 03:44:59, Peter Kasting wrote: > Nit: Prefer predecrement to postdecrement Done. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:327: InfoBarService::FromWebContents(web_contents()); On 2014/05/31 03:44:59, Peter Kasting wrote: > Nit: Just inline this into the next statement: > > InfoBarService::FromWebContents(web_contents())->RemoveObserver(this); > > This will allow removing the {} too. Done. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:425: number_of_infobars_++; On 2014/05/31 03:44:59, Peter Kasting wrote: > Nit: Preincrement Done. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:427: if (number_of_infobars_ == 1) On 2014/05/31 03:44:59, Peter Kasting wrote: > This needs to be placed inside the (state->infobar) conditional check. > Otherwise, if we add one infobar, then fail to create the second, we'll try to > AddObserver() again. Done. https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... File chrome/browser/ui/hung_plugin_tab_helper.h (right): https://codereview.chromium.org/297293002/diff/230001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.h:83: // The number of infobars we are observing. On 2014/05/31 03:44:59, Peter Kasting wrote: > Nit: We don't actually observe infobars, so how about: > > "The number of hung plugin infobars open. We use this to determine when we > should remove ourselves as an observer of InfoBarManager notifications." Done.
https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:266: DCHECK_EQ(0, number_of_infobars_); Are you sure we're guaranteed that the InfoBarManager is destroyed before the HungPluginTabHelper? Because if the order could go either way, this DCHECK isn't right. It would probably help me to know what the callstacks are that lead to these objects' destruction, so it's clear that one must be destroyed later than the other. We should also have a comment here describing why this DCHECK holds. https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:323: --number_of_infobars_; This is not safe. You get called on every kind of infobar removal, and you need to be sure that you're being called about a hung plugin infobar before you act. In other words, all this code needs to move inside the conditional below where you find that this infobar is one of the infobars you're tracking. https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:324: Nit: I'd avoid this newline (2 places) https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... File chrome/browser/ui/hung_plugin_tab_helper.h (right): https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.h:82: // should remove ourselves as an observer of InfoBarManager notifications. Nit: ourself
Finally got some time to debug it. Please, see below. https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... File chrome/browser/ui/hung_plugin_tab_helper.cc (right): https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:265: HungPluginTabHelper::~HungPluginTabHelper() { Peter, I got the backtrace for this. It is this (beautiful) thing: Breakpoint 1, HungPluginTabHelper::~HungPluginTabHelper (this=0x3390eeb12bd0) at ../../chrome/browser/ui/hung_plugin_tab_helper.cc:265 265 HungPluginTabHelper::~HungPluginTabHelper() { (gdb) bt #0 HungPluginTabHelper::~HungPluginTabHelper (this=0x3390eeb12bd0) at ../../chrome/browser/ui/hung_plugin_tab_helper.cc:265 #1 0x0000555557acd58c in non-virtual thunk to HungPluginTabHelper::~HungPluginTabHelper() () at ../../chrome/browser/ui/hung_plugin_tab_helper.cc:267 #2 0x00007ffff5eeb837 in linked_ptr<base::SupportsUserData::Data>::depart (this=0x3390eeaf4eb8) at ../../base/memory/linked_ptr.h:146 #3 0x00007ffff5eea965 in linked_ptr<base::SupportsUserData::Data>::~linked_ptr (this=0x3390eeaf4eb8) at ../../base/memory/linked_ptr.h:84 #4 0x00007ffff5eeb41c in std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> >::~pair (this=0x3390eeaf4eb0) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_pair.h:88 #5 0x00007ffff5eeb3ec in std::_Rb_tree_node<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > >::~_Rb_tree_node ( this=0x3390eeaf4e90) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:130 troy<std::_Rb_tree_node<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > > > (this=0x7fffffff8200, __p=0x3390eeaf4e90) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/ext/new_allocator.h:114 #7 0x00007ffff5eeb32c in std::_Rb_tree<void const*, std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> >, std::_Select1st<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>, std::allocator<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > > >::_M_destroy_node (this=0x7fffffff8200, __p=0x3390eeaf4e90) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:419 #8 0x00007ffff5eed2db in std::_Rb_tree<void const*, std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> >, std::_Select1st<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>, std::allocator<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > > >::_M_erase (this=0x7fffffff8200, __x=0x3390eeaf4e90) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:1084 #9 0x00007ffff5eed2be in std::_Rb_tree<void const*, std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> >, std::_Select1st<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>, std::allocator<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > > >::_M_erase (this=0x7fffffff8200, __x=0x3390ee68e100) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:1082 #10 0x00007ffff5eed265 in std::_Rb_tree<void const*, std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> >, std::_Select1st<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>, std::allocator<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > > >::~_Rb_tree (this=0x7fffffff8200) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:646 #11 0x00007ffff5eed235 in std::__cxx1998::map<void const*, linked_ptr<base::SupportsUserData::Data>, std::less<void const*>, std::allocator<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > > >::~map (this=0x7fffffff8200) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_map.h:90 #12 0x00007ffff5eeaae1 in std::__debug::map<void const*, linked_ptr<base::SupportsUserData::Data>, std::less<void const*>, std::allocator<std::pair<void const* const, linked_ptr<base::SupportsUserData::Data> > > >::~map (this=0x7fffffff8200) at /usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/debug/map.h:107 #13 0x00007ffff5eea30e in base::SupportsUserData::~SupportsUserData (this=0x3390eda13230) at ../../base/supports_user_data.cc:43 ---Type <return> to continue, or q <return> to quit--- #14 0x00007fffe93ec065 in content::WebContents::~WebContents (this=0x3390eda13220) at ../../content/public/browser/web_contents.h:149 #15 0x00007fffe93ade4c in content::WebContentsImpl::~WebContentsImpl (this=0x3390eda13220) at ../../content/browser/web_contents/web_contents_impl.cc:426 #16 0x00007fffe93ae1a9 in content::WebContentsImpl::~WebContentsImpl (this=0x3390eda13220) at ../../content/browser/web_contents/web_contents_impl.cc:358 [...] https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:323: --number_of_infobars_; On 2014/06/03 18:41:45, Peter Kasting wrote: > This is not safe. You get called on every kind of infobar removal, and you need > to be sure that you're being called about a hung plugin infobar before you act. > > In other words, all this code needs to move inside the conditional below where > you find that this infobar is one of the infobars you're tracking. Done. https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.cc:324: On 2014/06/03 18:41:45, Peter Kasting wrote: > Nit: I'd avoid this newline (2 places) Done.
https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... File chrome/browser/ui/hung_plugin_tab_helper.h (right): https://codereview.chromium.org/297293002/diff/250001/chrome/browser/ui/hung_... chrome/browser/ui/hung_plugin_tab_helper.h:82: // should remove ourselves as an observer of InfoBarManager notifications. On 2014/06/03 18:41:45, Peter Kasting wrote: > Nit: ourself Done.
Your backtrace only tells me that the HungPluginTabHelper is torn down during WebContents destruction. It doesn't say when the InfoBarManager is torn down or the answer to the underlying question, which is whether the destruction order is guaranteed to be one way or the other, or could happen to go either way.
Peter, the bt for InfoBarManager is:
#0 infobars::InfoBarManager::~InfoBarManager (this=0x870c75822f0) at
../../components/infobars/core/infobar_manager.cc:105
#1 0x0000555555b09e72 in InfoBarService::~InfoBarService (this=0x870c75822f0)
at ../../chrome/browser/infobars/infobar_service.cc:60
#2 0x0000555555b09f29 in InfoBarService::~InfoBarService (this=0x870c75822f0)
at ../../chrome/browser/infobars/infobar_service.cc:58
#3 0x0000555555b09fbc in non-virtual thunk to InfoBarService::~InfoBarService()
() at ../../chrome/browser/infobars/infobar_service.cc:60
#4 0x00007ffff5eeb837 in linked_ptr<base::SupportsUserData::Data>::depart
(this=0x870c80e3b38) at ../../base/memory/linked_ptr.h:146
#5 0x00007ffff5eea965 in linked_ptr<base::SupportsUserData::Data>::~linked_ptr
(this=0x870c80e3b38) at ../../base/memory/linked_ptr.h:84
#6 0x00007ffff5eeb41c in std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> >::~pair (this=0x870c80e3b30)
at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_pair.h:88
#7 0x00007ffff5eeb3ec in std::_Rb_tree_node<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >::~_Rb_tree_node (
this=0x870c80e3b10) at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:130
#8 0x00007ffff5eeb359 in
__gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >
>::destroy<std::_Rb_tree_node<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > > (this=0x870c7433238,
__p=0x870c80e3b10)
at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/ext/new_allocator.h:114
#9 0x00007ffff5eeb32c in std::_Rb_tree<void const*, std::pair<void const*
const, linked_ptr<base::SupportsUserData::Data> >,
std::_Select1st<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::_M_destroy_node
(this=0x870c7433238, __p=0x870c80e3b10)
at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:419
#10 0x00007ffff5eeb2de in std::_Rb_tree<void const*, std::pair<void const*
const, linked_ptr<base::SupportsUserData::Data> >,
std::_Select1st<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::_M_erase_aux
(this=0x870c7433238, __position=...)
at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:1498
#11 0x00007ffff5eeb25a in std::_Rb_tree<void const*, std::pair<void const*
const, linked_ptr<base::SupportsUserData::Data> >,
std::_Select1st<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > >, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::erase (this=0x870c7433238,
__position=...)
at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_tree.h:777
#12 0x00007ffff5eeb1f5 in std::__cxx1998::map<void const*,
linked_ptr<base::SupportsUserData::Data>, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::erase (this=0x870c7433238,
__position=...)
at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/stl_map.h:624
#13 0x00007ffff5eeaa1f in std::__debug::map<void const*,
linked_ptr<base::SupportsUserData::Data>, std::less<void const*>,
std::allocator<std::pair<void const* const,
linked_ptr<base::SupportsUserData::Data> > > >::erase (this=0x870c7433238,
__x=@0x7fffffff83d0: 0x55555b083f38
<content::WebContentsUserData<InfoBarService>::kLocatorKey>)
at
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/debug/map.h:297
#14 0x00007ffff5eea181 in base::SupportsUserData::RemoveUserData
(this=0x870c7433230,
key=0x55555b083f38
<content::WebContentsUserData<InfoBarService>::kLocatorKey>) at
../../base/supports_user_data.cc:29
#15 0x0000555555b0a2d0 in InfoBarService::WebContentsDestroyed
(this=0x870c75822f0) at ../../chrome/browser/infobars/infobar_service.cc:102
---Type <return> to continue, or q <return> to quit---
#16 0x0000555555b0a2fc in non-virtual thunk to
InfoBarService::WebContentsDestroyed() () at
../../chrome/browser/infobars/infobar_service.cc:105
#17 0x00007fffe93ada44 in content::WebContentsImpl::~WebContentsImpl
(this=0x870c7433220)
at ../../content/browser/web_contents/web_contents_impl.cc:414
#18 0x00007fffe93ae1a9 in content::WebContentsImpl::~WebContentsImpl
(this=0x870c7433220)
at ../../content/browser/web_contents/web_contents_impl.cc:358
#19 0x000055555779f9a7 in TabStripModel::InternalCloseTab (this=0x870c763a020,
contents=0x870c7433220, index=2, create_historical_tabs=true)
at ../../chrome/browser/ui/tabs/tab_strip_model.cc:1272
So whether it can be destroyed before or after HungPluginTabHelper, I'd say it
gets destroyed before, because it was registered with WebContents before.
InfoBarService is created at line 131 of chrome/browser/ui/tab_helpers.cc while
HungPluginTabHelper is created at line 154.
And that is because both are content::WebContentsObserver and when
WebContentsImpl is destroyed it fires WebContentsDestroyed() notification.
InfoBarService gets it first, because it was registered first.
That is my interpretation of the code and how it flows.
On 2014/06/09 02:00:40, tfarina wrote:
> So whether it can be destroyed before or after HungPluginTabHelper, I'd say it
> gets destroyed before, because it was registered with WebContents before.
>
> InfoBarService is created at line 131 of chrome/browser/ui/tab_helpers.cc
while
> HungPluginTabHelper is created at line 154.
I think you're correct, but I also think this sort of thing could very easily
change in the future. If we wanted to enforce this, at a minimum we'd need a
comment in tab_helpers.cc noting the destruction order.
I think a better fix would be to simply make the HungPluginTabHelper robust
against the infobar manager getting destroyed first. We could do this by
changing the destructor to something like the following:
// If we're getting shut down before the InfoBarService, we need to unregister
// as an observer.
if (number_of_infobars_ != 0)
InfoBarService::FromWebContents(web_contents())->RemoveObserver(this);
LGTM with that change.
Peter, thanks for the patience with me in this review. Change addressed. Pushing to CQ. Thanks,
The CQ bit was checked by tfarina@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tfarina@chromium.org/297293002/310001
Message was sent while issue was closed.
Change committed as 277292
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/338353003/ by tfarina@chromium.org. The reason for reverting is: Caused a crash on Windows. See crbug.com/354380. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
