Bug 81849 - WebHistory is busted after changing time zone
Summary: WebHistory is busted after changing time zone
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-03-21 17:19 PDT by Benjamin Poulain
Modified: 2012-03-22 17:40 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.22 KB, patch)
2012-03-21 17:34 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2012-03-22 16:33 PDT, Benjamin Poulain
beidson: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-03-21 17:19:08 PDT
All history from WebHistory disappear after changing time zone.
Comment 1 Benjamin Poulain 2012-03-21 17:19:43 PDT
Radar: <rdar://problem/9091908>
Comment 2 Benjamin Poulain 2012-03-21 17:34:00 PDT
Created attachment 133154 [details]
Patch
Comment 3 mitz 2012-03-21 18:34:18 PDT
Comment on attachment 133154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133154&action=review

> Source/WebKit/mac/History/WebHistory.mm:31
> +#import "Foundation/NSTimeZone.h"

This is a system header, so you should use <> instead of "", and sort it accordingly.
Comment 4 mitz 2012-03-21 22:11:16 PDT
Comment on attachment 133154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133154&action=review

>> Source/WebKit/mac/History/WebHistory.mm:31
>> +#import "Foundation/NSTimeZone.h"
> 
> This is a system header, so you should use <> instead of "", and sort it accordingly.

Actually, I am pretty sure this #import is unnecessary, since Foundation.h is imported.
Comment 5 Brady Eidson 2012-03-21 22:57:44 PDT
Comment on attachment 133154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133154&action=review

At least one of these methods get called on every navigation, and probably multiple ones.  I really hate to see the unconditionally added work at the start of rebuildHistoryByDayIfNeeded.

You might be surprised at how heavy handed [NSTimeZone defaultTimeZone] and [NSTimeZone isEqualToTimeZone:] might be.

I would really like us to think harder about how to do this smarter.

> Source/WebKit/mac/History/WebHistory.mm:55
> +static NSTimeZone *_timeZoneOfDayBoundaries = nil;

Why NSTimeZone when the pattern in this file was already to use CFTimeZone?
Comment 6 Benjamin Poulain 2012-03-22 00:00:49 PDT
> > This is a system header, so you should use <> instead of "", and sort it accordingly.
> 
> Actually, I am pretty sure this #import is unnecessary, since Foundation.h is imported.

Yep, that is likely right. I added the header because autocompletion was not working.
Comment 7 Benjamin Poulain 2012-03-22 00:16:12 PDT
(In reply to comment #5)
> (From update of attachment 133154 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133154&action=review
> 
> At least one of these methods get called on every navigation, and probably multiple ones.  I really hate to see the unconditionally added work at the start of rebuildHistoryByDayIfNeeded.
> 
> You might be surprised at how heavy handed [NSTimeZone defaultTimeZone] and [NSTimeZone isEqualToTimeZone:] might be.

Note that we were already creating a default time zone for every call.


> > Source/WebKit/mac/History/WebHistory.mm:55
> > +static NSTimeZone *_timeZoneOfDayBoundaries = nil;
> 
> Why NSTimeZone when the pattern in this file was already to use CFTimeZone?

I did not find any way to compare two CFTimeZone equivalent to [NSTimeZone isEqualToTimeZone:].


> I would really like us to think harder about how to do this smarter.

An alternative idea I had is to store all history as a single list of HistoryItem sorted by date in GMT. The split of items by date would be done on the fly with the current TimeZone.

This would make access to the dates O(n), and access by date O(log(n)). Which is why I dismissed this option.

Ideally we would want to be notified first of TimeZone changed but I do not know of any way to do that.

What do you suggest?
Comment 8 Mark Rowe (bdash) 2012-03-22 02:01:46 PDT
(In reply to comment #7)
> > > Source/WebKit/mac/History/WebHistory.mm:55
> > > +static NSTimeZone *_timeZoneOfDayBoundaries = nil;
> > 
> > Why NSTimeZone when the pattern in this file was already to use CFTimeZone?
> 
> I did not find any way to compare two CFTimeZone equivalent to [NSTimeZone isEqualToTimeZone:].

CFEqual is equivalent.

> > I would really like us to think harder about how to do this smarter.
> 
> An alternative idea I had is to store all history as a single list of HistoryItem sorted by date in GMT. The split of items by date would be done on the fly with the current TimeZone.
> 
> This would make access to the dates O(n), and access by date O(log(n)). Which is why I dismissed this option.
> 
> Ideally we would want to be notified first of TimeZone changed but I do not know of any way to do that.

I'm not sure I follow what you're looking for here, but if it's knowing when the system time zone changes then NSSystemTimeZoneDidChangeNotification / kCFTimeZoneSystemTimeZoneDidChangeNotification would probably be what you're after.
Comment 9 Mark Rowe (bdash) 2012-03-22 02:08:19 PDT
(In reply to comment #8)
> > Ideally we would want to be notified first of TimeZone changed but I do not know of any way to do that.
> 
> I'm not sure I follow what you're looking for here, but if it's knowing when the system time zone changes then NSSystemTimeZoneDidChangeNotification / kCFTimeZoneSystemTimeZoneDidChangeNotification would probably be what you're after.

Ah, you're concerned that the issue could still occur if the WebKit client accesses WebHistory in response to itself receiving such a notification, before we've received the notification and had a chance to perform the appropriate juggling.
Comment 10 Benjamin Poulain 2012-03-22 02:17:11 PDT
> > I did not find any way to compare two CFTimeZone equivalent to [NSTimeZone isEqualToTimeZone:].
> 
> CFEqual is equivalent.

Cool! I learned something :)
I'll update with CFTypes.


> Ah, you're concerned that the issue could still occur if the WebKit client accesses WebHistory in response to itself receiving such a notification, before we've received the notification and had a chance to perform the appropriate juggling.

Yep, that is my problem with notifications.
The UI needs to update in response to the same notification, the WebHistory must be in a consistent state when accessed at that point.

Maybe the UI should instead respond only to WebHistoryAllItemsRemovedNotification and then WebHistoryItemsAddedNotification?
Comment 11 Benjamin Poulain 2012-03-22 03:00:20 PDT
> Maybe the UI should instead respond only to WebHistoryAllItemsRemovedNotification and then WebHistoryItemsAddedNotification?

I'll try that tomorrow. This should be faster and robust enough.
Comment 12 Brady Eidson 2012-03-22 09:43:52 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 133154 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133154&action=review
> > 
> > At least one of these methods get called on every navigation, and probably multiple ones.  I really hate to see the unconditionally added work at the start of rebuildHistoryByDayIfNeeded.
> > 
> > You might be surprised at how heavy handed [NSTimeZone defaultTimeZone] and [NSTimeZone isEqualToTimeZone:] might be.
> 
> Note that we were already creating a default time zone for every call.

Once in 3 calls and zero times in the 4th.

Now twice in 3 calls and once in the 4th call.

> > > Source/WebKit/mac/History/WebHistory.mm:55
> > > +static NSTimeZone *_timeZoneOfDayBoundaries = nil;
> > 
> > Why NSTimeZone when the pattern in this file was already to use CFTimeZone?
> 
> I did not find any way to compare two CFTimeZone equivalent to [NSTimeZone isEqualToTimeZone:].

Mark got this one for you already.

(In reply to comment #10)
> > Ah, you're concerned that the issue could still occur if the WebKit client accesses WebHistory in response to itself receiving such a notification, before we've received the notification and had a chance to perform the appropriate juggling.
> 
> Yep, that is my problem with notifications.
> The UI needs to update in response to the same notification, the WebHistory must be in a consistent state when accessed at that point.
> 
> Maybe the UI should instead respond only to WebHistoryAllItemsRemovedNotification and then WebHistoryItemsAddedNotification?

(In reply to comment #11) 
> I'll try that tomorrow. This should be faster and robust enough.

I think this is a great way to go.
Comment 13 Benjamin Poulain 2012-03-22 16:33:01 PDT
Created attachment 133383 [details]
Patch
Comment 14 Brady Eidson 2012-03-22 17:13:26 PDT
Comment on attachment 133383 [details]
Patch

Awesome!
Comment 15 Build Bot 2012-03-22 17:27:30 PDT
Comment on attachment 133383 [details]
Patch

Attachment 133383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12120357
Comment 16 Benjamin Poulain 2012-03-22 17:39:39 PDT
Committed r111795: <http://trac.webkit.org/changeset/111795>
Comment 17 Benjamin Poulain 2012-03-22 17:40:10 PDT
(In reply to comment #15)
> (From update of attachment 133383 [details])
> Attachment 133383 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/12120357

Really bad timing. That built locally.
I'll find and land a fix.