My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 14734: Cookies never deleted due to Win/POSIX epoch difference
9 people starred this issue and may be notified of changes. Back to list
 
Reported by mahan...@gmail.com, Jun 19, 2009
Chrome Version       : 3.0.189.0 (Official Build 18569)
OS + version : Ubuntu 8.04
CPU architecture (32-bit / 64-bit): 64
window manager : Gnome
URLs (if applicable) : http://twitter.com/
Behavior in Firefox 3.x (if applicable): works
Behavior in Chrome for Windows (optional): works

What steps will reproduce the problem?
1. Login to twitter.com
2. Logout
3. Try to login again (same or different username, no difference)

What is the expected result?
It logs in

What happens instead?
redirects to https://twitter.com/sessions and shows
"403 Forbidden: The server understood the request, but is refusing to 
fulfill it."

Please provide any additional information below. Attach a screenshot
and backtrace if possible.
It is reproducible always: first login after starting the browser works, 
logout and relogin doesn't. Closing all windows, restarting works for one 
login again. No problems on Windows.
Jun 22, 2009
#1 evan@chromium.org
I don't have a twitter account so I can't confirm this.
Status: Available
Cc: willc...@chromium.org
Labels: Pri-1
Jun 23, 2009
#2 willchan@chromium.org
Confirmed, I was able to repro this.  I've got other stuff on my plate, so I'm leaving 
this Available for now in case someone else can get to it first.
Jun 25, 2009
#3 evan@chromium.org
(No comment was entered for this change.)
Labels: Mstone-LinuxBeta
Jun 25, 2009
#4 willchan@chromium.org
I started looking into this.  I suspect the issue has to do with the cookies.  The 
Cookie: headers we send out for chrome/windows and chrome/linux are different.  We 
are setting extra empty cookie values in chrome/linux.  It seems like when twitter is 
responding with "Set-Cookie: _twitter_sess=; path=/; ..." we don't delete 
_twitter_sess.  The difference between the first login and the second login in 
chrome/linux is that the first login's Cookie: header only contains the _twitter_sess 
cookie once, whereas in the second login, it has it twice (the first of which is 
_twitter_sess=;) which twitter presumably doesn't like.

I'm adding deanm to see if he can explain the different cookie behavior.  I'll take a 
look at this further tomorrow.
Cc: de...@chromium.org
Jun 26, 2009
#5 de...@chromium.org
I can't imagine any reason the cookie code would be different between platforms.  If I 
could guess the problem could be:

- Cookie importing, but I don't think we do this anymore.
- Twitter is doing something different based on our user-agent.
- There is some tiny cross-platform difference, but I would find that really strange.
Jun 26, 2009
#6 willchan@chromium.org
Hm, I don't know if it's for all cookies or just the code path in this case, but cookies on Linux (and probably Mac too) 
aren't getting expired.  I confirmed that this bug repros on my dev channel mac chrome release.

So, when one signs out of twitter, the browser gets headers like so:
Set-Cookie: _twitter_sess=; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT
...
Set-Cookie: 
_twitter_sess=BAh7DDoJdXNlcjA6DnJldHVybl90bzA6DGNzcmZfaWQiJTM5Mzk0MTI0ZjJl%250AZTA0ZWJmZDc2YTBhMWY0MzhkODdiOhNwYXNzd29yZF
90b2tlbjAiCmZsYXNo%250ASUM6J0FjdGlvbkNvbnRyb2xsZXI6OkZsYXNoOjpGbGFzaEhhc2h7AAY6CkB1%250Ac2VkewA6B2lkIiUwNTAwMGMzMjVhMTY5M
zRjMjI1ZmQ4NDk1NjAzMmI1ZDoN%250AYWRtaW5faWQw--c48da353cc7d28f0bc57f9825b7f98fdb788e796; domain=.twitter.com; path=/

The first is on twitter.com and has an expires set to the year 1970 to delete the cookie.  The second is on .twitter.com 
and does not have expires set.  Our posix time code contains a bug that causes the twitter.com _twitter_sess cookie _not_ 
to get deleted.  When one tries to sign in again within the same browser session, the _twitter_sess cookie for both 
twitter.com and .twitter.com get sent to twitter.com, which chokes on this (most likely it just reads the first one with 
the empty value) and returns a 403 Forbidden.

CookieMonster::ParseCookieTime() parses the expiry time correctly, but then passes the Time::Exploded object to 
Time::FromUTCExploded() which returns Time(0).  Apparently this is because time_posix.cc:68 has this line:

seconds = timegm(&timestruct)

which sets seconds to 0.  I'm not sure why timegm() is doing that, but that seems broken, and is causing the 
CookieMonster code to think there is no expiry time, so cookies don't expire which is unfortunate.
Summary: twitter relogin doesn't work on Linux&Mac
Status: Started
Labels: -Area-Misc Area-BrowserBackend OS-Mac
Jun 26, 2009
#7 de...@chromium.org
The time should be zero, since it is zero seconds from epoch.  On Windows epoch isn't
1970 so that's probably why this doesn't happen there.  It's unfortunate that we use
a 0 epoch value as Time IsNull.  Maybe wtc or brett have thoughts on this.
Cc: w...@chromium.org bre...@chromium.org
Jun 26, 2009
#8 willchan@chromium.org
Haha, duh.  Thanks for pointing that out Dean :)  Maybe we should initialize us_ to -
1?  I wonder if that'd be likely to break something.
Jun 29, 2009
#9 willchan@chromium.org
(No comment was entered for this change.)
Owner: willc...@chromium.org
Jun 29, 2009
#10 willchan@chromium.org
I took a look at switching to initializing Time::us_ to -1 instead of 0, but it looks 
like it'd break a lot of functionality (it definitely breaks some tests).  Anyone else 
want to comment on an appropriate strategy for fixing this?
Cc: da...@chromium.org
Jun 29, 2009
#11 brettw@chromium.org
It looks like Time on Linux uses a different epoch than on Windows? This is very bad! 
We should be using the same epoch on all systems or profiles can't be shared.
Jun 29, 2009
#12 willchan@chromium.org
What's your suggestion?  Switch the Mac/Linux code to use the Windows epoch?  Vice 
versa?
Regardless, it seems like we leave open the possibility of bugs by using a 0 epoch 
value as Time::is_null().  Perhaps if we didn't use the unix epoch, the probability of 
hitting the bug would be very small.
Jun 29, 2009
#13 brettw@chromium.org
It seemed like hitting the Windows epoch would be impossible, so 0 seemed perfectly 
fine to me when I wrote it.

I think Linux & Mac should be switched to the Windows epoch. We need to write 
conversion functions for the times written out in all databases: History, cookies, and 
all the full text index databases (we could also delete the FTS files).
Jun 29, 2009
#14 willchan@chromium.org
Is your suggestion to have a bit in the profile to indicate whether it's using the 
unix epoch or the windows epoch?  And for mac/linux, if we detect the profile was 
using the unix epoch, we convert all the times in the databases to the windows epoch?
Jun 29, 2009
#15 brettw@chromium.org
I think it will be per-database. There is database versioning for all of our databases. 
It would check the number to see if it's old. For Windows, updating to the newest 
version is a NOP, on Mac & Linux it would convert the times.
Jun 30, 2009
#16 de...@chromium.org
I can look into the Time epoch part, I'll see what it takes to make our time code use 
the Windows epoch.  I am guessing it will be a bit of a nightmare.

As for the profiles, I would just let me be messed up, or someone else can write all 
the database upgrading code.
Jun 30, 2009
#17 willchan@chromium.org
Dean's got a changelist out for review to fix this.
Owner: de...@chromium.org
Jul 16, 2009
#18 de...@chromium.org
I am stuck on the whole database upgrading thing.  We also have non-sql places we 
might stick times (session restore?).  I say we just let everyone leap back 370 years, 
but I guess I'm just lazy...

What do we do?
Jul 16, 2009
#19 pinkerton@chromium.org
(No comment was entered for this change.)
Cc: pinker...@chromium.org
Labels: Mstone-MacBeta
Jul 16, 2009
#20 bre...@gmail.com
I probably wouldn't worry about the non-SQL cases. We should double-check bookmarks,
though.

For SQL, this will update the times in table "mytable" with a time column "mytime"
and a primary key "id":

UPDATE mytable SET mytime = mytime * 1000000 + 11644473600000000 WHERE id IN (SELECT
id FROM mytable WHERE mytime > 0);

Not updating history will cause the history to get archived, which will have the same
effect of deleting it as far as the user can see. It would be better to actually
delete it than have this happen.

I think we should update history. We've always updated history, even when we had 50
users.
Jul 16, 2009
#21 wtch...@gmail.com
Another option is to add a
  bool valid_;
member to the Time class to tell whether a us_ of 0
means a Time object hasn't been initialized or it
really is the (platform-dependent) epoch.  This will
require us to replace code like
  bool first_response = response_.response_time == Time();
by
  bool first_response = response_.response_time.is_null();
throughout our code base.
Jul 16, 2009
#22 bre...@gmail.com
In my opinion, the main reason we need to do this change isn't this particular bug
but the fact that it means our databases are platform-specific. You should be able to
move them between platforms. The update code really isn't very hard: we've done
database version updates 15 times so far.
Jul 16, 2009
#23 wtc@chromium.org
I see.  Is this because of these two Time methods?

  static Time FromInternalValue(int64 us) {
    return Time(us);
  }

  int64 ToInternalValue() const {
    return us_;
  }

Then I agree we should use the same epoch across all platforms.

net/http/http_cache.cc also calls these methods to serialize and deserialize time 
stamps.
Jul 20, 2009
#24 de...@chromium.org
Spent some time researching the databases today.  It's not exactly simple having no 
background in how the history system works.

// |start_time| needs upgrading
CREATE TABLE downloads (id INTEGER PRIMARY KEY,full_path LONGVARCHAR NOT NULL,url 
LONGVARCHAR NOT NULL,start_time INTEGER NOT NULL,received_bytes INTEGER NOT 
NULL,total_bytes INTEGER NOT NULL,state INTEGER NOT NULL);

// Nothing?
CREATE TABLE keyword_search_terms (keyword_id INTEGER NOT NULL,url_id INTEGER NOT 
NULL,lower_term LONGVARCHAR NOT NULL,term LONGVARCHAR NOT NULL);

// Nothing? (What's in the value?)
CREATE TABLE meta(key LONGVARCHAR NOT NULL UNIQUE PRIMARY KEY,value LONGVARCHAR);

// Nothing?
CREATE TABLE presentation(url_id INTEGER PRIMARY KEY,pres_index INTEGER NOT NULL);

// What is |time_slot| ?  Looks to be a time.
CREATE TABLE segment_usage (id INTEGER PRIMARY KEY,segment_id INTEGER NOT 
NULL,time_slot INTEGER NOT NULL,visit_count INTEGER DEFAULT 0 NOT NULL);

// Nothing?
CREATE TABLE segments (id INTEGER PRIMARY KEY,name VARCHAR,url_id INTEGER NON 
NULL,pres_index INTEGER DEFAULT -1 NOT NULL);

// |last_vist_time| needs upgrading?
CREATE TABLE urls(id INTEGER PRIMARY KEY,url LONGVARCHAR,title 
LONGVARCHAR,visit_count INTEGER DEFAULT 0 NOT NULL,typed_count INTEGER DEFAULT 0 NOT 
NULL,last_visit_time INTEGER NOT NULL,hidden INTEGER DEFAULT 0 NOT NULL,favicon_id 
INTEGER DEFAULT 0 NOT NULL);

// |visit_time| needs upgrading?
CREATE TABLE visits(id INTEGER PRIMARY KEY,url INTEGER NOT NULL,visit_time INTEGER 
NOT NULL,from_visit INTEGER,transition INTEGER DEFAULT 0 NOT NULL,segment_id 
INTEGER,is_indexed BOOLEAN);
Jul 20, 2009
#25 de...@chromium.org
Brett, I didn't understand your SQL, why are you multiplying by 1000000?  The times 
should have been stored in microseconds on all platforms, it was just a matter of the 
epoch shift, no?
Aug 3, 2009
#26 evan@chromium.org
(No comment was entered for this change.)
Summary: Cookies never deleted due to Win/POSIX epoch difference
Aug 4, 2009
#27 de...@chromium.org
(No comment was entered for this change.)
Status: Available
Owner: ---
Aug 12, 2009
#28 brettw@chromium.org
Dean is correct, you don't need the multiplication in the SQL I gave above. So it 
should be:
  UPDATE mytable SET mytime = mytime + 11644473600000000 WHERE id IN (SELECT
  id FROM mytable WHERE mytime > 0);

I think we must update the following information, and I don't think a patch should 
be accepted that does not update:
   segment_usage.time_slot
   urls.last_visit_tile
   visits.visit_time
Not not updating will break the autocomplete suggestions and most visited page for 
all Mac & Linux users, and this data will have to be re-learned. I think this is 
unacceptable.

Migration code should also call TextDatabaseManager.DeleteAll and set all 
visits.is_indexed to 0, since the date change will make the indexer confused. This 
isn't really worth migrating, but we should avoid the bad data by deleting it (the 
function is already there to do so so it should be trivial). To update the 
is_indexed just execute:
  UPDATE visits SET is_indexed = 0;
Aug 13, 2009
#29 mark@chromium.org
Whatever happens here, I think it's important to realize that if this slips and doesn't 
make the Mac and Linux betas, we really will need to migrate everything that's affected.

Personally, I think that dumping all of the data for Mac and Linux users now, while we're 
pre-beta, is tolerable, although certainly not ideal.  If that's all that we're able to do 
before beta, we should consider doing it if it saves us from having to do a heavier 
migration post-beta.
Aug 14, 2009
#30 mal.chromium@gmail.com
We need to use the same epoch on all platforms and migrate Linux and Mac users' data. 
We should do this now.

@deanm: assigning to you, since it looks like you've already dug into this a bit. I can 
re-assign if needed; I haven't looked at what else is on your list.
Status: Assigned
Owner: de...@chromium.org
Cc: m...@chromium.org
Aug 17, 2009
#31 de...@chromium.org
I don't have time to work on this.

Thanks
Owner: m...@chromium.org
Aug 17, 2009
#32 mal.chromium@gmail.com
(No comment was entered for this change.)
Owner: bre...@chromium.org
Aug 24, 2009
#33 evan@chromium.org
Fixed?
Aug 24, 2009
#34 willchan@chromium.org
Not yet, Brett's mailed a changelist for this that is under review.
Aug 25, 2009
#35 bugdroid1@gmail.com
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=24417 

------------------------------------------------------------------------
r24417 | brettw@chromium.org | 2009-08-25 19:53:36 -0700 (Tue, 25 Aug 2009) | 8 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/time.h?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/time_mac.cc?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/time_posix.cc?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/time_unittest.cc?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/bookmarks/bookmark_codec.cc?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/history_backend.cc?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/history_database.cc?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/history_database.h?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/password_manager/password_store_mac_unittest.cc?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/profiles/typical_history/Default/History?r1=24417&r2=24416
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/base/cookie_monster_unittest.cc?r1=24417&r2=24416

Convert internal time format to Windows 1601 epoch on Linux & Mac.

Although we represent time internally starting from 1601, there are still
things like time explosion that will not work before the year 1900.  This
limitation is the same as it was previously.

BUG=14734
Review URL: http://codereview.chromium.org/173296
------------------------------------------------------------------------

Aug 25, 2009
#36 brettw@chromium.org
(No comment was entered for this change.)
Status: Fixed
Sep 2, 2009
#37 deep...@chromium.org
Works as expected on Mac. Verified in 4.0.205.0 (Official Build 25113).
Status: Verified
Dec 18, 2009
#38 mal.chromium@gmail.com
(No comment was entered for this change.)
Labels: -Area-BrowserBackend Area-Internals
Oct 11, 2012
#39 bugdro...@chromium.org
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.
Labels: Restrict-AddIssueComment-Commit
Mar 10, 2013
#40 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Area-Internals Cr-Internals
Sign in to add a comment

Powered by Google Project Hosting