Bug 81849 – WebHistory is busted after changing time zone

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Benjamin Poulain]

All history from WebHistory disappear after changing time zone.

[Benjamin Poulain]

Radar: <rdar://problem/9091908>

[Benjamin Poulain]

Created attachment 133154 [details]
Patch

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 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.

[Brady Eidson]

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?

[Benjamin Poulain]

> > 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.

[Benjamin Poulain]

(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?

[Mark Rowe (bdash)]

(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.

[Mark Rowe (bdash)]

(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.

[Benjamin Poulain]

> > 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?

[Benjamin Poulain]

> Maybe the UI should instead respond only to WebHistoryAllItemsRemovedNotification and then WebHistoryItemsAddedNotification?

I'll try that tomorrow. This should be faster and robust enough.

[Brady Eidson]

(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.

[Benjamin Poulain]

Created attachment 133383 [details]
Patch

[Brady Eidson]

Comment on attachment 133383 [details]
Patch

Awesome!

[Build Bot]

Comment on attachment 133383 [details]
Patch

Attachment 133383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12120357

[Benjamin Poulain]

Committed r111795: <http://trac.webkit.org/changeset/111795>

[Benjamin Poulain]

(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.