Upstream the updated Java Omaha XML parser

Upstream the updated Java Omaha XML parser

We hit an issue where the server protocol was updated on 10/29 to send an additional tag was not part of the original spec.  This caused all Chrome clients across all versions to simultaneously think that the server was failing to send a valid response and send an increased number of pings to the server.
    
This CL rewrites the Java parser to be more robust to random changes to the XML as a precursor to the C++ reimplementation (which is still a long ways out).  The code also gets moved upstream so that we can get it reviewed easier.

BUG=181323
TEST=ResponseParserTest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247594

[gone, 2013-11-08 22:05:32]

Moving this to the public repo so that the code's easier to review.

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:205:
success &= TextUtils.equals("3.0", node.attributes.get("protocol"));
How long will server protocol 3.0 be handled for?

[waffles, 2013-11-11 17:34:26]

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:128:
public int getDaystartSeconds() {
I'm curious about how Chrome on these platforms uses Daystart. Why do we record
it at all?

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:192:
}
Why require that a response that contains an install event may not contain an
update response or ping response (or vice versa)?

i.e. if this happens (for whatever reason), is it worth throwing out the
response (and retrying the request later)?

[nyquist, 2013-11-11 17:43:41]

https://codereview.chromium.org/67313003/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://codereview.chromium.org/67313003/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:89:
private String mMarketVersion;
mPlayVersion ?

https://codereview.chromium.org/67313003/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:205:
success &= TextUtils.equals("3.0", node.attributes.get("protocol"));
On 2013/11/08 22:05:32, dfalcantara wrote:
> How long will server protocol 3.0 be handled for?

And also, what is the expected client behavior when it will not work anymore...?

https://codereview.chromium.org/67313003/diff/1/chrome/android/javatests/src/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java
(right):

https://codereview.chromium.org/67313003/diff/1/chrome/android/javatests/src/...
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java:15:
"https://market.android.com/details?id=com.google.android.apps.chrome";
Is this still the URL we want?
Could we use https://play.google.com/store/apps/details?id=[some package]

[gone, 2013-11-11 18:22:34]

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:89:
private String mMarketVersion;
On 2013/11/11 17:43:41, nyquist (GMT 11.4-11.18) wrote:
> mPlayVersion ?

Done.

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:128:
public int getDaystartSeconds() {
On 2013/11/11 17:34:27, waffles wrote:
> I'm curious about how Chrome on these platforms uses Daystart. Why do we
record
> it at all?

We actually used to use it to send a single ping per day before a requirement
came in that we check for updates more frequently than every 24 hours.  At that
point, rather than send two separate requests for update checks and pings with
two different timers, we combined them into a single request.  Would it make
sense to split them back apart?

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:192:
}
On 2013/11/11 17:34:27, waffles wrote:
> Why require that a response that contains an install event may not contain an
> update response or ping response (or vice versa)?
> 
> i.e. if this happens (for whatever reason), is it worth throwing out the
> response (and retrying the request later)?

It's just the way our client works...  We don't try to send a request containing
an {install event} and {ping, updatecheck} simultaneously because of the install
age attribute.  Granted, if we did start sending everything simultaneously we'd
probably avoid issues where the install events block ping events from going
through.  Which would make more sense?

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:205:
success &= TextUtils.equals("3.0", node.attributes.get("protocol"));
On 2013/11/11 17:43:41, nyquist (GMT 11.4-11.18) wrote:
> On 2013/11/08 22:05:32, dfalcantara wrote:
> > How long will server protocol 3.0 be handled for?
> 
> And also, what is the expected client behavior when it will not work
anymore...?

Could we just assume that the server got the request fine instead of failing? 
The requests we send do explicitly state that we used protocol 3.0.

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java:15:
"https://market.android.com/details?id=com.google.android.apps.chrome";
On 2013/11/11 17:43:41, nyquist (GMT 11.4-11.18) wrote:
> Is this still the URL we want?
> Could we use https://play.google.com/store/apps/details?id=%5Bsome package]

Done.

[waffles, 2013-11-11 18:48:40]

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:192:
}
On 2013/11/11 18:22:34, dfalcantara wrote:
> On 2013/11/11 17:34:27, waffles wrote:
> > Why require that a response that contains an install event may not contain
an
> > update response or ping response (or vice versa)?
> > 
> > i.e. if this happens (for whatever reason), is it worth throwing out the
> > response (and retrying the request later)?
> 
> It's just the way our client works...  We don't try to send a request
containing
> an {install event} and {ping, updatecheck} simultaneously because of the
install
> age attribute.  Granted, if we did start sending everything simultaneously
we'd
> probably avoid issues where the install events block ping events from going
> through.  Which would make more sense?

From the server side, it is equivalent to bundle these three together, using an
installage of "-1".

Whether you decide to make that change in the clients or not, though, it seems
like the "contains (install) or (ping and updatecheck) but not both" should be
an expectation on the request, not the response.

https://chromiumcodereview.appspot.com/67313003/diff/1/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:205:
success &= TextUtils.equals("3.0", node.attributes.get("protocol"));
On 2013/11/11 18:22:34, dfalcantara wrote:
> On 2013/11/11 17:43:41, nyquist (GMT 11.4-11.18) wrote:
> > On 2013/11/08 22:05:32, dfalcantara wrote:
> > > How long will server protocol 3.0 be handled for?
> > 
> > And also, what is the expected client behavior when it will not work
> anymore...?
> 
> Could we just assume that the server got the request fine instead of failing? 
> The requests we send do explicitly state that we used protocol 3.0.

The server is obligated to respond in protocol 3.0 when the client's request is
3.0.

[stuartmorgan, 2013-11-11 19:39:24]

On 2013/11/11 18:48:40, waffles wrote:
> From the server side, it is equivalent to bundle these three together, using
an
> installage of "-1".

How do you reconcile that with the fact that the requirements for version and
nextversion differ for install and for ping? We used to send everything at once
on iOS and were told to break it apart because of the version stuff.

[waffles, 2013-11-11 20:56:02]

On 2013/11/11 19:39:24, stuartmorgan wrote:
> On 2013/11/11 18:48:40, waffles wrote:
> > From the server side, it is equivalent to bundle these three together, using
> an
> > installage of "-1".
> 
> How do you reconcile that with the fact that the requirements for version and
> nextversion differ for install and for ping? We used to send everything at
once
> on iOS and were told to break it apart because of the version stuff.

I'm surprised at that. Can you forward/link me the discussion?


The old contract used to be:
* "<app version>" is the current version of the app.
* "<app nextversion>" is the version that the app will be after the operation
that drove the <event> completes.

So it seems like nextversion == version for all <events> that iOS / Android
would send, since the install ping is always sent after the install is
completed. Hm, I see there is a note in the protocol that nextversion is "" for
an initial updatecheck. Maybe that's the source of the problem?

Anyways, I'm of the opinion that iOS / Android should only ever send "version"
and never send "nextversion", unless I'm mistaken about what kind of <event>s
get sent - but it looks like only update complete and install complete events.
If we can't do that, I'd prefer to switch to the below newer scheme; but it's
not a big deal if we want to keep these requests separate.

----------------
There is a newer scheme for these fields:

We had problems with the older definition having "event" information at the
<app> level, so we came up with:

* "<app version>" is the current version of the app at the time the request was
sent.
* "<event nextversion>" is the version that the app after the operation that
drove this <event> completes.
* "<event previousversion>" is the version that the app before the operation
that drove this <event> began.
* "<app nextversion>" is unused in this scheme.

[stuartmorgan, 2013-11-11 22:11:29]

On 2013/11/11 20:56:02, waffles wrote:
> I'm surprised at that. Can you forward/link me the discussion?

Not without a bunch of email archeology.

In practice, it's been really hard to get a working implementation of the
client. The documentation
('https://code.google.com/p/omaha/wiki/ServerProtocol') is not always complete,
and IIRC hasn't always matched what we have been told about what we should
actually do. iOS had to go through a number of iterations before things actually
worked correctly, and in most cases we ended up having to just do what desktop
and Android do, because we know that works. Given that, I'm pretty reluctant to
make a change to the format here unless everyone is very, very sure that it's
the right thing to do. Desktop does *not* send a combined install/update + ping
(because different processes send them), and what I was told desktop does with
version/nextversion is not consistent with combining them. Is having two
different, inconsistent streams between desktop and mobile absolutely what you
want? Have you tested that the whole pipeline definitely works with a combined
ping?

> The old contract used to be:
> * "<app version>" is the current version of the app.

But "current version" was supposed to mean "the pre-update version", based on
what I was told. This makes sense for an installer process where when it starts,
"current version" *is* "old version", but for Android/iOS, current version is
the post-update version. So the ping stream for desktop would be:
  event: version=x, nextversion=x+1
  ping: version=x+1 nextversion=""

If we combine them on mobile, then exactly the same versions would be either:
  event=ping: version=x+1 nextversion=x+1
or
  event=ping: version=x+1 nextversion=""
Neither of which is the same as what's done on desktop, or what's done on mobile
now.

> Anyways, I'm of the opinion that iOS / Android should only ever send "version"
> and never send "nextversion",

Is that an opinion shared by the entire Omaha pipeline implementation? I cannot
stress enough how much I do not want to break, and thus debug, the entire Omaha
reporting chain yet again.

[waffles, 2013-11-11 22:29:27]

On 2013/11/11 22:11:29, stuartmorgan wrote:
> On 2013/11/11 20:56:02, waffles wrote:
> > I'm surprised at that. Can you forward/link me the discussion?
> 
> Not without a bunch of email archeology.
> 
> In practice, it's been really hard to get a working implementation of the
> client. The documentation
> ('https://code.google.com/p/omaha/wiki/ServerProtocol') is not always
complete,
> and IIRC hasn't always matched what we have been told about what we should
> actually do. iOS had to go through a number of iterations before things
actually
> worked correctly, and in most cases we ended up having to just do what desktop
> and Android do, because we know that works. Given that, I'm pretty reluctant
to
> make a change to the format here unless everyone is very, very sure that it's
> the right thing to do. Desktop does *not* send a combined install/update +
ping
> (because different processes send them), and what I was told desktop does with
> version/nextversion is not consistent with combining them. Is having two
> different, inconsistent streams between desktop and mobile absolutely what you
> want? Have you tested that the whole pipeline definitely works with a combined
> ping?
> 

Like I said, it is not a big deal to keep the pings separate. I just feel like
parsing the response is the wrong place to enforce that they are separate.

I agree that the protocol documentation is challenging and mixes normative and
informative sections with wild abandon. It's also plagued by some interesting
choices and some evolutions that have led to an inconsistent view of what is and
what ought to be. I would really like to have a specific and prescriptive
document.

I am OK with not combining the pings, and continuing to send version and
nextversion as they are being sent today. I agree that this is the minimum-risk
option.

> > The old contract used to be:
> > * "<app version>" is the current version of the app.
> 
> But "current version" was supposed to mean "the pre-update version", based on
> what I was told. This makes sense for an installer process where when it
starts,
> "current version" *is* "old version", but for Android/iOS, current version is
> the post-update version. So the ping stream for desktop would be:
>   event: version=x, nextversion=x+1
>   ping: version=x+1 nextversion=""
> 
> If we combine them on mobile, then exactly the same versions would be either:
>   event=ping: version=x+1 nextversion=x+1
> or
>   event=ping: version=x+1 nextversion=""
> Neither of which is the same as what's done on desktop, or what's done on
mobile
> now.
> 

Hm, as you pointed out, what you were told does not match the protocol
definition. I'll hunt around the source code and get back to you.

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

On 2013/11/11 22:29:27, waffles wrote:
> On 2013/11/11 22:11:29, stuartmorgan wrote:
> > On 2013/11/11 20:56:02, waffles wrote:
> > > I'm surprised at that. Can you forward/link me the discussion?
> > 
> > Not without a bunch of email archeology.
> > 
> > In practice, it's been really hard to get a working implementation of the
> > client. The documentation
> > ('https://code.google.com/p/omaha/wiki/ServerProtocol') is not always
> complete,
> > and IIRC hasn't always matched what we have been told about what we should
> > actually do. iOS had to go through a number of iterations before things
> actually
> > worked correctly, and in most cases we ended up having to just do what
desktop
> > and Android do, because we know that works. Given that, I'm pretty reluctant
> to
> > make a change to the format here unless everyone is very, very sure that
it's
> > the right thing to do. Desktop does *not* send a combined install/update +
> ping
> > (because different processes send them), and what I was told desktop does
with
> > version/nextversion is not consistent with combining them. Is having two
> > different, inconsistent streams between desktop and mobile absolutely what
you
> > want? Have you tested that the whole pipeline definitely works with a
combined
> > ping?
> > 
> 
> Like I said, it is not a big deal to keep the pings separate. I just feel like
> parsing the response is the wrong place to enforce that they are separate.
> 
> I agree that the protocol documentation is challenging and mixes normative and
> informative sections with wild abandon. It's also plagued by some interesting
> choices and some evolutions that have led to an inconsistent view of what is
and
> what ought to be. I would really like to have a specific and prescriptive
> document.
> 
> I am OK with not combining the pings, and continuing to send version and
> nextversion as they are being sent today. I agree that this is the
minimum-risk
> option.
> 
> > > The old contract used to be:
> > > * "<app version>" is the current version of the app.
> > 
> > But "current version" was supposed to mean "the pre-update version", based
on
> > what I was told. This makes sense for an installer process where when it
> starts,
> > "current version" *is* "old version", but for Android/iOS, current version
is
> > the post-update version. So the ping stream for desktop would be:
> >   event: version=x, nextversion=x+1
> >   ping: version=x+1 nextversion=""
> > 
> > If we combine them on mobile, then exactly the same versions would be
either:
> >   event=ping: version=x+1 nextversion=x+1
> > or
> >   event=ping: version=x+1 nextversion=""
> > Neither of which is the same as what's done on desktop, or what's done on
> mobile
> > now.
> > 
> 
> Hm, as you pointed out, what you were told does not match the protocol
> definition. I'll hunt around the source code and get back to you.

:( You were right to object.

There's only two pieces of app version information that the aggregation pipeline
actually uses: App.Version, and App.Event.PreviousVersion. We also log (but do
not aggregate) App.Event.NextVersion.

However, it turns out that the server (when provided with both "version" and
"nextversion"), logs "nextversion" as the App.Version and "version" as the
Event.PreviousVersion of every <event> of that app.

Event.PreviousVersion is overridden with the value of <event previousversion>.

We can leave it the way it is, to mirror desktop more exactly.

For completeness, the alternative would be:
[Combined Event & Update Check]
<app version> is x+1         --> App.Version is x+1.
<event previousversion> is x --> App.Event.PreviousVersion is x.
<event nextversion> is x+1   --> App.Event.NextVersion ix x+1.

This matches the existing output, which is
[Event]
<app version> is x           --> App.Event.PreviousVersion is x.
<app nextversion> is x+1     --> App.Version is x+1.

[Update Check]
<app version> is x+1         --> App.Version is x+1.
<app nextversion> is ""      _/

But this will only save us one request per install/update per client. It might
not be worth the work, even setting the risk aside.

[gone, 2013-11-12 21:38:48]

On 2013/11/11 22:59:59, waffles wrote:
> On 2013/11/11 22:29:27, waffles wrote:
> > On 2013/11/11 22:11:29, stuartmorgan wrote:
> > > On 2013/11/11 20:56:02, waffles wrote:
> > > > I'm surprised at that. Can you forward/link me the discussion?
> > > 
> > > Not without a bunch of email archeology.
> > > 
> > > In practice, it's been really hard to get a working implementation of the
> > > client. The documentation
> > > ('https://code.google.com/p/omaha/wiki/ServerProtocol') is not always
> > complete,
> > > and IIRC hasn't always matched what we have been told about what we should
> > > actually do. iOS had to go through a number of iterations before things
> > actually
> > > worked correctly, and in most cases we ended up having to just do what
> desktop
> > > and Android do, because we know that works. Given that, I'm pretty
reluctant
> > to
> > > make a change to the format here unless everyone is very, very sure that
> it's
> > > the right thing to do. Desktop does *not* send a combined install/update +
> > ping
> > > (because different processes send them), and what I was told desktop does
> with
> > > version/nextversion is not consistent with combining them. Is having two
> > > different, inconsistent streams between desktop and mobile absolutely what
> you
> > > want? Have you tested that the whole pipeline definitely works with a
> combined
> > > ping?
> > > 
> > 
> > Like I said, it is not a big deal to keep the pings separate. I just feel
like
> > parsing the response is the wrong place to enforce that they are separate.
> > 
> > I agree that the protocol documentation is challenging and mixes normative
and
> > informative sections with wild abandon. It's also plagued by some
interesting
> > choices and some evolutions that have led to an inconsistent view of what is
> and
> > what ought to be. I would really like to have a specific and prescriptive
> > document.
> > 
> > I am OK with not combining the pings, and continuing to send version and
> > nextversion as they are being sent today. I agree that this is the
> minimum-risk
> > option.
> > 
> > > > The old contract used to be:
> > > > * "<app version>" is the current version of the app.
> > > 
> > > But "current version" was supposed to mean "the pre-update version", based
> on
> > > what I was told. This makes sense for an installer process where when it
> > starts,
> > > "current version" *is* "old version", but for Android/iOS, current version
> is
> > > the post-update version. So the ping stream for desktop would be:
> > >   event: version=x, nextversion=x+1
> > >   ping: version=x+1 nextversion=""
> > > 
> > > If we combine them on mobile, then exactly the same versions would be
> either:
> > >   event=ping: version=x+1 nextversion=x+1
> > > or
> > >   event=ping: version=x+1 nextversion=""
> > > Neither of which is the same as what's done on desktop, or what's done on
> > mobile
> > > now.
> > > 
> > 
> > Hm, as you pointed out, what you were told does not match the protocol
> > definition. I'll hunt around the source code and get back to you.
> 
> :( You were right to object.
> 
> There's only two pieces of app version information that the aggregation
pipeline
> actually uses: App.Version, and App.Event.PreviousVersion. We also log (but do
> not aggregate) App.Event.NextVersion.
> 
> However, it turns out that the server (when provided with both "version" and
> "nextversion"), logs "nextversion" as the App.Version and "version" as the
> Event.PreviousVersion of every <event> of that app.
> 
> Event.PreviousVersion is overridden with the value of <event previousversion>.
> 
> We can leave it the way it is, to mirror desktop more exactly.
> 
> For completeness, the alternative would be:
> [Combined Event & Update Check]
> <app version> is x+1         --> App.Version is x+1.
> <event previousversion> is x --> App.Event.PreviousVersion is x.
> <event nextversion> is x+1   --> App.Event.NextVersion ix x+1.
> 
> This matches the existing output, which is
> [Event]
> <app version> is x           --> App.Event.PreviousVersion is x.
> <app nextversion> is x+1     --> App.Version is x+1.
> 
> [Update Check]
> <app version> is x+1         --> App.Version is x+1.
> <app nextversion> is ""      _/
> 
> But this will only save us one request per install/update per client. It might
> not be worth the work, even setting the risk aside.

Hrm, with all that said, what needs to be done for this CL to proceed?  Chrome
on Android doesn't report the nextversion tag currently, but that's sent as part
of the request instead of the response.  That will eventually come when I find
time to work on https://chromiumcodereview.appspot.com/19684004/ again.

Should we add unit tests and support for weird cases with multiple response or
app tags (e.g.), neither of which should happen on Android?

[waffles, 2013-11-12 21:48:40]

On 2013/11/12 21:38:48, dfalcantara wrote:
> On 2013/11/11 22:59:59, waffles wrote:
> > On 2013/11/11 22:29:27, waffles wrote:
> > > On 2013/11/11 22:11:29, stuartmorgan wrote:
> > > > On 2013/11/11 20:56:02, waffles wrote:
> > > > > I'm surprised at that. Can you forward/link me the discussion?
> > > > 
> > > > Not without a bunch of email archeology.
> > > > 
> > > > In practice, it's been really hard to get a working implementation of
the
> > > > client. The documentation
> > > > ('https://code.google.com/p/omaha/wiki/ServerProtocol') is not always
> > > complete,
> > > > and IIRC hasn't always matched what we have been told about what we
should
> > > > actually do. iOS had to go through a number of iterations before things
> > > actually
> > > > worked correctly, and in most cases we ended up having to just do what
> > desktop
> > > > and Android do, because we know that works. Given that, I'm pretty
> reluctant
> > > to
> > > > make a change to the format here unless everyone is very, very sure that
> > it's
> > > > the right thing to do. Desktop does *not* send a combined install/update
+
> > > ping
> > > > (because different processes send them), and what I was told desktop
does
> > with
> > > > version/nextversion is not consistent with combining them. Is having two
> > > > different, inconsistent streams between desktop and mobile absolutely
what
> > you
> > > > want? Have you tested that the whole pipeline definitely works with a
> > combined
> > > > ping?
> > > > 
> > > 
> > > Like I said, it is not a big deal to keep the pings separate. I just feel
> like
> > > parsing the response is the wrong place to enforce that they are separate.
> > > 
> > > I agree that the protocol documentation is challenging and mixes normative
> and
> > > informative sections with wild abandon. It's also plagued by some
> interesting
> > > choices and some evolutions that have led to an inconsistent view of what
is
> > and
> > > what ought to be. I would really like to have a specific and prescriptive
> > > document.
> > > 
> > > I am OK with not combining the pings, and continuing to send version and
> > > nextversion as they are being sent today. I agree that this is the
> > minimum-risk
> > > option.
> > > 
> > > > > The old contract used to be:
> > > > > * "<app version>" is the current version of the app.
> > > > 
> > > > But "current version" was supposed to mean "the pre-update version",
based
> > on
> > > > what I was told. This makes sense for an installer process where when it
> > > starts,
> > > > "current version" *is* "old version", but for Android/iOS, current
version
> > is
> > > > the post-update version. So the ping stream for desktop would be:
> > > >   event: version=x, nextversion=x+1
> > > >   ping: version=x+1 nextversion=""
> > > > 
> > > > If we combine them on mobile, then exactly the same versions would be
> > either:
> > > >   event=ping: version=x+1 nextversion=x+1
> > > > or
> > > >   event=ping: version=x+1 nextversion=""
> > > > Neither of which is the same as what's done on desktop, or what's done
on
> > > mobile
> > > > now.
> > > > 
> > > 
> > > Hm, as you pointed out, what you were told does not match the protocol
> > > definition. I'll hunt around the source code and get back to you.
> > 
> > :( You were right to object.
> > 
> > There's only two pieces of app version information that the aggregation
> pipeline
> > actually uses: App.Version, and App.Event.PreviousVersion. We also log (but
do
> > not aggregate) App.Event.NextVersion.
> > 
> > However, it turns out that the server (when provided with both "version" and
> > "nextversion"), logs "nextversion" as the App.Version and "version" as the
> > Event.PreviousVersion of every <event> of that app.
> > 
> > Event.PreviousVersion is overridden with the value of <event
previousversion>.
> > 
> > We can leave it the way it is, to mirror desktop more exactly.
> > 
> > For completeness, the alternative would be:
> > [Combined Event & Update Check]
> > <app version> is x+1         --> App.Version is x+1.
> > <event previousversion> is x --> App.Event.PreviousVersion is x.
> > <event nextversion> is x+1   --> App.Event.NextVersion ix x+1.
> > 
> > This matches the existing output, which is
> > [Event]
> > <app version> is x           --> App.Event.PreviousVersion is x.
> > <app nextversion> is x+1     --> App.Version is x+1.
> > 
> > [Update Check]
> > <app version> is x+1         --> App.Version is x+1.
> > <app nextversion> is ""      _/
> > 
> > But this will only save us one request per install/update per client. It
might
> > not be worth the work, even setting the risk aside.
> 
> Hrm, with all that said, what needs to be done for this CL to proceed?  Chrome
> on Android doesn't report the nextversion tag currently, but that's sent as
part
> of the request instead of the response.  That will eventually come when I find
> time to work on https://chromiumcodereview.appspot.com/19684004/ again.
> 
> Should we add unit tests and support for weird cases with multiple response or
> app tags (e.g.), neither of which should happen on Android?

I think the best way to proceed is to remove the restriction on the response,
and add unit tests that ensure that the request (not response) doesn't contain
both an <event> and a <ping>/<updatecheck>.

Re: multiple app tags, etc.: Unit tests that show that the parser does not fail
when given a response that uses other (currently unused) features of the
protocol would be nice, but they seem like they have questionable value.

[gone, 2014-01-03 01:54:57]

On 2013/11/12 21:48:40, waffles wrote:
> On 2013/11/12 21:38:48, dfalcantara wrote:
> > On 2013/11/11 22:59:59, waffles wrote:
> > > On 2013/11/11 22:29:27, waffles wrote:
> > > > On 2013/11/11 22:11:29, stuartmorgan wrote:
> > > > > On 2013/11/11 20:56:02, waffles wrote:
> > > > > > I'm surprised at that. Can you forward/link me the discussion?
> > > > > 
> > > > > Not without a bunch of email archeology.
> > > > > 
> > > > > In practice, it's been really hard to get a working implementation of
> the
> > > > > client. The documentation
> > > > > ('https://code.google.com/p/omaha/wiki/ServerProtocol') is not always
> > > > complete,
> > > > > and IIRC hasn't always matched what we have been told about what we
> should
> > > > > actually do. iOS had to go through a number of iterations before
things
> > > > actually
> > > > > worked correctly, and in most cases we ended up having to just do what
> > > desktop
> > > > > and Android do, because we know that works. Given that, I'm pretty
> > reluctant
> > > > to
> > > > > make a change to the format here unless everyone is very, very sure
that
> > > it's
> > > > > the right thing to do. Desktop does *not* send a combined
install/update
> +
> > > > ping
> > > > > (because different processes send them), and what I was told desktop
> does
> > > with
> > > > > version/nextversion is not consistent with combining them. Is having
two
> > > > > different, inconsistent streams between desktop and mobile absolutely
> what
> > > you
> > > > > want? Have you tested that the whole pipeline definitely works with a
> > > combined
> > > > > ping?
> > > > > 
> > > > 
> > > > Like I said, it is not a big deal to keep the pings separate. I just
feel
> > like
> > > > parsing the response is the wrong place to enforce that they are
separate.
> > > > 
> > > > I agree that the protocol documentation is challenging and mixes
normative
> > and
> > > > informative sections with wild abandon. It's also plagued by some
> > interesting
> > > > choices and some evolutions that have led to an inconsistent view of
what
> is
> > > and
> > > > what ought to be. I would really like to have a specific and
prescriptive
> > > > document.
> > > > 
> > > > I am OK with not combining the pings, and continuing to send version and
> > > > nextversion as they are being sent today. I agree that this is the
> > > minimum-risk
> > > > option.
> > > > 
> > > > > > The old contract used to be:
> > > > > > * "<app version>" is the current version of the app.
> > > > > 
> > > > > But "current version" was supposed to mean "the pre-update version",
> based
> > > on
> > > > > what I was told. This makes sense for an installer process where when
it
> > > > starts,
> > > > > "current version" *is* "old version", but for Android/iOS, current
> version
> > > is
> > > > > the post-update version. So the ping stream for desktop would be:
> > > > >   event: version=x, nextversion=x+1
> > > > >   ping: version=x+1 nextversion=""
> > > > > 
> > > > > If we combine them on mobile, then exactly the same versions would be
> > > either:
> > > > >   event=ping: version=x+1 nextversion=x+1
> > > > > or
> > > > >   event=ping: version=x+1 nextversion=""
> > > > > Neither of which is the same as what's done on desktop, or what's done
> on
> > > > mobile
> > > > > now.
> > > > > 
> > > > 
> > > > Hm, as you pointed out, what you were told does not match the protocol
> > > > definition. I'll hunt around the source code and get back to you.
> > > 
> > > :( You were right to object.
> > > 
> > > There's only two pieces of app version information that the aggregation
> > pipeline
> > > actually uses: App.Version, and App.Event.PreviousVersion. We also log
(but
> do
> > > not aggregate) App.Event.NextVersion.
> > > 
> > > However, it turns out that the server (when provided with both "version"
and
> > > "nextversion"), logs "nextversion" as the App.Version and "version" as the
> > > Event.PreviousVersion of every <event> of that app.
> > > 
> > > Event.PreviousVersion is overridden with the value of <event
> previousversion>.
> > > 
> > > We can leave it the way it is, to mirror desktop more exactly.
> > > 
> > > For completeness, the alternative would be:
> > > [Combined Event & Update Check]
> > > <app version> is x+1         --> App.Version is x+1.
> > > <event previousversion> is x --> App.Event.PreviousVersion is x.
> > > <event nextversion> is x+1   --> App.Event.NextVersion ix x+1.
> > > 
> > > This matches the existing output, which is
> > > [Event]
> > > <app version> is x           --> App.Event.PreviousVersion is x.
> > > <app nextversion> is x+1     --> App.Version is x+1.
> > > 
> > > [Update Check]
> > > <app version> is x+1         --> App.Version is x+1.
> > > <app nextversion> is ""      _/
> > > 
> > > But this will only save us one request per install/update per client. It
> might
> > > not be worth the work, even setting the risk aside.
> > 
> > Hrm, with all that said, what needs to be done for this CL to proceed? 
Chrome
> > on Android doesn't report the nextversion tag currently, but that's sent as
> part
> > of the request instead of the response.  That will eventually come when I
find
> > time to work on https://chromiumcodereview.appspot.com/19684004/ again.
> > 
> > Should we add unit tests and support for weird cases with multiple response
or
> > app tags (e.g.), neither of which should happen on Android?
> 
> I think the best way to proceed is to remove the restriction on the response,
> and add unit tests that ensure that the request (not response) doesn't contain
> both an <event> and a <ping>/<updatecheck>.
> 
> Re: multiple app tags, etc.: Unit tests that show that the parser does not
fail
> when given a response that uses other (currently unused) features of the
> protocol would be nice, but they seem like they have questionable value.

Unfortunately got distracted with other higher priority bugs again, but I made
time to revisit this.  I've split off the bit where I expected the tags to be
exclusive, but since the XML request generator isn't being upstreamed as part of
this CL, there's nothing to enforce the exclusivity.  Tests also got expanded a
bit to look for exactly when they fail during parsing.  Although I don't
normally like having such granularity, I've seen situations where downstream
tests would pass or fail in unexpected ways.  This should help with that...

[waffles, 2014-01-10 21:28:23]

lgtm as far as the handling of the protocol goes; 1 comment.

https://codereview.chromium.org/67313003/diff/340001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://codereview.chromium.org/67313003/diff/340001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:146:
success &= TextUtils.equals("prod", node.attributes.get("server"));
This means the client won't be able to use our test servers, or a local dev
server (unless it's configured to pretend it's "prod").

[gone, 2014-01-15 19:32:09]

Tommy?

https://chromiumcodereview.appspot.com/67313003/diff/340001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/340001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:146:
success &= TextUtils.equals("prod", node.attributes.get("server"));
On 2014/01/10 21:28:24, waffles wrote:
> This means the client won't be able to use our test servers, or a local dev
> server (unless it's configured to pretend it's "prod").

Removed that requirement, made it a logged warning if prod isn't encountered.

[nyquist, 2014-01-23 00:54:01]

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/RequestFailureException.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/RequestFailureException.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
How do you feel about changing all of these headers to 2014?

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:70:
public ResponseParser(String appId, boolean expectInstallEvent, boolean
expectPing,
Is this constructor ever used?

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:242:
if (success) {
This is absolutely always true. Did you mean to update success somewhere?

If not, you could just remove |success| and just set |mParsedUpdatecheck| to
true and return. No need to log error, since it will never happen.

I believe this happened between patch set 4 and 5 when you removed the
requirement for the |server| node.

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/omaha/XMLParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/XMLParser.java:83:
throw new SAXException("Tag stack is empty when it shouldn't be.");
single line?

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java:116:
throws IllegalArgumentException, IllegalStateException, IOException {
Both IllegalXxxException are unchecked right? So they shouldn't really be
declared as thrown.

[gone, 2014-01-24 07:43:11]

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/RequestFailureException.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/RequestFailureException.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/01/23 00:54:02, nyquist wrote:
> How do you feel about changing all of these headers to 2014?

Done.

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:70:
public ResponseParser(String appId, boolean expectInstallEvent, boolean
expectPing,
On 2014/01/23 00:54:02, nyquist wrote:
> Is this constructor ever used?

Upstream it is, yeah, but the only difference is that strictParsing bit.

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:242:
if (success) {
On 2014/01/23 00:54:02, nyquist wrote:
> This is absolutely always true. Did you mean to update success somewhere?
> 
> If not, you could just remove |success| and just set |mParsedUpdatecheck| to
> true and return. No need to log error, since it will never happen.
> 
> I believe this happened between patch set 4 and 5 when you removed the
> requirement for the |server| node.

Yeah, no longer necessary AFAICT.

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/omaha/XMLParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/XMLParser.java:83:
throw new SAXException("Tag stack is empty when it shouldn't be.");
On 2014/01/23 00:54:02, nyquist wrote:
> single line?

Done.

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/javatests/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java:116:
throws IllegalArgumentException, IllegalStateException, IOException {
On 2014/01/23 00:54:02, nyquist wrote:
> Both IllegalXxxException are unchecked right? So they shouldn't really be
> declared as thrown.

Done.

[gone, 2014-01-24 07:43:51]

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java
(right):

https://chromiumcodereview.appspot.com/67313003/diff/410001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/omaha/ResponseParser.java:70:
public ResponseParser(String appId, boolean expectInstallEvent, boolean
expectPing,
On 2014/01/24 07:43:12, dfalcantara wrote:
> On 2014/01/23 00:54:02, nyquist wrote:
> > Is this constructor ever used?
> 
> Upstream it is, yeah, but the only difference is that strictParsing bit.

Er, downstream.  Bleh.

[nyquist, 2014-01-28 20:02:04]

lgtm

[commit-bot: I haz the power, 2014-01-28 20:07:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/67313003/500001

[commit-bot: I haz the power, 2014-01-29 02:25:02]

Change committed as 247594