Closed Bug 790499 Opened 12 years ago Closed 12 years ago

Time API: Call JS_ClearDateCaches() to update Date object's timezone when the system timezone is reset

Categories

(Core :: DOM: Device Interfaces, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: airpingu, Assigned: slee)

References

Details

(Whiteboard: [LOE:M])

Attachments

(1 file, 4 obsolete files)

The timezone change notification must be propagated to JS Date object by calling JS_ClearDateCaches() so that the Date.getTimezoneOffset() won't get a deprecated timezone info.
Blocks: 714358
No longer blocks: 714358
Depends on: 714358, 285615
blocking-basecamp: --- → ?
Assignee: nobody → slee
blocking-basecamp: ? → +
Keywords: feature
Whiteboard: [LOE:M]
Attached patch patch (obsolete) — Splinter Review
nsSystemTimeChangeObserver calls JS_ClearDateCaches when timezone is changed. TimeSetting register system time change observer for making sure that JS_ClearDateCaches must be called in nsSystemTimeChangeObserver. It handles the condition that nobody listens to moztimechange event and someone changes timezone.
Attachment #660674 - Flags: review?(justin.lebar+bug)
Comment on attachment 660674 [details] [diff] [review] patch We can nit about the plumbing here which calls ClearDateCaches, but first, I'd like to make sure we get ClearDateCaches() correct. Sean, is ClearDateCaches() here correct? Or do we need to somehow iterate over all JS runtimes? (So as to catch workers? I'm really naive here.)
Attachment #660674 - Flags: review?(sstangl)
Comment on attachment 660674 [details] [diff] [review] patch Clearing this from my queue until sstangl has a chance to look at this.
Attachment #660674 - Flags: review?(justin.lebar+bug)
Comment on attachment 660674 [details] [diff] [review] patch Review of attachment 660674 [details] [diff] [review]: ----------------------------------------------------------------- This is safe, assuming that there is only a single instance of the JS engine. On multiprocess, JS_ClearDateCaches() must be called for each process that runs SpiderMonkey. Internally, JS_ClearDateCaches() just updates a global double LocalTZA. Since the double is process-global, and WebWorkers are threads within a single process, each thread will eventually see an updated LocalTZA, assuming cache coherency. However, because of races, it is not guaranteed that no incorrect timezone information can be returned after JS_ClearDateCaches() is called.
Attachment #660674 - Flags: review?(sstangl) → review+
Attachment #660674 - Flags: review?(justin.lebar+bug)
Comment on attachment 660674 [details] [diff] [review] patch I have some suggestions about how to improve the TimeSetting class. As is, I think it's pretty confusing, because, despite how it appears at first, TimeSetting is merely a timezone setting change observer: It watches the settings API and notices when the timezone changes. As far as I can tell, nobody is supposed to call TimeSetting::SetTimezone (and indeed, nobody does call TimeSetting::SetTimezone, afaict). Instead, one should call hal::SetTimezone. Because this class is an observer (and only an observer): * it should not be called "TimeSetting", which suggests that it can be correctly used to set the current time. ("TimeSetting" is doubly misleading, because this class notices changes in the /timezone/, not the time, and because it's not used for setting anything.) A better name might be TimezoneSettingObserver or TimezoneSettingWatcher. * TimeSetting::SetTimezone() should not be a public static method, because that suggests that it's meant to be called by other code. One way to make very clear the fact that this class is only an observer is to remove the class from the header file altogether. Instead, the header file could contain only a single function, for initialization. See for example xpcom/base/AvailableMemoryTracker.h. You could call mozilla::system::StartWatchingTimezoneSetting() (or whatever you decide to call it) from SystemWorkerManager, where we currently initialize the static reference to the TimeSetting instance. You could either explicitly call StopWatchingTimezoneSetting() on shutdown, or alternatively (and probably better), you could register your own shutdown listener as part of StartWatchingTimezoneSetting(). (You probably could just use ClearOnShutdown.) The design principal at work here is that we should write interfaces such that the correct ways of using them and the obvious ways of using them are the same. In this case, there are a number of obvious (to me, anyway) ways to use TimeSetting that would be incorrect. And it's not at all obvious that the correct way to use it is by creating a static reference to an instance of the class on startup. I'd be happy to review any changes you'd like to make to this code (which we should do in a separate bug). Now that I have that out of my system, I'll do the review of this patch that I owe you. :)
Comment on attachment 660674 [details] [diff] [review] patch >diff --git a/dom/system/gonk/TimeSetting.h b/dom/system/gonk/TimeSetting.h >+#include "mozilla/Hal.h" > #include "jspubtd.h" > #include "nsIObserver.h" > > namespace mozilla { > namespace system { > > class ResultListener; > >-class TimeSetting : public nsIObserver >+class TimeSetting : public nsIObserver, >+ public hal::SystemTimeObserver > { > public: > NS_DECL_ISUPPORTS > NS_DECL_NSIOBSERVER > > TimeSetting(); > virtual ~TimeSetting(); > static nsresult SetTimezone(const JS::Value &aValue, JSContext *aContext); >+ void Notify(const mozilla::hal::SystemTimeChange& aReason) {} > }; >diff --git a/dom/system/gonk/TimeSetting.cpp b/dom/system/gonk/TimeSetting.cpp >@@ -97,16 +97,18 @@ TimeSetting::TimeSetting() >+ hal::RegisterSystemTimeChangeObserver(this); >@@ -124,16 +126,17 @@ nsresult TimeSetting::SetTimezone(const > TimeSetting::~TimeSetting() > { > nsCOMPtr<nsIObserverService> observerService = services::GetObserverService(); > if (observerService) { > observerService->RemoveObserver(this, MOZSETTINGS_CHANGED); > } >+ hal::UnregisterSystemTimeChangeObserver(this); > } Maybe I'm missing something, but it's not clear to me that this does what we want. I understand that this code is to cover the case (from comment 1) "that nobody listens to moztimechange event and someone changes timezone." But suppose nobody is listening to the moztimechange event. That means that TimeChangeObserver's mWindowListeners.Length() == 0, right? And when that happens, TimeChangeObserver calls UnregisterSystemTimeChangeObserver(sObserver). So I don't see how calling hal::RegisterSystemTimeChangeObserver(this) above ensures that TimeChangeObserver is notified of changes to the system time. It seems to me that the simplest thing to do would be to have hal::NotifySystemTimeChange() call JS_ClearDateCaches. (We should call it whenever the time changes, not just when the timezone changes.) But does hal::NotifySystemTimeChange get called in every process? If not, none of this will work. It looks like hal::NotifySystemTimeChange doesn't get broadcast to all processes, but I'm not done reading quite yet...
Attachment #660674 - Flags: review?(justin.lebar+bug) → review-
> It looks like hal::NotifySystemTimeChange doesn't get broadcast to all processes, but I'm not done > reading quite yet... Yeah, I don't see how this gets broadcast to all processes, so atm, it looks like NotifySystemTimeChange will only notify the main process, instead of notifying all processes as it should. So that's another bug you'll have to fix here or elsewhere.
Depends on: 791962
No longer depends on: 791962
(In reply to Justin Lebar [:jlebar] from comment #5) > Comment on attachment 660674 [details] [diff] [review] > I'd be happy to review any changes you'd like to make to this code (which we > should do in a separate bug). > > Now that I have that out of my system, I'll do the review of this patch that > I > owe you. :) Hi Justin, I looked Bug783021. The timezone is set through settings api. TimeSetting is to observe the timezone change. When timezone is changed, TimeSetting should call hal::SetTimezone(). If we want to change TimeSetting to a pure observer, we may need a way to set time zone and I am happy to take this job. :)
> If we want to change TimeSetting to a pure observer, we may > need a way to set time zone and I am happy to take this job. :) Could you rephrase this? I don't understand.
(In reply to Justin Lebar [:jlebar] from comment #6) > Comment on attachment 660674 [details] [diff] [review] > patch > So I don't see how calling hal::RegisterSystemTimeChangeObserver(this) above > ensures that TimeChangeObserver is notified of changes to the system time. Ah, you are right. > > It seems to me that the simplest thing to do would be to have > hal::NotifySystemTimeChange() call JS_ClearDateCaches. (We should call it > whenever the time changes, not just when the timezone changes.) > > But does hal::NotifySystemTimeChange get called in every process? If not, > none > of this will work. It looks like hal::NotifySystemTimeChange doesn't get > broadcast to all processes, but I'm not done reading quite yet... My original thought was to call JS_ClearDateCaches in hal::NotifySystemTimeChange. But I think it's a little strange to call JS functions in hal. So that I tried to implement it in TimeSetting. It seems that I had better to call this function in hal::NotifySystemTimeChange.
(In reply to Justin Lebar [:jlebar] from comment #9) > Could you rephrase this? I don't understand. I think I misunderstood what you mean. What you want is 1. Change the name to a suitable one. 2. Hide TimeSetting class since it is not meant to be called by others. Is that right?
> My original thought was to call JS_ClearDateCaches in > hal::NotifySystemTimeChange. But I think it's a little strange > to call JS functions in hal. I agree. If you wanted to add a separate observer just for this, that would be fine with me. It's a bit clumsy either way. > I think I misunderstood what you mean. What you want is > 1. Change the name to a suitable one. > 2. Hide TimeSetting class since it is not meant to be called by > others. > Is that right? Yes. I'd add maybe (3) Add some (brief!) documentation.
Attached patch patch (obsolete) — Splinter Review
Add an extra observer to observe system clock/time zone change. This observer lives in nsGlobalWindow. It should solve the problem that only main process can be notified when time zone changes.
Attachment #660674 - Attachment is obsolete: true
Attachment #662428 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 662428 [details] [diff] [review] patch This is the right idea, but it has two problems: - We'll reset the date caches once for every window we have. nsGlobalWindow is not (despite its name) a top-level Firefox window. Rather, it corresponds to a JS "global", which corresponds to a JS |window| object. We get one nsGlobalWindow for each tab and iframe, plus more for Firefox's chrome windows. We only want to call JS_ClearDateCaches once per process. - Not all processes have an nsGlobalWindow instance. Most do, but in B2G, we have a "spare" process which we use to accelerate app startup, and I don't think that process has any nsGlobalWindows. But we still want to call JS_ClearDateCaches in that process when the system time changes. If you want to keep the JS_ClearDateCaches() call out of hal, perhaps it would make sense to incorporate this code into the time zone change code we talked about elsewhere. Somewhere, you call mozilla::time::InitializeTimeChangeObservers(), and that creates a time zone change observer (if appropriate -- we don't want one on all systems) and also creates a time change observer, like above.
Attachment #662428 - Flags: feedback?(justin.lebar+bug) → feedback-
Attached patch patch (obsolete) — Splinter Review
Attachment #662428 - Attachment is obsolete: true
Attachment #662863 - Flags: review?(justin.lebar+bug)
Comment on attachment 662863 [details] [diff] [review] patch >diff --git a/dom/system/gonk/TimeSetting.cpp b/dom/system/gonk/TimeSetting.cpp >--- a/dom/system/gonk/TimeSetting.cpp >+++ b/dom/system/gonk/TimeSetting.cpp > do_GetService("@mozilla.org/settingsService;1"); > if (!settingsService) { > ERR("Failed to get settingsLock service!"); > return; > } > settingsService->CreateLock(getter_AddRefs(lock)); > nsCOMPtr<nsISettingsServiceCallback> callback = new InitTimezoneCb(); > lock->Get(TIME_TIMEZONE, callback); >+ >+ time::InitializeDateCacheCleaner(); > } Hmm. This file is Gonk-only code. In theory, we want to have a DateCacheCleaner on all platforms, because in theory, we should inform the JSAPI when the system timezone changes on any platform. In practice, we're not /listening/ for timezone changes on any other platform. But I feel like we still ought to run this code everywhere, so that if and when we do start listening for timezone changes on other platforms, we won't wonder why JS isn't hearing those changes. How about we just put this in nsLayoutStatics::Initialize()? (That has the added advantage of always being called, whereas TimeSetting() is called from SystemWorkerManager() which is lazily initialized, so probably has the same problem in the "spare" process as we had before.) >diff --git a/dom/time/DateCacheCleaner.h b/dom/time/DateCacheCleaner.h >new file mode 100644 >--- /dev/null >+++ b/dom/time/DateCacheCleaner.h >@@ -0,0 +1,10 @@ Please add a brief comment here explaining what InitializeDateCacheCleaner() does. >+namespace mozilla { >+namespace time { >+void InitializeDateCacheCleaner(); >+} //namespace time >+} //namespace mozilla >diff --git a/dom/time/DateCacheCleaner.cpp b/dom/time/DateCacheCleaner.cpp >new file mode 100644 >--- /dev/null >+++ b/dom/time/DateCacheCleaner.cpp >@@ -0,0 +1,59 @@ >+#include "mozilla/Hal.h" >+#include "mozilla/ClearOnShutdown.h" >+#include "DateCacheCleaner.h" >+ >+#include "nsIJSContextStack.h" >+#include "mozilla/StaticPtr.h" >+ >+ >+using namespace mozilla::hal; >+ >+typedef mozilla::Observer<mozilla::hal::SystemTimeChange> SystemTimeChangeObserver; Since this isn't a header file, we prefer |using namespace| to explicit namespace names: using namespace mozilla; using namespace mozilla::hal; typedef Observer<SystemTimeChange> SystemTimeChangeObserver; Although it seems to me that a more correct way to do this would be to move the typedef into hal/HalTypes.h (like SwitchObserver) and then to include hal/HalTypes.h here and in the one other place SystemTimeChangeObserver is used. >+namespace mozilla { >+namespace time { >+ >+class DateCacheCleaner : public SystemTimeChangeObserver >+{ >+public: >+ DateCacheCleaner() >+ { >+ RegisterSystemTimeChangeObserver(this); >+ } >+ >+ ~DateCacheCleaner() >+ { >+ UnregisterSystemTimeChangeObserver(this); >+ } >+ void Notify(const SystemTimeChange& aReason) Nit: Newline after }. >+ { >+ nsCOMPtr<nsIThreadJSContextStack> stack = >+ do_GetService("@mozilla.org/js/xpc/ContextStack;1"); >+ if (!stack) { >+ NS_WARNING("Failed to get JSContextStack"); >+ } >+ JSContext *cx = stack->GetSafeJSContext(); >+ if (!cx) { >+ NS_WARNING("Failed to GetSafeJSContext"); >+ } >+ JS_ClearDateCaches(cx); Sorry, I was completely wrong -- we only want to do this when the timezone changes, as you had it originally. (I mean, it's not hurting anything how it is, but the only thing impacted by the ClearDateCaches call is the JS engine's timezone cache.) Would you mind putting back the check you had originally? This is basically right, but I'd like to take another look.
Attachment #662863 - Flags: review?(justin.lebar+bug) → review-
(In reply to Justin Lebar [:jlebar] from comment #16) > How about we just put this in nsLayoutStatics::Initialize()? > > (That has the added advantage of always being called, whereas TimeSetting() > is > called from SystemWorkerManager() which is lazily initialized, so probably > has > the same problem in the "spare" process as we had before.) This is a better place to call InitializeDateCacheCleaner. I wanted to find such place but I was not sure where is good. > Sorry, I was completely wrong -- we only want to do this when the timezone > changes, as you had it originally. (I mean, it's not hurting anything how it > is, but the only thing impacted by the ClearDateCaches call is the JS > engine's > timezone cache.) > > Would you mind putting back the check you had originally? That's OK, just put the code back.
Attached patch patch - V2 (obsolete) — Splinter Review
Attachment #662863 - Attachment is obsolete: true
Attachment #663299 - Flags: review?(justin.lebar+bug)
Comment on attachment 663299 [details] [diff] [review] patch - V2 It looks like you missed one of the nits from earlier > Nit: Newline after }. But I'll add that before I push this. Thanks for the patch!
Attachment #663299 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f16bba541cc (Note: I pushed this without try coverage, because I think it has a better-than-even chance of going green.)
Hi Justin, Sorry for the convenience. I found that one crash problem is resulted from that both sDateCacheCleaner and sSystemTimeObservers are deleted by ClearOnShutdown mechanism. The initial sequence is sDateCacheCleaner then sSystemTimeObservers. And release order is sSystemTimeObservers then sDateCacheCleaner. It looks good. But when deleting sDateCacheCleaner, it calls UnregisterSystemChangeObserver and this function try to use sSytemTimeObservers. Then the program crashes. The patch of Bug 792443 changes the usage of sSystemTimeObservers which does not use StaticAutoPtr. I will try it on try server and check whether it can solve the current problem.
Sounds good to me.
Hi Justin, After Bug792243 is applied, I found the test cases still crashed. The cause of the problem is that when the chrome process is killed or the ipc between chrome process or content process is disconnected. The ipc resource of content process is freed and content process tries to shutdown. It calls UnregisterSystemChangeObserver and uses the ipc resource that is freed. After discussed with Cervantes, we found that HalParent implements ActorDestroy to handle such condition but HalChild does not. http://mxr.mozilla.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#381 We think we should have a ActorDestroy for HalChild to deal with it. I will file another bug to solve this problem first.
Depends on: 793759
Hi Justin, The crash problem is resulted from the release sequence of content process. For detail information, please check https://bugzilla.mozilla.org/show_bug.cgi?id=793759#c10. In comment, https://bugzilla.mozilla.org/show_bug.cgi?id=793759#c11, cjones said that we should not unregister content process's observers on shutdown. If we don't unregister observers, we get many memory leak reports in test cases, https://tbpl.mozilla.org/?tree=Try&rev=f2b896453307 :(. I think I can 1. do not unregister observers on content process. There will be memory leak but only when the content process is going down. 2. call the destructor during uiMessageLoop.MessageLoop::Run()[1]. I need some time to check where is the proper places to call. [1]http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#485
> If we don't unregister observers, we get many memory leak reports in test cases, > https://tbpl.mozilla.org/?tree=Try&rev=f2b896453307 :(. Are you certain those are content-process leaks? I'm not sure our infrastructure even does leak checking in the content process, but even if it did, I would be surprised if we started a content process on all desktop test suites.
(In reply to Justin Lebar [:jlebar] from comment #26) > Are you certain those are content-process leaks? I'm not sure our > infrastructure even does leak checking in the content process, but even if > it did, I would be surprised if we started a content process on all desktop > test suites. Yes, I did the test again and found that the leak happens in content process.
If it's really content process leaks that are turning the test suite orange, then you obviously can't follow cjones's advice to allow those objects to leak. But could you show me how you know that they're content process leaks? I'm pretty surprised that we load a content process, for example, on Windows crashtests (one of tests which turned orange in the try build from comment 25).
(In reply to Justin Lebar [:jlebar] from comment #28) > But could you show me how you know that they're content process leaks? I'm > pretty surprised that we load a content process, for example, on Windows > crashtests (one of tests which turned orange in the try build from comment > 25). I log the process's type by |XRE_GetProcessType()| and its process id. After test cases are done, I compare the process type and pid with my log. I found that the leaking processes are content processes.
I do not think this is a feature. By not fixing this bug, an application showing a date (like the status bar) will not be correctly updated if the user decides to change the timezone of his device IIRC. ... but in my current B2G build, it's not possible to change the time (or it's very well hidden) so I can't confirm that.
(In reply to Mounir Lamouri (:mounir) from comment #30) > I do not think this is a feature. By not fixing this bug, an application > showing a date (like the status bar) will not be correctly updated if the > user decides to change the timezone of his device IIRC. > > ... but in my current B2G build, it's not possible to change the time (or > it's very well hidden) so I can't confirm that. Just leaving some notes here if anyone is hoping to change system time: 1. Time clock: navigator.mozTime.set((new Date())); 2. Time zone: navigator.mozSettings.createLock({time.timezone: "America/Chicago"});
(In reply to Mounir Lamouri (:mounir) from comment #30) > I do not think this is a feature. By not fixing this bug, an application > showing a date (like the status bar) will not be correctly updated if the > user decides to change the timezone of his device IIRC. > > ... but in my current B2G build, it's not possible to change the time (or > it's very well hidden) so I can't confirm that. IanLiu is working on this feature. You can change the date and time on Settings app now.
(In reply to StevenLee from comment #32) > (In reply to Mounir Lamouri (:mounir) from comment #30) > > I do not think this is a feature. By not fixing this bug, an application > > showing a date (like the status bar) will not be correctly updated if the > > user decides to change the timezone of his device IIRC. > > > > ... but in my current B2G build, it's not possible to change the time (or > > it's very well hidden) so I can't confirm that. > > IanLiu is working on this feature. You can change the date and time on > Settings app now. I've heard it has landed a few hours ago.
After talking with Steven, we agreed with Mounir that it's not a feature implementation case. I removed 'feature' from keywords. Tag back if it's needed. Suggestions and/or comments are welcome. Thanks.
Keywords: feature
FYI, I landed bug 796523 to clear the timezone cache whenever we create a new compartment. I don't think that obviates this bug, or makes it not a blocker, particularly because we need to update the timezone cache /immediately/ so that the clock displays the correct time, and triggering that update on gc / compartment creation just won't work.
Blocks: 801096
Priority: -- → P1
Blocks: 801838
Steven, did you made any progress regarding the issues you found (re comment 25)?
(In reply to Mounir Lamouri (:mounir) from comment #36) > Steven, did you made any progress regarding the issues you found (re comment > 25)? Hi Mounir, Bug793759 is landed that fixed the memory leaking problem. The testing results on try server confuses me. At first, the test cases crashed on WinXP-debug, https://tbpl.mozilla.org/?tree=Try&rev=f21fcf0e1a98. I tried to build and tested on windows. But a few days later, I updated the source code and tried again. The crash problem is gone. Then the same condition happens on Linux64-debug https://tbpl.mozilla.org/?tree=Try&rev=f6411f383a6c. I tried to figure out the problem. Today I updated the source code and tried the same patch on try server again. There is no crash problem on Linux64-debug but it has crash problem on OS X 10.7 debug. I am not sure if I could check in the patch now. :(
Attached patch patch v2.1Splinter Review
rebase
Attachment #663299 - Attachment is obsolete: true
Attachment #672197 - Flags: review+
Try run for bbb73a4579fa is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=bbb73a4579fa Results (out of 237 total builds): success: 219 warnings: 18 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/slee@mozilla.com-bbb73a4579fa
Keywords: checkin-needed
> I am not sure if I could check in the patch now. :( You can always ask on IRC for help interpreting tryserver results. edmorley and philor can definitely help, and others probably can as well. I pushed this to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/80fffd499dca If you remind me tomorrow, I can push this to Aurora for you.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Does the fix here remove the need to have bug 815414 fixed?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: