AU: HttpFetcher: Fix segfault with multi fetcher and small transfers
When a proxy resolver resolves proxies, it calls into HttpFetcher w/
the results, which then kicks off a transfer (by executing a member
variable callback of HttpFetcher). After HttpFetcher calls that
callback, it sets the callback to NULL.
When using MultiHttpFetcher, the same HttpFetcher is used multiple
times. If we have a very small transfer, what can happen is this.
Time goes down, callstack grows to the right:
First transfer proxies resolved
++ HttpFetcher called, calls a callback
+++ Transfer begins
++++ Transfer ends
+++++ MultiFetcher told transfer ends
++++++ MultiFetcher starts new transfer
+++++++ HttpFetcher installs a new callback member var
++ HttpFetcher zeros out callback member var
Second transfer proxies resolved
++ HttpFetcher member var is NULL - CRASH
To fix this issue, HttpFetcher makes a local copy of the callback
pointer, then sets the member variable to NULL, then calls the
callback.
(On a side note, it is CLs and bugs like these that make me wonder if
maybe we should have used Chrome's threading model rather than going
single threaded and async.)
Also, fix a comment in MultiHttpFetcher
Change-Id: Ib87c1e75211bc56cecea7f7298576371184d586c
BUG=chromium-os:13388
TEST=unittests
Review URL: http://codereview.chromium.org/6693047
diff --git a/http_fetcher.cc b/http_fetcher.cc
index 7d9297a..9e1e0ac 100644
--- a/http_fetcher.cc
+++ b/http_fetcher.cc
@@ -31,6 +31,7 @@
no_resolver_idle_id_ = g_idle_add(utils::GlibRunClosure, callback);
return true;
}
+ CHECK_EQ(reinterpret_cast<Closure*>(NULL), callback_);
callback_ = callback;
return proxy_resolver_->GetProxiesForUrl(url,
&HttpFetcher::StaticProxiesResolved,
@@ -41,8 +42,11 @@
no_resolver_idle_id_ = 0;
if (!proxies.empty())
SetProxies(proxies);
- callback_->Run();
+ CHECK_NE(reinterpret_cast<Closure*>(NULL), callback_);
+ Closure* callback = callback_;
callback_ = NULL;
+ // This may indirectly call back into ResolveProxiesForUrl():
+ callback->Run();
}
} // namespace chromeos_update_engine