diff options
| author | sky@chromium.org <sky@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-01-13 16:59:52 +0000 |
|---|---|---|
| committer | sky@chromium.org <sky@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-01-13 16:59:52 +0000 |
| commit | 475478894062d23834cd660f4bba6eaf385e958d (patch) | |
| tree | 4f54862d694060219110391e6c51d5c3de80ca93 /chrome | |
| parent | 8b8fab971151ba246c0930fa25c68b705f6a6c0b (diff) | |
| download | chromium_src-475478894062d23834cd660f4bba6eaf385e958d.zip chromium_src-475478894062d23834cd660f4bba6eaf385e958d.tar.gz chromium_src-475478894062d23834cd660f4bba6eaf385e958d.tar.bz2 | |
Attempt at fixing crash in ProcessPendingTabs. With the current code,
during closing a window with unload handlers if a tab is disconnected then
we delay cleanup by way of PostTask with the tab being
disconnected. During the time between when the disconnect happens and
the task is run the unloader handler sets still maintain a reference
to the tab. If ProcessPendingTabs is invoked during this window and
the tab is deleted, then we can crash.
I'm fixing it by making disconnect remove from the sets immediately,
but delay the call to ProcessPendingTabs. I'm also doing a similar
thing in TabDetachedAt.
BUG=15620
TEST=none
Review URL: http://codereview.chromium.org/6203006
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@71327 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
| -rw-r--r-- | chrome/browser/ui/browser.cc | 35 | ||||
| -rw-r--r-- | chrome/browser/ui/browser.h | 9 | ||||
| -rw-r--r-- | chrome/test/data/reliability/known_crashes.txt | 3 |
3 files changed, 30 insertions, 17 deletions
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc index 3ba24b0..da64bdaa 100644 --- a/chrome/browser/ui/browser.cc +++ b/chrome/browser/ui/browser.cc @@ -2837,7 +2837,7 @@ void Browser::CloseContents(TabContents* source) { // waiting for unload to fire. Don't actually try to close the tab as it // will go down the slow shutdown path instead of the fast path of killing // all the renderer processes. - ClearUnloadState(source); + ClearUnloadState(source, true); return; } @@ -3198,12 +3198,10 @@ void Browser::Observe(NotificationType type, switch (type.value) { case NotificationType::TAB_CONTENTS_DISCONNECTED: if (is_attempting_to_close_browser_) { - // Need to do this asynchronously as it will close the tab, which is - // currently on the call stack above us. - MessageLoop::current()->PostTask( - FROM_HERE, - method_factory_.NewRunnableMethod(&Browser::ClearUnloadState, - Source<TabContents>(source).ptr())); + // Pass in false so that we delay processing. We need to delay the + // processing as it may close the tab, which is currently on the call + // stack above us. + ClearUnloadState(Source<TabContents>(source).ptr(), false); } break; @@ -3860,7 +3858,7 @@ void Browser::ProcessPendingTabs() { if (tab->render_view_host()) { tab->render_view_host()->FirePageBeforeUnload(false); } else { - ClearUnloadState(tab); + ClearUnloadState(tab, true); } } else if (!tabs_needing_unload_fired_.empty()) { // We've finished firing all beforeunload events and can proceed with unload @@ -3877,7 +3875,7 @@ void Browser::ProcessPendingTabs() { if (tab->render_view_host()) { tab->render_view_host()->ClosePage(false, -1, -1); } else { - ClearUnloadState(tab); + ClearUnloadState(tab, true); } } else { NOTREACHED(); @@ -3917,18 +3915,23 @@ bool Browser::RemoveFromSet(UnloadListenerSet* set, TabContents* tab) { return false; } -void Browser::ClearUnloadState(TabContents* tab) { +void Browser::ClearUnloadState(TabContents* tab, bool process_now) { // Closing of browser could be canceled (via IsClosingPermitted) between the // time when request was initiated and when this method is called, so check // for is_attempting_to_close_browser_ flag before proceeding. if (is_attempting_to_close_browser_) { RemoveFromSet(&tabs_needing_before_unload_fired_, tab); RemoveFromSet(&tabs_needing_unload_fired_, tab); - ProcessPendingTabs(); + if (process_now) { + ProcessPendingTabs(); + } else { + MessageLoop::current()->PostTask( + FROM_HERE, + method_factory_.NewRunnableMethod(&Browser::ProcessPendingTabs)); + } } } - /////////////////////////////////////////////////////////////////////////////// // Browser, In-progress download termination handling (private): @@ -4087,6 +4090,14 @@ void Browser::TabDetachedAtImpl(TabContentsWrapper* contents, int index, find_bar_controller_->ChangeTabContents(NULL); } + if (is_attempting_to_close_browser_) { + // If this is the last tab with unload handlers, then ProcessPendingTabs + // would call back into the TabStripModel (which is invoking this method on + // us). Avoid that by passing in false so that the call to + // ProcessPendingTabs is delayed. + ClearUnloadState(contents->tab_contents(), false); + } + registrar_.Remove(this, NotificationType::TAB_CONTENTS_DISCONNECTED, Source<TabContentsWrapper>(contents)); } diff --git a/chrome/browser/ui/browser.h b/chrome/browser/ui/browser.h index 16b0aba..ecc1d15 100644 --- a/chrome/browser/ui/browser.h +++ b/chrome/browser/ui/browser.h @@ -909,8 +909,13 @@ class Browser : public TabHandlerDelegate, // Cleans up state appropriately when we are trying to close the browser and // the tab has finished firing its unload handler. We also use this in the // cases where a tab crashes or hangs even if the beforeunload/unload haven't - // successfully fired. - void ClearUnloadState(TabContents* tab); + // successfully fired. If |process_now| is true |ProcessPendingTabs| is + // invoked immediately, otherwise it is invoked after a delay (PostTask). + // + // Typically you'll want to pass in true for |process_now|. Passing in true + // may result in deleting |tab|. If you know that shouldn't happen (because of + // the state of the stack), pass in false. + void ClearUnloadState(TabContents* tab, bool process_now); // In-progress download termination handling ///////////////////////////////// diff --git a/chrome/test/data/reliability/known_crashes.txt b/chrome/test/data/reliability/known_crashes.txt index 03f42b1..8413585 100644 --- a/chrome/test/data/reliability/known_crashes.txt +++ b/chrome/test/data/reliability/known_crashes.txt @@ -40,9 +40,6 @@ REGEX : `anonymous namespace'::purecall$ # is the only one on the stack, so ignore that instance REGEX : `anonymous namespace'::invalidparameter$ -# 15620 -PREFIX : browser::processpendingtabs___browser::clearunloadstate - # 47207 PREFIX : messageloop::runtask___messageloop::deferorrunpendingtask___messageloop::dodelayedwork___base::messagepumpforio::dorunloop___base::messagepumpwin::run___messageloop::runinternal___messageloop::run___base::thread::run___base::thread::threadmain___`anonymous namespace'::threadfunc PREFIX : messageloop::runtask___messageloop::dodelayedwork___base::messagepumpforio::dorunloop___base::messagepumpwin::run___messageloop::runinternal___messageloop::run___base::thread::run___base::thread::threadmain___`anonymous namespace'::threadfunc |