New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 134715 link

Starred by 4 users

Issue metadata

Status: Verified
Owner:
Email to this user bounced
Closed: Jun 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Commit's no longer send protocol version

Project Member Reported by zea@chromium.org, Jun 26 2012

Issue description

We need the protocol version properly set in order to determine whether the client supports certain features, such as per-datatype-throttling. It appears that we no longer use the same method of building the ClientToServerMessage in BuildAndPostCommits as we did previously, which would have taken care of setting the protocol version for us.
 
I was really surprised that this regressed.  I couldn't figure out how my changes could have affected this.  Then I found out where the version settings is normally done: syncproto.h.  

It turns out that I'm using a sync_pb::ClientToServerMessage rather than a csync::ClientToServerMessage.  I'll fix this, then look into removing syncproto.h so this never happens again.
I was too quick to blame syncproto.h.  It turns out that file is only partially related.  

It defines csync::ClientToServerMessage as follows:

class ClientToServerMessage : public sync_pb::ClientToServerMessage {
 public:
  ClientToServerMessage() {
    set_protocol_version(protocol_version());
  }
};

It's like a sync_pb::ClientToServerMessage in every way, except that its constructor cleverly sets the protocol version to the default protocol version.  This might seem like a no-op, but it's not.  It ensures that has_protocol_version() will return true.  

In commit.cc, we call Clear() on the message to ensure it is in a pristine state before filling it with data to be sent to the server.  This unsets the has_protocol_version() flag.  The server later reads this value through a method that returns 0 if !has_protocol_version().

======

I think there are several design decisions here that ought to be reconsidered.  However, beta channel is neither the time or place to be making design decisions.  I'll write a one line fix for now, and I'll try to come up with a better solution for M22.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 27 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=144339

------------------------------------------------------------------------
r144339 | rlarocque@chromium.org | Tue Jun 26 17:40:01 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/commit.cc?r1=144339&r2=144338&pathrev=144339
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/test/engine/mock_connection_manager.cc?r1=144339&r2=144338&pathrev=144339

sync: Set explicit protocol_version on commit

The use of Message::Clear() function in commit.cc was unsetting this
field.  This change explicitly sets the field to the current default
value following the Clear().  It also adds a test assertion to verify
that the field is properly set.

BUG= 134769 , 134715 
TEST=Run against the test server and verify the field is in the logs.

Review URL: https://chromiumcodereview.appspot.com/10679010
------------------------------------------------------------------------
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 27 2012

Labels: merge-merged-1180_11
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=144347

------------------------------------------------------------------------
r144347 | rlarocque@chromium.org | Tue Jun 26 18:10:03 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1180_11/src/sync/engine/commit.cc?r1=144347&r2=144346&pathrev=144347
 M http://src.chromium.org/viewvc/chrome/branches/1180_11/src/sync/test/engine/mock_connection_manager.cc?r1=144347&r2=144346&pathrev=144347

Merge 144339 - sync: Set explicit protocol_version on commit

The use of Message::Clear() function in commit.cc was unsetting this
field.  This change explicitly sets the field to the current default
value following the Clear().  It also adds a test assertion to verify
that the field is properly set.

BUG= 134769 , 134715 
TEST=Run against the test server and verify the field is in the logs.

Review URL: https://chromiumcodereview.appspot.com/10679010

TBR=rlarocque@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10687008
------------------------------------------------------------------------
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 27 2012

Labels: merge-merged-1180
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=144348

------------------------------------------------------------------------
r144348 | rlarocque@chromium.org | Tue Jun 26 18:11:30 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/sync/engine/commit.cc?r1=144348&r2=144347&pathrev=144348
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/sync/test/engine/mock_connection_manager.cc?r1=144348&r2=144347&pathrev=144348

Merge 144339 - sync: Set explicit protocol_version on commit

The use of Message::Clear() function in commit.cc was unsetting this
field.  This change explicitly sets the field to the current default
value following the Clear().  It also adds a test assertion to verify
that the field is properly set.

BUG= 134769 , 134715 
TEST=Run against the test server and verify the field is in the logs.

Review URL: https://chromiumcodereview.appspot.com/10679010

TBR=rlarocque@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10681009
------------------------------------------------------------------------

Comment 6 by kareng@google.com, Jun 27 2012

Status: Fixed
Labels: VerfiiedIn-M21
Owner: annapop@chromium.org
Status: Verified
Verified fix, build 21.0.1180.15.
Closing.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 12 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=146393

------------------------------------------------------------------------
r146393 | rlarocque@chromium.org | Thu Jul 12 11:15:45 PDT 2012

Changed paths:
 A http://src.chromium.org/viewvc/chrome/trunk/src/sync/syncable/syncable_proto_util.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/syncable/model_type.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncer_util.cc?r1=146393&r2=146392&pathrev=146393
 A http://src.chromium.org/viewvc/chrome/trunk/src/sync/syncable/syncable_proto_util.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/syncable/syncable_unittest.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/commit.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/sync.gyp?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/sessions/session_state.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/verify_updates_command.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/process_commit_response_command_unittest.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/process_commit_response_command.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/process_commit_response_command.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncer.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/sessions/status_controller.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncer_proto_util_unittest.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/build_commit_command_unittest.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/build_commit_command.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/test/engine/mock_connection_manager.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncer_proto_util.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/syncable/DEPS?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/download_updates_command.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncer_util.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/net/server_connection_manager.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/build_commit_command.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncer_proto_util.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/sessions/status_controller_unittest.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncer_unittest.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncer.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/sessions/status_controller.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/process_updates_command.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/verify_updates_command_unittest.cc?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/verify_updates_command.h?r1=146393&r2=146392&pathrev=146393
 D http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncproto_unittest.cc?r1=146393&r2=146392&pathrev=146393
 D http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/syncproto.h?r1=146393&r2=146392&pathrev=146393
 M http://src.chromium.org/viewvc/chrome/trunk/src/sync/engine/store_timestamps_command.cc?r1=146393&r2=146392&pathrev=146393

Remove syncproto.h

Replace sync/engine/syncproto.h with sync/syncable/syncable_proto_util.h and
.cc.  The tasks that used to be performed by member functions of the syncer::
proto wrapper classes are now handled by static member functions.

Unfortunately, serialization and de-serialization of syncable::Id to/from proto
fields has gotten a bit uglier.  On the other hand, it's now much less magical
and mysterious.

The test intended to prevent regressions of  crbug.com/134715  has been replaced
with a DCHECK.  We'll have to rely on it to ensure that the protocol_version
field is always explicitly set.

BUG= 136454 
TEST=


Review URL: https://chromiumcodereview.appspot.com/10735041
------------------------------------------------------------------------
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-Internals -Feature-Sync -Mstone-21 Cr-Services-Sync M-21 Cr-Internals
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Sign in to add a comment