Changed the update protocol for component updater from v2 to v3.

Changed the update protocol for component updater from v2 to v3.
BUG=314521
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236727

[Sorin Jianu, 2013-11-18 02:19:38]

Josh, let's get a first pass at this.
 
The manifest parser is "branched" off the extensions updater code and I kept the
public interface to it as close as possible. Similar for its unit tests.

[waffles, 2013-11-18 22:15:59]

Thanks!

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_ping_manager.cc:83:
item->next_version.GetString().c_str(),         // "nextversion"
What is the benefit of sending previous_version and next_version at the <app>
level rather than at the <event> level? Simply to avoid sending one of the
versions twice?

We briefly discussed Friday:

What is the outcome version of the update?
Well, if <event eventresult represents a success, it is <app nextversion>,
otherwise it is <app version>.
I don't like this, and I don't like the fact that it makes various elements of
our protocol uncombinable (e.g. you can never send an <event> and an
<updatecheck> together).

(Sidenote: it's frustrating that we don't have a prescriptive definition of our
protocol. The document we have is primarily descriptive but lacks detail. Note
that our protocol document never explicitly states that an <event> and an
<updatecheck> may not be sent in the same request.)

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_ping_manager.cc:96:
std::string ping_event("<event eventtype=\"3\"");
Any thoughts about sending eventtype="2" for updates from 0.0.0.0? I guess I'm
inclined to say that we shouldn't. Even such an update is still an "update", not
really an "install".

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:796: // Next we
can go back to components we already checked, here
Can't we get rid of these three separate loops altogether now, and simply have
one that adds every item to the updatecheck?

If you like, we can still enforce the MinimumReCheckWait for non-on-demand
items, but I'm not sure what having the MinimumReCheckWait gives us anymore.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:857: // process
for the production environment. This mitigates the case where an
This comment seems to be out of date.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:874: size_t
num_updates_pending = 0;
Why not just a bool?

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:923: if
(!it->crx_urls.empty())
What happens if <urls> is empty? When do we reset crx_url? Should we clear it
here? Note that we don't clear the URLs in AddItemToUpdateCheck.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:925: if
(!it->crx_diffurls.empty())
Same as above for diffurls, except that the parser doesn't report an error if
diffurls is empty, so we need to be more careful about this case.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_utils.h (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_utils.h:37: // Sends a
protocol request to the the service endpoind specified by |url|.
endpoind -> endpoint

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/test/component_updater_service_unittest.cc
(right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/test/component_updater_service_unittest.cc:221:
RunThreadsUntilIdle();
Are you certain that this is safe?

runloop.Run() will quit when the test_configurator tells it to (e.g. at the end
of N runs).

If we RunThreadsUntilIdle here, we need to verify that no test will break if we
do an extra call to ProcessPendingItems, or that when the test_configurator
tells the loop to quit, we haven't posted an additional call to
ProcessPendingItems already (or will soon).

Unless quitting prevents us from posting additional tasks to the threads... hm,
not sure.

Are you sure this is safe?

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/test/update_manifest_unittest.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/test/update_manifest_unittest.cc:170: 
Should we add a test for a response with diffs?

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/test/url_request_post_interceptor.h
(right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/test/url_request_post_interceptor.h:57: //
Returns how many request in total have been captured by the intereceptor.
request -> requests
intereceptor -> interceptor

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/update_manifest.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/update_manifest.cc:251: 
Extra newline here.

https://codereview.chromium.org/74893002/diff/1/chrome/test/data/components/u...
File chrome/test/data/components/updatecheck_diff_reply_3.xml (right):

https://codereview.chromium.org/74893002/diff/1/chrome/test/data/components/u...
chrome/test/data/components/updatecheck_diff_reply_3.xml:4: <updatecheck
status='noupdate'/>
spaces at end of line

https://codereview.chromium.org/74893002/diff/1/chrome/test/data/components/u...
File chrome/test/data/components/updatecheck_reply_3.xml (right):

https://codereview.chromium.org/74893002/diff/1/chrome/test/data/components/u...
chrome/test/data/components/updatecheck_reply_3.xml:4: <updatecheck
status='noupdate'/>
spaces at end of line

https://codereview.chromium.org/74893002/diff/1/chrome/test/data/components/u...
File chrome/test/data/components/updatecheck_reply_noupdate.xml (right):

https://codereview.chromium.org/74893002/diff/1/chrome/test/data/components/u...
chrome/test/data/components/updatecheck_reply_noupdate.xml:4: <updatecheck
status='noupdate'/>
spaces at end of line

[Sorin Jianu, 2013-11-19 04:25:45]

Thank you Josh!

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_ping_manager.cc:83:
item->next_version.GetString().c_str(),         // "nextversion"
Let's discuss and see if there is a way to fix that.

On 2013/11/18 22:15:59, waffles wrote:
> What is the benefit of sending previous_version and next_version at the <app>
> level rather than at the <event> level? Simply to avoid sending one of the
> versions twice?
> 
> We briefly discussed Friday:
> 
> What is the outcome version of the update?
> Well, if <event eventresult represents a success, it is <app nextversion>,
> otherwise it is <app version>.
> I don't like this, and I don't like the fact that it makes various elements of
> our protocol uncombinable (e.g. you can never send an <event> and an
> <updatecheck> together).
> 
> (Sidenote: it's frustrating that we don't have a prescriptive definition of
our
> protocol. The document we have is primarily descriptive but lacks detail. Note
> that our protocol document never explicitly states that an <event> and an
> <updatecheck> may not be sent in the same request.)

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_ping_manager.cc:96:
std::string ping_event("<event eventtype=\"3\"");
On 2013/11/18 22:15:59, waffles wrote:
> Any thoughts about sending eventtype="2" for updates from 0.0.0.0? I guess I'm
> inclined to say that we shouldn't. Even such an update is still an "update",
not
> really an "install".

If could add that is you think it can help with the queries. I would like to
avoid it but I will make the change if you'd like.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:796: // Next we
can go back to components we already checked, here
On 2013/11/18 22:15:59, waffles wrote:
> Can't we get rid of these three separate loops altogether now, and simply have
> one that adds every item to the updatecheck?
> 
> If you like, we can still enforce the MinimumReCheckWait for non-on-demand
> items, but I'm not sure what having the MinimumReCheckWait gives us anymore.

If we do that, I will do it in another change.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:857: // process
for the production environment. This mitigates the case where an
On 2013/11/18 22:15:59, waffles wrote:
> This comment seems to be out of date.

Done.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:874: size_t
num_updates_pending = 0;
On 2013/11/18 22:15:59, waffles wrote:
> Why not just a bool?

Because it was a size_t before and it seems useful to know how many
components have updates for debugging purposes.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:923: if
(!it->crx_urls.empty())
On 2013/11/18 22:15:59, waffles wrote:
> What happens if <urls> is empty? When do we reset crx_url? Should we clear it
> here? Note that we don't clear the URLs in AddItemToUpdateCheck.

Thank you. I am now resetting the urls along with the rest of the state in
AddItemToUpdateCheck.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.cc:925: if
(!it->crx_diffurls.empty())
On 2013/11/18 22:15:59, waffles wrote:
> Same as above for diffurls, except that the parser doesn't report an error if
> diffurls is empty, so we need to be more careful about this case.

Done. If the GURLs are empty, then the download code with fail at the time of
download and a ping will be created then and there.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_utils.h (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_utils.h:37: // Sends a
protocol request to the the service endpoind specified by |url|.
On 2013/11/18 22:15:59, waffles wrote:
> endpoind -> endpoint

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/test/component_updater_service_unittest.cc
(right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/test/component_updater_service_unittest.cc:221:
RunThreadsUntilIdle();
On 2013/11/18 22:15:59, waffles wrote:
> Are you certain that this is safe?
> 
> runloop.Run() will quit when the test_configurator tells it to (e.g. at the
end
> of N runs).
> 
> If we RunThreadsUntilIdle here, we need to verify that no test will break if
we
> do an extra call to ProcessPendingItems, or that when the test_configurator
> tells the loop to quit, we haven't posted an additional call to
> ProcessPendingItems already (or will soon).
> 
> Unless quitting prevents us from posting additional tasks to the threads...
hm,
> not sure.
> 
> Are you sure this is safe?

I am not sure about it and I removed the call.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/test/update_manifest_unittest.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/test/update_manifest_unittest.cc:170: 
On 2013/11/18 22:15:59, waffles wrote:
> Should we add a test for a response with diffs?

I've added a quick test.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/test/url_request_post_interceptor.h
(right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/test/url_request_post_interceptor.h:57: //
Returns how many request in total have been captured by the intereceptor.
On 2013/11/18 22:15:59, waffles wrote:
> request -> requests
> intereceptor -> interceptor

Done.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/update_manifest.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/update_manifest.cc:251: 
On 2013/11/18 22:15:59, waffles wrote:
> Extra newline here.

Done.

https://codereview.chromium.org/74893002/diff/1/chrome/test/data/components/u...
File chrome/test/data/components/updatecheck_diff_reply_3.xml (right):

https://codereview.chromium.org/74893002/diff/1/chrome/test/data/components/u...
chrome/test/data/components/updatecheck_diff_reply_3.xml:4: <updatecheck
status='noupdate'/>
On 2013/11/18 22:15:59, waffles wrote:
> spaces at end of line

Done.

[waffles, 2013-11-19 18:33:57]

lgtm

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_ping_manager.cc:96:
std::string ping_event("<event eventtype=\"3\"");
On 2013/11/19 04:25:45, Sorin Jianu wrote:
> On 2013/11/18 22:15:59, waffles wrote:
> > Any thoughts about sending eventtype="2" for updates from 0.0.0.0? I guess
I'm
> > inclined to say that we shouldn't. Even such an update is still an "update",
> not
> > really an "install".
> 
> If could add that is you think it can help with the queries. I would like to
> avoid it but I will make the change if you'd like.

Let's avoid it for now. The distinction between install and update is kind of
arbitrary in this case.

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/test/component_updater_service_unittest.cc
(right):

https://codereview.chromium.org/74893002/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/test/component_updater_service_unittest.cc:221:
RunThreadsUntilIdle();
On 2013/11/19 04:25:45, Sorin Jianu wrote:
> On 2013/11/18 22:15:59, waffles wrote:
> > Are you certain that this is safe?
> > 
> > runloop.Run() will quit when the test_configurator tells it to (e.g. at the
> end
> > of N runs).
> > 
> > If we RunThreadsUntilIdle here, we need to verify that no test will break if
> we
> > do an extra call to ProcessPendingItems, or that when the test_configurator
> > tells the loop to quit, we haven't posted an additional call to
> > ProcessPendingItems already (or will soon).
> > 
> > Unless quitting prevents us from posting additional tasks to the threads...
> hm,
> > not sure.
> > 
> > Are you sure this is safe?
> 
> I am not sure about it and I removed the call.

Ok, looks like it was needed after all? (As mentioned in person, I'm pretty sure
it's not a problem, was just nervous about it.)

[Cris Neckar, 2013-11-19 23:44:43]

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
File chrome/browser/component_updater/update_manifest.cc (right):

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:116: p.namediff =
GetAttribute(package, "namediff");
What happens on null return from GetAttribute() for all of these?

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:124: if
(base::StringToInt(GetAttribute(package, "size"), &size)) {
This looks like at minimum a null deref in the failure case.

what if the size if zero/negative/very large?

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:227: if
(GetAttribute(updatecheck, "status") == "noupdate") {
Is it ok that we drop through here when this returns an empty string?

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:283: ScopedXmlErrorFunc
error_func(&xml_errors, &XmlErrorFunc);
As discussed over chat this is unsafe at any speed as it is global. Another
thread using libxml concurrently will cause this to go *boom*.

That being said, and because we are going to get rid of libxml for this use
case, this seems to be the only consumer of libxml in the browser process so it
is theoretically safe until someone else adds something that uses it.

filed https://code.google.com/p/chromium/issues/detail?id=321336

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:286: ScopedXmlDocument
document(xmlParseDoc(
Reopened https://code.google.com/p/chromium/issues/detail?id=124228

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:315: std::string
elapsed_seconds = GetAttribute(first, "elapsed_seconds");
Need to handle the failure case here

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:317: if
(base::StringToInt(elapsed_seconds, &parsed_elapsed)) {
Sanity check here maybe. What happens if this is negative/zero/huge.

[Sorin Jianu, 2013-11-20 04:05:21]

Cris, thank you very much for reviewing the code. I think we are good so far,
and please take another look.

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
File chrome/browser/component_updater/update_manifest.cc (right):

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:116: p.namediff =
GetAttribute(package, "namediff");
On 2013/11/19 23:44:43, Cris Neckar wrote:
> What happens on null return from GetAttribute() for all of these?

GetAttribute() returns an empty std::string which is assign to the left hand
side of the expression. In this case, p.namediff will contain an empty string.
Later on, namediff will be used in this context:
it->crx_diffurls[0].Resolve(package.namediff);

In general, we should handle empty strings for the manifest elements graciously,
and I believe we do, since our UT canned responses usually contain the bare
minimum of information.

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:124: if
(base::StringToInt(GetAttribute(package, "size"), &size)) {
On 2013/11/19 23:44:43, Cris Neckar wrote:
> This looks like at minimum a null deref in the failure case.
> 
> what if the size if zero/negative/very large?

StringToInt works correctly when provided with empty arguments. It deals with
input out of range as well as invalid input. At any rate, I've added a unit test
to check for these conditions.

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:227: if
(GetAttribute(updatecheck, "status") == "noupdate") {
On 2013/11/19 23:44:43, Cris Neckar wrote:
> Is it ok that we drop through here when this returns an empty string?

It is ok. It means that we don't have an update. In this case, we won't have any
version in the results, therefore, when we iterate for each component, we will
consider it up to date and move on.

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:283: ScopedXmlErrorFunc
error_func(&xml_errors, &XmlErrorFunc);
On 2013/11/19 23:44:43, Cris Neckar wrote:
> As discussed over chat this is unsafe at any speed as it is global. Another
> thread using libxml concurrently will cause this to go *boom*.
> 
> That being said, and because we are going to get rid of libxml for this use
> case, this seems to be the only consumer of libxml in the browser process so
it
> is theoretically safe until someone else adds something that uses it.
> 
> filed https://code.google.com/p/chromium/issues/detail?id=321336

It turns out the the story is more complex than this. 

libxml is thread safe if built and used as a threaded library. This error
handler is per-thread data. So if we used it correctly, then we would be ok.

I tested using local debug and release builds of Chromium and I can see that the
code is thread-aware, and in practice we are ok. However, I can't see where in
the build scripts we are setting up the flags to build libxml as a MT library.
Also, we are not calling the libxml functions to initialize the library at all,
however, inspecting and tracing the libxml code, the library is lazy
initializing its data structures for MT usage.

In conclusion, we are safe, and this could become a moot point if we change the
way we parse.

Ok so far?

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:286: ScopedXmlDocument
document(xmlParseDoc(
On 2013/11/19 23:44:43, Cris Neckar wrote:
> Reopened https://code.google.com/p/chromium/issues/detail?id=124228

Agreed. The bug is not related with v2 to v3 migration. We will address that bug
in a different CL.

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:315: std::string
elapsed_seconds = GetAttribute(first, "elapsed_seconds");
On 2013/11/19 23:44:43, Cris Neckar wrote:
> Need to handle the failure case here

Empty string is acceptable and the code handles that.

https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
chrome/browser/component_updater/update_manifest.cc:317: if
(base::StringToInt(elapsed_seconds, &parsed_elapsed)) {
On 2013/11/19 23:44:43, Cris Neckar wrote:
> Sanity check here maybe. What happens if this is negative/zero/huge.

as above, StringToInt handles the errors.

[Cris Neckar, 2013-11-20 18:40:36]

On 2013/11/20 04:05:21, Sorin Jianu wrote:
> Cris, thank you very much for reviewing the code. I think we are good so far,
> and please take another look.
> 
>
https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
> File chrome/browser/component_updater/update_manifest.cc (right):
> 
>
https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
> chrome/browser/component_updater/update_manifest.cc:116: p.namediff =
> GetAttribute(package, "namediff");
> On 2013/11/19 23:44:43, Cris Neckar wrote:
> > What happens on null return from GetAttribute() for all of these?
> 
> GetAttribute() returns an empty std::string which is assign to the left hand
> side of the expression. In this case, p.namediff will contain an empty string.
> Later on, namediff will be used in this context:
> it->crx_diffurls[0].Resolve(package.namediff);
> 
> In general, we should handle empty strings for the manifest elements
graciously,
> and I believe we do, since our UT canned responses usually contain the bare
> minimum of information.
> 
>
https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
> chrome/browser/component_updater/update_manifest.cc:124: if
> (base::StringToInt(GetAttribute(package, "size"), &size)) {
> On 2013/11/19 23:44:43, Cris Neckar wrote:
> > This looks like at minimum a null deref in the failure case.
> > 
> > what if the size if zero/negative/very large?
> 
> StringToInt works correctly when provided with empty arguments. It deals with
> input out of range as well as invalid input. At any rate, I've added a unit
test
> to check for these conditions.
> 
>
https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
> chrome/browser/component_updater/update_manifest.cc:227: if
> (GetAttribute(updatecheck, "status") == "noupdate") {
> On 2013/11/19 23:44:43, Cris Neckar wrote:
> > Is it ok that we drop through here when this returns an empty string?
> 
> It is ok. It means that we don't have an update. In this case, we won't have
any
> version in the results, therefore, when we iterate for each component, we will
> consider it up to date and move on.
> 
>
https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
> chrome/browser/component_updater/update_manifest.cc:283: ScopedXmlErrorFunc
> error_func(&xml_errors, &XmlErrorFunc);
> On 2013/11/19 23:44:43, Cris Neckar wrote:
> > As discussed over chat this is unsafe at any speed as it is global. Another
> > thread using libxml concurrently will cause this to go *boom*.
> > 
> > That being said, and because we are going to get rid of libxml for this use
> > case, this seems to be the only consumer of libxml in the browser process so
> it
> > is theoretically safe until someone else adds something that uses it.
> > 
> > filed https://code.google.com/p/chromium/issues/detail?id=321336
> 
> It turns out the the story is more complex than this. 
> 
> libxml is thread safe if built and used as a threaded library. This error
> handler is per-thread data. So if we used it correctly, then we would be ok.
> 
> I tested using local debug and release builds of Chromium and I can see that
the
> code is thread-aware, and in practice we are ok. However, I can't see where in
> the build scripts we are setting up the flags to build libxml as a MT library.
> Also, we are not calling the libxml functions to initialize the library at
all,
> however, inspecting and tracing the libxml code, the library is lazy
> initializing its data structures for MT usage.
> 
> In conclusion, we are safe, and this could become a moot point if we change
the
> way we parse.
> 
> Ok so far?
> 
>
https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
> chrome/browser/component_updater/update_manifest.cc:286: ScopedXmlDocument
> document(xmlParseDoc(
> On 2013/11/19 23:44:43, Cris Neckar wrote:
> > Reopened https://code.google.com/p/chromium/issues/detail?id=124228
> 
> Agreed. The bug is not related with v2 to v3 migration. We will address that
bug
> in a different CL.
> 
>
https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
> chrome/browser/component_updater/update_manifest.cc:315: std::string
> elapsed_seconds = GetAttribute(first, "elapsed_seconds");
> On 2013/11/19 23:44:43, Cris Neckar wrote:
> > Need to handle the failure case here
> 
> Empty string is acceptable and the code handles that.
> 
>
https://codereview.chromium.org/74893002/diff/210001/chrome/browser/component...
> chrome/browser/component_updater/update_manifest.cc:317: if
> (base::StringToInt(elapsed_seconds, &parsed_elapsed)) {
> On 2013/11/19 23:44:43, Cris Neckar wrote:
> > Sanity check here maybe. What happens if this is negative/zero/huge.
> 
> as above, StringToInt handles the errors.

LGTM

[Sorin Jianu, 2013-11-20 19:12:52]

Carlos, could you please take a look here from the code owners perspective?

Thank you!

[cpu_(ooo_6.6-7.5), 2013-11-22 02:38:42]

looks great lgtm

[commit-bot: I haz the power, 2013-11-22 02:40:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/74893002/480001

[commit-bot: I haz the power, 2013-11-22 04:33:20]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-22 04:53:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/74893002/480001

[commit-bot: I haz the power, 2013-11-22 06:54:30]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-22 07:27:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/74893002/480001

[commit-bot: I haz the power, 2013-11-22 08:23:24]

Change committed as 236727