Closed
Bug 714358
Opened 13 years ago
Closed 12 years ago
Time API: notification and datetime set
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: cjones, Assigned: slee)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, feature, Whiteboard: [WebAPI:P1])
Attachments
(3 files, 27 obsolete files)
(deleted),
patch
|
slee
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
slee
:
review+
justin.lebar+bug
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
slee
:
review+
|
Details | Diff | Splinter Review |
This is useful on all platforms. I don't believe that notifying apps when time has changed is a privacy/security concern; they can (approximately) poll for system-clock changes themselves.
Strawman proposal: a "clockchange" event that content can register for with window.addEventListener("clockchange", ...). Certainly open for suggestions on the name; maybe "systemclockchange" is better? "systemtimechange"?
Comment 1•13 years ago
|
||
What are the use case for that?
Do we want to notify when there is an unexpected change in the clock or only when there is a TZ change?
Blocks: webapi
Comment 2•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> What are the use case for that?
I mean for regular content, not a clock app. We could reasonably assume that a clock app could use setInterval() to get the current time.
Comment 3•13 years ago
|
||
What would trigger that event? I think we are talking about /adjusting/ the system time here, right?
Comment 4•13 years ago
|
||
If this event should also apply when there is a TZ change, you should note that our Date object implementation has a bug and doesn't return an updated value when the system TZ changes apparently.
Reporter | ||
Comment 5•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> What are the use case for that?
Any app that displays time relative to "now". Clock, calendar, alarm, email, ...
> Do we want to notify when there is an unexpected change in the clock or only
> when there is a TZ change?
Both. I don't know of a case when an app would do something different with a system-clock change vs. a timezone change. But if you can think of something, maybe it would be useful to have separate notifications.
(In reply to Jens Müller from comment #3)
> What would trigger that event? I think we are talking about /adjusting/ the
> system time here, right?
That's right. The event would be triggered by system-clock and timezone reconfiguration.
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> If this event should also apply when there is a TZ change, you should note
> that our Date object implementation has a bug and doesn't return an updated
> value when the system TZ changes apparently.
We should just fix that :).
Comment 6•13 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> > Do we want to notify when there is an unexpected change in the clock or only
> > when there is a TZ change?
>
> Both. I don't know of a case when an app would do something different with
> a system-clock change vs. a timezone change. But if you can think of
> something, maybe it would be useful to have separate notifications.
Actually, I would prefer to not have any differentiation.
Reporter | ||
Comment 7•13 years ago
|
||
See bug 715041 comment 5; looks like this API should be privileged, too.
Reporter | ||
Comment 8•13 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> (In reply to Jesse Ruderman from comment #5)
> > But I am worried about privacy, and in particular attacks that correlate an
> > anonymous identity with another identity. If both identities are constantly
> > broadcasting their idleness, it becomes much easier to correlate them. Kind
> > of like what Aaron Barr tried to do to members of Anonymous
> > <http://arstechnica.com/tech-policy/news/2011/02/how-one-security-firm-
> > tracked-anonymousand-paid-a-heavy-price.ars/2>.
> >
>
> Interesting. Bug 714358 would be vulnerable to this as well. I guess we
> should make both of these privileged.
>
Actually, since content can already poll for system-clock changes with high precision, a systemclockchanged notification doesn't reveal any new information, even though it may be vulnerable to the same attack if NITZ updates result in system-clock adjustments frequently.
Reporter | ||
Comment 9•13 years ago
|
||
Yes.
Summary: Notification when system time has changed → Notification when system clock is recalibrated and when the timezone changes
Reporter | ||
Updated•13 years ago
|
Blocks: b2g-demo-phone
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → slee
Assignee | ||
Comment 10•13 years ago
|
||
Hi Chris,
After discussed with Thinker, we planed to use timerfd_create a file descriptor with flag, TFD_TIMER_CANCEL_ON_SET. Setting a very long timeout, listen to it and when system time changes, we can get notification. But this flag is available after kernel 3.0.27. Should I implement a poller thread that periodically(ex, every second or longer) checking if the system time is changed first?
Reporter | ||
Comment 11•13 years ago
|
||
On gonk, the system clock and timezone can only be set through the hal:: APIs for time. All we need to do is broadcast a notification when those APIs are used.
Don't worry about other platforms for the moment.
Assignee | ||
Comment 12•13 years ago
|
||
Interface of DOMEvent. When system time is changed, SystemTimeChangedEvent will be fired. There is only one data member that tells the app whether time clock or time zone is changed.
Attachment #622572 -
Flags: superreview?(jonas)
Assignee | ||
Comment 13•13 years ago
|
||
Assignee | ||
Comment 14•13 years ago
|
||
Comment 15•13 years ago
|
||
Comment on attachment 622572 [details] [diff] [review]
SystemTimeChanged event interface
Review of attachment 622572 [details] [diff] [review]:
-----------------------------------------------------------------
You don't need that specific event. Listening for timezone change should be done trough the settings api.
Attachment #622572 -
Flags: review-
Comment 16•13 years ago
|
||
Steven, I'm not sure which API you are implementing. Could you implement what is described in the proposal that has been made in the WebAPI mailing-list: http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/f382abb068abd4c5
Attachment #622572 -
Flags: superreview?(jonas)
Mounir, I think a timezonechange event would actually make sense. That way it's a cleaner and safer API which we can expose to all web pages. Web pages can notice timezone changes anyway by polling |new Date()| so it doesn't hurt to expose it to everyone.
However we shouldn't have a numeric 'reason' property. Instead we should just have separate ontimechange and ontimezonechange events. That way there's no need for a new event interface.
Actually, Mounir convinced me. We should just have a single event for when the time or timezone changes.
Unless we have any strong use-cases I don't think we should expose a reason though. Any time the timezone changes, the time does as well.
Assignee | ||
Comment 19•13 years ago
|
||
Hi Mounir,
I will implement this interface later.
Assignee | ||
Comment 20•13 years ago
|
||
Attachment #622572 -
Attachment is obsolete: true
Attachment #623916 -
Flags: superreview?(jonas)
Comment 21•13 years ago
|
||
Comment on attachment 623916 [details] [diff] [review]
SystemTimeChanged event interface V2
Review of attachment 623916 [details] [diff] [review]:
-----------------------------------------------------------------
It's useless to create a new event type with no new attributes. You can just use nsIDOMEvent.
Attachment #623916 -
Flags: superreview?(jonas) → review-
Assignee | ||
Comment 22•13 years ago
|
||
Attachment #622573 -
Attachment is obsolete: true
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #622574 -
Attachment is obsolete: true
Comment 24•13 years ago
|
||
What is |InternalNotifySystemTimeChange| for?
Updated•13 years ago
|
Blocks: b2g-product-phone
Updated•13 years ago
|
No longer blocks: b2g-demo-phone
Updated•13 years ago
|
Whiteboard: [b2g:blocking+]
Assignee | ||
Comment 25•13 years ago
|
||
Hi Mounir,
|InternalNotifySystemTimeChange| is called when apps are changing system clock or time zone. After time is changed, this function notifies the registered observers. Since this interface is not called by apps directly so that I add the prefix Internal.
Assignee | ||
Updated•13 years ago
|
Attachment #627903 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #627905 -
Flags: review?
Comment 26•13 years ago
|
||
Steven, you have to ask a review to a hal person (cjones, jlebar or me) and a Gonk person for "Implement the observer that will notify apps when system changes". For the second patch, you need to ask a review to a DOM peer.
Assignee | ||
Updated•13 years ago
|
Attachment #627903 -
Flags: review? → review?(mounir)
Assignee | ||
Updated•13 years ago
|
Attachment #627905 -
Flags: review? → review?(bent.mozilla)
Comment 27•13 years ago
|
||
Comment on attachment 627903 [details] [diff] [review]
Implement the observer that will notify apps when system changes - V2
Review of attachment 627903 [details] [diff] [review]:
-----------------------------------------------------------------
::: hal/Hal.cpp
@@ +444,5 @@
> AdjustSystemClock(int32_t aDeltaMilliseconds)
> {
> AssertMainThread();
> PROXY_IF_SANDBOXED(AdjustSystemClock(aDeltaMilliseconds));
> + InternalNotifySystemTimeChange(SYS_TIME_CHANGE_CLOCK);
Don't do that: AdjustSystemClock() should call Notify() instead.
@@ +452,5 @@
> SetTimezone(const nsCString& aTimezoneSpec)
> {
> AssertMainThread();
> PROXY_IF_SANDBOXED(SetTimezone(aTimezoneSpec));
> + InternalNotifySystemTimeChange(SYS_TIME_CHANGE_TZ);
ditto.
::: hal/Hal.h
@@ +265,5 @@
> +/**
> + * Notify of a change in the system cloeck or time zone.
> + * @param aReason
> + */
> +void NotifySystemTimeChange(const hal::SystemTimeChange& aReason);
What is the reason for? We decided that the DOM API will not show the reason. Do we want to provide that for something else?
::: hal/HalInternal.h
@@ +63,5 @@
> * Disable switch notifications from the backend
> */
> void DisableSwitchNotifications(hal::SwitchDevice aDevice);
>
> +void InternalNotifySystemTimeChange(const hal::SystemTimeChange& aReason);
This is useless. You should use the Notify method.
::: hal/HalTypes.h
@@ +72,5 @@
> +enum SystemTimeChange {
> + SYS_TIME_CHANGE_UNKNOWN = -1,
> + SYS_TIME_CHANGE_CLOCK,
> + SYS_TIME_CHANGE_TZ,
> + SYS_TIME_CHANGE_NUM
That should be suffixed by _GUARD.
::: hal/fallback/FallbackTime.cpp
@@ +20,5 @@
>
> +void
> +InternalNotifySystemTimeChange(const hal::SystemTimeChange& aReason)
> +{}
> +
AFAICT, you are missing Register, Unregister and Notify methods here.
Attachment #627903 -
Flags: review?(mounir) → review-
Comment 28•13 years ago
|
||
Comment on attachment 627905 [details] [diff] [review]
DOMEvent implementation
Review of attachment 627905 [details] [diff] [review]:
-----------------------------------------------------------------
Based on the previous patch, this patch should obviously be updated. Also, you don't need a new DOM event object.
Attachment #627905 -
Flags: review?(bent.mozilla) → review-
Assignee | ||
Comment 29•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #27)
> Comment on attachment 627903 [details] [diff] [review]
> Implement the observer that will notify apps when system changes - V2
>
> Review of attachment 627903 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: hal/Hal.cpp
> @@ +444,5 @@
> > AdjustSystemClock(int32_t aDeltaMilliseconds)
> > {
> > AssertMainThread();
> > PROXY_IF_SANDBOXED(AdjustSystemClock(aDeltaMilliseconds));
> > + InternalNotifySystemTimeChange(SYS_TIME_CHANGE_CLOCK);
>
> Don't do that: AdjustSystemClock() should call Notify() instead.
>
Do you mean that I should call InternalNotifySystemTimeChange in PROXY_IF_SANDBOXED(AdjustSystemClock(aDeltaMilliseconds))?
Is that for performance issue? If it is, I think we should not worry too much.
Since most of the users do not change time very often.
> ::: hal/Hal.h
> @@ +265,5 @@
> > +/**
> > + * Notify of a change in the system cloeck or time zone.
> > + * @param aReason
> > + */
> > +void NotifySystemTimeChange(const hal::SystemTimeChange& aReason);
>
> What is the reason for? We decided that the DOM API will not show the
> reason. Do we want to provide that for something else?
I will delete this parameter in next version.
>
> ::: hal/fallback/FallbackTime.cpp
> @@ +20,5 @@
> >
> > +void
> > +InternalNotifySystemTimeChange(const hal::SystemTimeChange& aReason)
> > +{}
> > +
>
> AFAICT, you are missing Register, Unregister and Notify methods here.
These functions do not need IPC. Do I need to implement here?
Assignee | ||
Comment 30•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #28)
> Comment on attachment 627905 [details] [diff] [review]
> DOMEvent implementation
>
> Review of attachment 627905 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Based on the previous patch, this patch should obviously be updated. Also,
> you don't need a new DOM event object.
This is my first time implementing DOMEvent. Sorry for asking stupid questions.
When I use the DOMEvent with a new name "systemtimechange", does it mean that creating a new DOMEvent? I thought that adding a new type inherited from nsIDOMEvent is to creating a new DOM event, is that right?
Assignee | ||
Comment 31•13 years ago
|
||
> Since most of the users do not change time very often.
> > ::: hal/Hal.h
> > @@ +265,5 @@
> > > +/**
> > > + * Notify of a change in the system cloeck or time zone.
> > > + * @param aReason
> > > + */
> > > +void NotifySystemTimeChange(const hal::SystemTimeChange& aReason);
> >
> > What is the reason for? We decided that the DOM API will not show the
> > reason. Do we want to provide that for something else?
> I will delete this parameter in next version.
>
Hi Mounir,
ObserverList needs a data type to instantiate the object. If we want to Notify without anything, we need another ObserverList. I cannot find the suitable class from the existing code. Can you suggest any class that is good for this?
Thanks~
Comment 32•13 years ago
|
||
(In reply to StevenLee from comment #29)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #27)
> > ::: hal/Hal.cpp
> > @@ +444,5 @@
> > > AdjustSystemClock(int32_t aDeltaMilliseconds)
> > > {
> > > AssertMainThread();
> > > PROXY_IF_SANDBOXED(AdjustSystemClock(aDeltaMilliseconds));
> > > + InternalNotifySystemTimeChange(SYS_TIME_CHANGE_CLOCK);
> >
> > Don't do that: AdjustSystemClock() should call Notify() instead.
> >
> Do you mean that I should call InternalNotifySystemTimeChange in
> PROXY_IF_SANDBOXED(AdjustSystemClock(aDeltaMilliseconds))?
> Is that for performance issue? If it is, I think we should not worry too
> much.
> Since most of the users do not change time very often.
No, this is not for performance reasons. It is because the backend should be responsible of Notifying there is a time change, not HAL. In addition, |AdjustSystemClock| could happen to not change the time (imagine there is a call with aDeltaMs = 0).
Also, doing so would remove the need of |InternalNotifySystemTimeChange|.
> > ::: hal/fallback/FallbackTime.cpp
> > @@ +20,5 @@
> > >
> > > +void
> > > +InternalNotifySystemTimeChange(const hal::SystemTimeChange& aReason)
> > > +{}
> > > +
> >
> > AFAICT, you are missing Register, Unregister and Notify methods here.
> These functions do not need IPC. Do I need to implement here?
Oups, indeed.
(In reply to StevenLee from comment #30)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #28)
> > Based on the previous patch, this patch should obviously be updated. Also,
> > you don't need a new DOM event object.
> This is my first time implementing DOMEvent. Sorry for asking stupid
> questions.
> When I use the DOMEvent with a new name "systemtimechange", does it mean
> that creating a new DOMEvent? I thought that adding a new type inherited
> from nsIDOMEvent is to creating a new DOM event, is that right?
No. Sending a DOM event with a specific name can be done simply with a nsDOMEvent.
See: BatteryManager::DispatchTrustedEventToSelf or nsContentUtils::DispatchTrustedEvent
What you are doing is for new DOM Event object with new parameters.
(In reply to StevenLee from comment #31)
> > Since most of the users do not change time very often.
> > > ::: hal/Hal.h
> > > @@ +265,5 @@
> > > > +/**
> > > > + * Notify of a change in the system cloeck or time zone.
> > > > + * @param aReason
> > > > + */
> > > > +void NotifySystemTimeChange(const hal::SystemTimeChange& aReason);
> > >
> > > What is the reason for? We decided that the DOM API will not show the
> > > reason. Do we want to provide that for something else?
> > I will delete this parameter in next version.
> >
> Hi Mounir,
> ObserverList needs a data type to instantiate the object. If we want to
> Notify without anything, we need another ObserverList. I cannot find the
> suitable class from the existing code. Can you suggest any class that is
> good for this?
Good question. I see actually only two solutions: whether you use a dummy object (more or less like you are doing) as a workaround. Otherwise, you could use XPCOM observers. I would tend to the second solution. Though, I would prefer to have Chris' opinion here.
Updated•13 years ago
|
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+]
Reporter | ||
Comment 33•13 years ago
|
||
Whether or not the DOM API exposes the reason for time change, I don't see any reason for hal not to. But another solution is to use ObserverList<void_t>, which I would strongly prefer over observer service.
Assignee | ||
Comment 34•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #32)
> > Do you mean that I should call InternalNotifySystemTimeChange in
> > PROXY_IF_SANDBOXED(AdjustSystemClock(aDeltaMilliseconds))?
> > Is that for performance issue? If it is, I think we should not worry too
> > much.
> > Since most of the users do not change time very often.
>
> No, this is not for performance reasons. It is because the backend should be
> responsible of Notifying there is a time change, not HAL. In addition,
> |AdjustSystemClock| could happen to not change the time (imagine there is a
> call with aDeltaMs = 0).
> Also, doing so would remove the need of |InternalNotifySystemTimeChange|.
OK~ I see.
> (In reply to StevenLee from comment #30)
> > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #28)
> > > Based on the previous patch, this patch should obviously be updated. Also,
> > > you don't need a new DOM event object.
> > This is my first time implementing DOMEvent. Sorry for asking stupid
> > questions.
> > When I use the DOMEvent with a new name "systemtimechange", does it mean
> > that creating a new DOMEvent? I thought that adding a new type inherited
> > from nsIDOMEvent is to creating a new DOM event, is that right?
>
> No. Sending a DOM event with a specific name can be done simply with a
> nsDOMEvent.
> See: BatteryManager::DispatchTrustedEventToSelf or
> nsContentUtils::DispatchTrustedEvent
>
> What you are doing is for new DOM Event object with new parameters.
Thanks, I will modify it.
> (In reply to StevenLee from comment #31)
> > > Since most of the users do not change time very often.
> > > > ::: hal/Hal.h
> > > > @@ +265,5 @@
> > > > > +/**
> > > > > + * Notify of a change in the system cloeck or time zone.
> > > > > + * @param aReason
> > > > > + */
> > > > > +void NotifySystemTimeChange(const hal::SystemTimeChange& aReason);
> > > >
> > > > What is the reason for? We decided that the DOM API will not show the
> > > > reason. Do we want to provide that for something else?
> > > I will delete this parameter in next version.
> > >
> > Hi Mounir,
> > ObserverList needs a data type to instantiate the object. If we want to
> > Notify without anything, we need another ObserverList. I cannot find the
> > suitable class from the existing code. Can you suggest any class that is
> > good for this?
>
> Good question. I see actually only two solutions: whether you use a dummy
> object (more or less like you are doing) as a workaround. Otherwise, you
> could use XPCOM observers. I would tend to the second solution. Though, I
> would prefer to have Chris' opinion here.
From Chris and your comment, I think I will use ObserverList<SystemTimeChange> to implement the list.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #33)
> Whether or not the DOM API exposes the reason for time change, I don't see
> any reason for hal not to. But another solution is to use
> ObserverList<void_t>, which I would strongly prefer over observer service.
Chris, thanks for your comment.
Assignee | ||
Comment 35•13 years ago
|
||
Attachment #627903 -
Attachment is obsolete: true
Attachment #629718 -
Flags: review?(mounir)
Assignee | ||
Updated•13 years ago
|
Attachment #629718 -
Flags: review?(mounir)
Comment 36•13 years ago
|
||
Steven, did you cancel the review request on purpose?
Assignee | ||
Comment 37•13 years ago
|
||
Hi Mounir, yes, I updated but found an error. So I cancel the request and I will update a new one. Sorry for bothering you.
Assignee | ||
Comment 38•13 years ago
|
||
Attachment #623916 -
Attachment is obsolete: true
Attachment #629718 -
Attachment is obsolete: true
Attachment #630041 -
Flags: review?(mounir)
Comment 39•13 years ago
|
||
Comment on attachment 630041 [details] [diff] [review]
Implement the observer that will notify apps when system changes - V4
Review of attachment 630041 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the questions answered and the Notify() calls being sync unless there is a good reason to not do that.
::: hal/Hal.cpp
@@ +409,5 @@
> AssertMainThread();
> RETURN_PROXY_IF_SANDBOXED(GetLight(light, aConfig));
> }
>
> +typedef mozilla::ObserverList<SystemTimeChange> SystemTimeChangeObserver;
I believe |mozilla::| isn't needed, we are in the mozilla namespace already.
@@ +411,5 @@
> }
>
> +typedef mozilla::ObserverList<SystemTimeChange> SystemTimeChangeObserver;
> +
> +static SystemTimeChangeObserver sSystemTimeObserver;
You could do:
static ObserverList<SystemTimeChange> sSystemTimeObserver;
But it's up to you.
::: hal/HalTypes.h
@@ +79,2 @@
> }
> }
nit:
As long as you are here, change:
}
}
to:
} // namespace hal
} // namespace mozilla
::: hal/gonk/GonkHal.cpp
@@ +564,5 @@
>
> +class SystemTimeChangeRunnable : public nsRunnable
> +{
> +public:
> + SystemTimeChangeRunnable(const hal::SystemTimeChange& aReason) : mReason(aReason)
SystemTimeChangeRunnable(const hal::SystemTimeChange& aReason)
: mReason(aReason)
{}
@@ +602,5 @@
> now.tv_nsec += NsecPerSec;
> now.tv_sec -= 1;
> }
> // we need to have root privilege.
> sys_clock_settime(CLOCK_REALTIME, &now);
Is it possible that this method doesn't change the time as expected?
@@ +604,5 @@
> }
> // we need to have root privilege.
> sys_clock_settime(CLOCK_REALTIME, &now);
> +
> + NS_DispatchToMainThread(new SystemTimeChangeRunnable(hal::SYS_TIME_CHANGE_CLOCK));
Why are you making this call async? Seems like calling Notify() from here would just work.
@@ +612,5 @@
> +IsSameTimeZone(const nsCString& aTimezoneSpec)
> +{
> + char timezone[32];
> + property_get("persist.sys.timezone", timezone, "");
> + if (aTimezoneSpec.EqualsASCII(timezone)) {
return aTimezoneSpec.EqualsASCII(timezone);
@@ +630,5 @@
> property_set("persist.sys.timezone", aTimezoneSpec.get());
> // this function is automatically called by the other time conversion
> // functions that depend on the timezone. To be safe, we call it manually.
> tzset();
> + NS_DispatchToMainThread(new SystemTimeChangeRunnable(hal::SYS_TIME_CHANGE_TZ));
Same as above.
Attachment #630041 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 40•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #39)
> Comment on attachment 630041 [details] [diff] [review]
> Implement the observer that will notify apps when system changes - V4
>
> Review of attachment 630041 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r=me with the questions answered and the Notify() calls being sync unless
> there is a good reason to not do that.
>
> @@ +602,5 @@
> > now.tv_nsec += NsecPerSec;
> > now.tv_sec -= 1;
> > }
> > // we need to have root privilege.
> > sys_clock_settime(CLOCK_REALTIME, &now);
>
> Is it possible that this method doesn't change the time as expected?
It could return fail. According to man page, there are 3 possible errors, EFAULT, EINVAL, and EPERM. I will use MOZ_NOT_REACHED to catch the error.
>
> @@ +604,5 @@
> > }
> > // we need to have root privilege.
> > sys_clock_settime(CLOCK_REALTIME, &now);
> > +
> > + NS_DispatchToMainThread(new SystemTimeChangeRunnable(hal::SYS_TIME_CHANGE_CLOCK));
>
> Why are you making this call async? Seems like calling Notify() from here
> would just work.
OK, I will call Notify() directly.
Assignee | ||
Comment 41•13 years ago
|
||
Attachment #627905 -
Attachment is obsolete: true
Attachment #630459 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 42•13 years ago
|
||
Hi Mounir,
Please check the updated patch, thanks~
Attachment #630041 -
Attachment is obsolete: true
Attachment #630460 -
Flags: review+
Comment 43•13 years ago
|
||
Comment on attachment 630459 [details] [diff] [review]
Dispatch systemtimechange event
Review of attachment 630459 [details] [diff] [review]:
-----------------------------------------------------------------
Please, can you describe which API you are implementing here? It doesn't look like what Jonas, you and me agreed on.
Attachment #630459 -
Flags: review?(bent.mozilla) → review-
Comment 44•13 years ago
|
||
Comment on attachment 630460 [details] [diff] [review]
Implement the observer that will notify apps when system changes - V4.1
Review of attachment 630460 [details] [diff] [review]:
-----------------------------------------------------------------
I would like to see a new patch with the error handling fixed and also an answer to my question regarding timezone values.
::: hal/gonk/GonkHal.cpp
@@ +588,5 @@
> now.tv_sec -= 1;
> }
> // we need to have root privilege.
> + if (sys_clock_settime(CLOCK_REALTIME, &now) != 0) {
> + MOZ_NOT_REACHED("sys_clock_settime failed");
Please, don't use MOZ_NOT_REACHED. That error should be used when you should never reach a part of code like in a 'default' switch statement if all expected values have been handled in 'case' statements.
Here, you might want to use NS_ERROR() because even MOZ_ASSERT wouldn't be appropriate because a failure could happen.
Call |return;| after calling NS_ERROR().
@@ +613,1 @@
> property_set("persist.sys.timezone", aTimezoneSpec.get());
Sorry if I come back late here but when does your code check if the timezone value is okay? Can SetTimezone be called with "foo/bar"?
Attachment #630460 -
Flags: review+ → review-
Assignee | ||
Comment 45•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #44)
> Comment on attachment 630460 [details] [diff] [review]
> Implement the observer that will notify apps when system changes - V4.1
>
> Review of attachment 630460 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I would like to see a new patch with the error handling fixed and also an
> answer to my question regarding timezone values.
>
> ::: hal/gonk/GonkHal.cpp
> @@ +588,5 @@
> > now.tv_sec -= 1;
> > }
> > // we need to have root privilege.
> > + if (sys_clock_settime(CLOCK_REALTIME, &now) != 0) {
> > + MOZ_NOT_REACHED("sys_clock_settime failed");
>
> Please, don't use MOZ_NOT_REACHED. That error should be used when you should
> never reach a part of code like in a 'default' switch statement if all
> expected values have been handled in 'case' statements.
> Here, you might want to use NS_ERROR() because even MOZ_ASSERT wouldn't be
> appropriate because a failure could happen.
>
> Call |return;| after calling NS_ERROR().
OK~ I see. I will change the error handling here.
>
> @@ +613,1 @@
> > property_set("persist.sys.timezone", aTimezoneSpec.get());
>
> Sorry if I come back late here but when does your code check if the timezone
> value is okay? Can SetTimezone be called with "foo/bar"?
I did a test. I sent the following command through adb, |setprop persist.sys.timezone foo|. Then I get the value through, |getprop persist.sys.timezone|. The returned value is "foo".
In my opinion, I think timezone value should be filtered by UI. Most of the users do not set the timezone by string, such as "America/Los_Angeles". They choose the timezone from a list. If I need to do error handling here, I need a timezone table and check. It's not difficult but I think we should put the responsibility on UI.
Comment 46•13 years ago
|
||
(In reply to StevenLee from comment #45)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #44)
> > @@ +613,1 @@
> > > property_set("persist.sys.timezone", aTimezoneSpec.get());
> >
> > Sorry if I come back late here but when does your code check if the timezone
> > value is okay? Can SetTimezone be called with "foo/bar"?
> I did a test. I sent the following command through adb, |setprop
> persist.sys.timezone foo|. Then I get the value through, |getprop
> persist.sys.timezone|. The returned value is "foo".
> In my opinion, I think timezone value should be filtered by UI. Most of the
> users do not set the timezone by string, such as "America/Los_Angeles". They
> choose the timezone from a list. If I need to do error handling here, I need
> a timezone table and check. It's not difficult but I think we should put the
> responsibility on UI.
And how does your system time change if you set the timezone to "foo"?
So, one good thing is that there will be no direct DOM API to change the timezone. However, the Settings API will allow that and I'm not sure we should rely on UIs to show correct values. Maybe the Settings API should ignore invalid values when the caller tries to set a timezone. I will be okay to have this handled in a follow-up though.
Assignee | ||
Comment 47•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #46)
> And how does your system time change if you set the timezone to "foo"?
> So, one good thing is that there will be no direct DOM API to change the
> timezone. However, the Settings API will allow that and I'm not sure we
> should rely on UIs to show correct values. Maybe the Settings API should
> ignore invalid values when the caller tries to set a timezone. I will be
> okay to have this handled in a follow-up though.
If the input value is not correct, system time will change to GMT. But the clock on Gaia does not change. I also found that the timezone can be set via string, "America/Los_Angeles", or UTC offset, -7.
Comment 48•13 years ago
|
||
(In reply to StevenLee from comment #47)
> If the input value is not correct, system time will change to GMT. But the
> clock on Gaia does not change.
Is that because Gaia doesn't listen for time/tz change yet?
> I also found that the timezone can be set via
> string, "America/Los_Angeles", or UTC offset, -7.
That's interesting but unfortunately, UTC offset can't really be used because a TZ might have different offsets depending on the time of the year. For example "Europe/Paris" will be UTC-1 or UTC-2 depending on if the TZ is currently in daylight saving or not.
Assignee | ||
Comment 49•13 years ago
|
||
Attachment #630460 -
Attachment is obsolete: true
Attachment #630516 -
Flags: review?(mounir)
Updated•13 years ago
|
Attachment #630516 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 50•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #43)
> Comment on attachment 630459 [details] [diff] [review]
> Dispatch systemtimechange event
>
> Review of attachment 630459 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Please, can you describe which API you are implementing here? It doesn't
> look like what Jonas, you and me agreed on.
Hi Mounir,
Did you mean http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/f382abb068abd4c5 ?
I thought this API is for Bug 714357, is that right?
From comment #18, I thought that I should change my original interface. The old one implements a new DOMEvent and it also contains a data member. The new version uses the existing DOMEvent to inform that system time or timezone is changed.
If I misunderstand anything, please tell me.
Thanks~
Comment 51•13 years ago
|
||
(In reply to StevenLee from comment #50)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #43)
> > Comment on attachment 630459 [details] [diff] [review]
> > Dispatch systemtimechange event
> >
> > Review of attachment 630459 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > Please, can you describe which API you are implementing here? It doesn't
> > look like what Jonas, you and me agreed on.
> Hi Mounir,
> Did you mean
> http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/
> f382abb068abd4c5 ?
> I thought this API is for Bug 714357, is that right?
This API is for this bug and bug 714357. Both bug will implement a part. They could be merged actually.
> From comment #18, I thought that I should change my original interface. The
> old one implements a new DOMEvent and it also contains a data member. The
> new version uses the existing DOMEvent to inform that system time or
> timezone is changed.
Indeed, you need one event to inform that there is a change it time/timezone. However, you don't need to create a new DOMEvent class for that. You can simply send an event as explained in comment 32. Also, the event should be sent to navigator.mozTime.
Assignee | ||
Comment 52•13 years ago
|
||
Hi Mounir,
This patch is the interface I wrote from the mail list. Please help me check if it is what you thought.
Thanks~
Attachment #630895 -
Flags: review?(mounir)
Comment 53•13 years ago
|
||
Comment on attachment 630895 [details] [diff] [review]
Time manager interface
Review of attachment 630895 [details] [diff] [review]:
-----------------------------------------------------------------
Please, have those IDLs in dom/time/.
::: dom/interfaces/base/nsIDOMTimeManager.idl
@@ +11,5 @@
> +{
> + // set time by date
> + void setByDate(in jsval time);
> + // set time by UTC seconds
> + void setByUTCSec(in double time);
Do:
void set(in jsval time);
Then you can just check what was passed in the implementation.
@@ +13,5 @@
> + void setByDate(in jsval time);
> + // set time by UTC seconds
> + void setByUTCSec(in double time);
> +
> + attribute nsIDOMEventListener onTimeChange;
ontimechange
Attachment #630895 -
Flags: review?(mounir)
Assignee | ||
Comment 54•13 years ago
|
||
Attachment #630895 -
Attachment is obsolete: true
Attachment #631014 -
Flags: review?(mounir)
Comment 55•13 years ago
|
||
Comment on attachment 631014 [details] [diff] [review]
Time manager interface V2
Review of attachment 631014 [details] [diff] [review]:
-----------------------------------------------------------------
f+ with the changes applied.
::: dom/time/Makefile.in
@@ +15,5 @@
> +FORCE_STATIC_LIB = 1
> +
> +include $(topsrcdir)/dom/dom-config.mk
> +
> +#EXPORTS_NAMESPACES = mozilla/dom/time
Remove that if you don't need it.
@@ +17,5 @@
> +include $(topsrcdir)/dom/dom-config.mk
> +
> +#EXPORTS_NAMESPACES = mozilla/dom/time
> +
> +CPPSRCS = $(NULL)
ditto
@@ +18,5 @@
> +
> +#EXPORTS_NAMESPACES = mozilla/dom/time
> +
> +CPPSRCS = $(NULL)
> +
Remove this empty line.
@@ +26,5 @@
> + nsIDOMTimeManager.idl \
> + $(NULL)
> +
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk
Don't think you need ipc/chromium/...
::: dom/time/nsIDOMTimeManager.idl
@@ +9,5 @@
> +[scriptable, builtinclass, uuid(d29beaaa-bd54-4fd5-9f18-e0eedb1dc96d)]
> +interface nsIDOMMozTimeManager : nsIDOMEventTarget
> +{
> + // jsval could be Date object or UTC seconds
> + void setByDate(in jsval time);
void set(in jsval time);
Attachment #631014 -
Flags: review?(mounir) → feedback+
Assignee | ||
Comment 56•12 years ago
|
||
Attachment #631014 -
Attachment is obsolete: true
Attachment #631792 -
Flags: superreview?(jonas)
Attachment #631792 -
Flags: feedback+
Updated•12 years ago
|
Keywords: dev-doc-needed
Hmmm.. might it be easier to simply fire the timechange event on the window object? That way we can keep the privileged .set() function separate from the unprivileged timechange event.
Mounir?
Comment 58•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) Vacation June 11-22 from comment #57)
> Hmmm.. might it be easier to simply fire the timechange event on the window
> object? That way we can keep the privileged .set() function separate from
> the unprivileged timechange event.
What about the navigator object instead?
Comment 59•12 years ago
|
||
Hi Steven and Mounir,
The timezone-change observer is an essential part of Alarm API. Since it seems the upper-layer event things still need more confirmations, is that possible for you to check in the fundamental hal/gonk part first if it is already good to go? I'll also try to integrate this part into the Alarm API myself to test its performance.
Thanks,
Gene
Given that the navigator object isn't an EventTarget in other browsers, we might have an easier time simply firing the event on the Window object, so I think we should do that.
Comment 61•12 years ago
|
||
I'm told B2G wants bug 285615, which means they want this bug.
- Is there an ETA?
- For bug 285615, we're going to need to call a JSAPI function on every instance of the JS engine when the timezone changes. What's the most direct path to that from the notification system being built here?
Comment 62•12 years ago
|
||
Comment on attachment 631792 [details] [diff] [review]
Time manager interface V3
Review of attachment 631792 [details] [diff] [review]:
-----------------------------------------------------------------
Also, add the Makefile to toolkit/toolkit-makefiles.sh
And make the event happens on the nsIDOMWindow (dom/interfaces/base/nsIDOMWindow.idl).
Ask me for a super-review with those changes.
::: dom/time/Makefile.in
@@ +15,5 @@
> +FORCE_STATIC_LIB = 1
> +
> +include $(topsrcdir)/dom/dom-config.mk
> +
> +CPPSRCS = $(NULL)
nit: no need to put so many spaces.
@@ +19,5 @@
> +CPPSRCS = $(NULL)
> +
> +XPIDLSRCS = \
> + nsIDOMNavigatorTime.idl \
> + nsIDOMTimeManager.idl \
nit: indentation issues, leave only two spaces.
Attachment #631792 -
Flags: superreview?(jonas)
Comment 63•12 years ago
|
||
Steven, are you going to be able to look at this bug within the next few days?
Assignee | ||
Comment 64•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #62)
> Comment on attachment 631792 [details] [diff] [review]
> Time manager interface V3
>
> Review of attachment 631792 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Also, add the Makefile to toolkit/toolkit-makefiles.sh
> And make the event happens on the nsIDOMWindow
> (dom/interfaces/base/nsIDOMWindow.idl).
Hi Mounir,
Should I keep the original interface? Or I just send events in nsIDOMWindow?
Thanks~
Assignee | ||
Comment 65•12 years ago
|
||
Hi Mounir,
Is the interface correct? Apps call setTime from nsIDOMWinow and TimeChange event will be sent to nsIDOMWindow.
Attachment #631792 -
Attachment is obsolete: true
Attachment #638632 -
Flags: superreview?(mounir)
Comment 66•12 years ago
|
||
Comment on attachment 638632 [details] [diff] [review]
Interface
Review of attachment 638632 [details] [diff] [review]:
-----------------------------------------------------------------
set() should be in navigator.time and the event on window.
Attachment #638632 -
Flags: superreview?(mounir) → superreview-
Assignee | ||
Comment 67•12 years ago
|
||
Attachment #638632 -
Attachment is obsolete: true
Attachment #638638 -
Flags: superreview?(mounir)
Comment 68•12 years ago
|
||
Comment on attachment 638638 [details] [diff] [review]
Time manager interface V4
Review of attachment 638638 [details] [diff] [review]:
-----------------------------------------------------------------
sr=me with the requested changes.
::: dom/interfaces/base/nsIDOMWindow.idl
@@ +496,5 @@
> [implicit_jscontext] attribute jsval ondeviceorientation;
> [implicit_jscontext] attribute jsval ondeviceproximity;
> [implicit_jscontext] attribute jsval onuserproximity;
> [implicit_jscontext] attribute jsval ondevicelight;
> + [implicit_jscontext] attribute jsval ontimechange;
onmoztimechange
::: dom/time/Makefile.in
@@ +18,5 @@
> +
> +CPPSRCS = $(NULL)
> +
> +XPIDLSRCS = \
> + nsIDOMNavigatorTime.idl \
nit: indentation
::: dom/time/nsIDOMTimeManager.idl
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsIDOMEventTarget.idl"
> +
> +interface nsIDOMEventListener;
no need for that anymore
Attachment #638638 -
Flags: superreview?(mounir) → superreview+
Updated•12 years ago
|
Component: DOM: Events → DOM: Device Interfaces
QA Contact: events → device-interfaces
Assignee | ||
Comment 69•12 years ago
|
||
Attachment #638638 -
Attachment is obsolete: true
Attachment #638939 -
Flags: superreview?(mounir)
Updated•12 years ago
|
Attachment #638939 -
Flags: superreview?(mounir) → superreview+
Assignee | ||
Comment 70•12 years ago
|
||
Attachment #630459 -
Attachment is obsolete: true
Attachment #640606 -
Flags: review?(mounir)
Comment 71•12 years ago
|
||
Comment on attachment 640606 [details] [diff] [review]
TimeManager implementation
Mounir, do you want me to do this review? I may have more cycles than you at this point.
Comment 72•12 years ago
|
||
Comment on attachment 640606 [details] [diff] [review]
TimeManager implementation
Review of attachment 640606 [details] [diff] [review]:
-----------------------------------------------------------------
I will take your offer :)
Attachment #640606 -
Flags: review?(mounir) → review?(justin.lebar+bug)
Comment 73•12 years ago
|
||
It's an aesthetic thing, but can we put this code in mozilla::dom instead of adding a new mozilla::dom::time namespace? Ehsan wrote to the list a while ago about how we have too many namespaces. I don't entirely agree with that, but in this case, the namespace adds very little value.
Comment 74•12 years ago
|
||
Comment on attachment 640606 [details] [diff] [review]
TimeManager implementation
>+NS_IMETHODIMP
>+Navigator::GetMozTime(nsIDOMMozTimeManager** aTime)
>+{
>+ if (!mTimeManager) {
>+ *aTime = nsnull;
>+ nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
>+ NS_ENSURE_TRUE(win && win->GetDocShell(), NS_OK);
Why are you checking that the window has a docshell?
Also, shouldn't there be a permission check somewhere?
>diff --git a/dom/time/TimeManager.cpp b/dom/time/TimeManager.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/TimeManager.cpp
>@@ -0,0 +1,106 @@
>+#include <android/log.h>
Surely you don't want to include this unconditionally?
>+#include "jsapi.h"
>+#include "mozilla/Hal.h"
>+#include "nsDOMEvent.h"
>+#include "nsDOMEventTargetHelper.h"
>+#include "nsIDOMClassInfo.h"
>+#include "prtime.h"
>+#include "TimeManager.h"
>+
>+using namespace mozilla::hal;
>+
>+#define SYSTEM_TIME_CHANGE_EVENT_NAME NS_LITERAL_STRING("systemtimechange")
You use this in just one place; I'd prefer that you just inlined the string.
>+#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "TimeManager" , ## args)
Similarly, this should be guarded by ifdefs, if you were going to have it at
all. But you don't use it, and anyway if you did, we'd certainly want to use
PRLog or something more general so we'd have logging on other platforms.
>+DOMCI_DATA(MozTimeManager, mozilla::dom::time::TimeManager)
>+
>+namespace mozilla {
>+namespace dom {
>+namespace time {
This namespace is pretty useless, but it matches the existing code, so OK. :)
>+TimeManager::TimeManager() {
>+}
>+
>+NS_IMPL_EVENT_HANDLER(TimeManager, timechange)
>+
>+void
>+TimeManager::Init(nsPIDOMWindow* aWindow) {
>+ BindToOwner(aWindow);
>+ hal::RegisterSystemTimeChangeNotify(this);
>+}
>+
>+void
>+TimeManager::Shutdown() {
>+ hal::UnregisterSystemTimeChangeNotify(this);
>+}
Please make the destructor ensure that this is called. Note that
ObserverList<> doesn't keep strong references to the objects (see
xpcom/glue/Observer.h), so if the destructor doesn't remove this object from
the list, forgetting to call Shutdown() would result in a dangling pointer!
In the case of this object, it seems that you could get rid of ::Shutdown
altogether and rely on the destructor to unregister the notifications.
>+nsresult
>+TimeManager::DispatchTrustedEventToSelf() {
>+ nsRefPtr<nsDOMEvent> event = new nsDOMEvent(nsnull, nsnull);
>+ nsresult rv = event->InitEvent(SYSTEM_TIME_CHANGE_EVENT_NAME, false, false);
Could you do me a favor and comment what the booleans mean?
InitEvent(..., /* bubbles = */ false, /* cancelable = */ false);
>From the idl file:
>+interface nsIDOMMozTimeManager : nsISupports
>+{
>+ // jsval could be Date object or UTC seconds
>+ void set(in jsval time);
>+};
It sounds here like you mean that |time| is an absolute time, not relative to
the current time. I think that's the correct API.
Please update the documentation here to elaborate that
- "UTC seconds" means "seconds since the epoch, midnight UTC on January 1, 1970".
- If |time| is a date object, set(time) is equivalent to set(time.getTime()).
(In particular, one might think that set(time) would set the system timezone
or somesuch, but it doesn't.)
>+nsresult
>+TimeManager::Set(const JS::Value& date, JSContext* ctx) {
>+ double nowMSec = JS_Now() / 1000;
... but then here, the time is interpreted as relative to the current time!
That doesn't seem right.
>+ double dateMSec;
>+
>+ if (date.isObject()) {
>+ JSObject* dateObj = JSVAL_TO_OBJECT(date);
>+ jsval jsvalUTC;
>+
>+ if (JS_CallFunctionName(ctx, dateObj, "getTime", 0, nsnull, &jsvalUTC) != JS_TRUE) {
>+ NS_ERROR("JS_CallFunctionName fail");
>+ }
>+ ToNumber(ctx, jsvalUTC, &dateMSec);
So this is kind of weird -- we'll call getTime on any object, but you only mean to call it on date objects.
Searching through the code for JS_ObjectIsDate, I found the following snippet in nsDeviceStorage:
JSObject* obj = JSVAL_TO_OBJECT(params.since);
if (JS_ObjectIsDate(aCx, obj) && js_DateIsValid(aCx, obj)) {
result = js_DateGetMsecSinceEpoch(aCx, obj);
I think you want to do that.
>+ } else if (date.isNumber()) {
>+ JS_ValueToNumber(ctx, date, &dateMSec);
>+ } else {
>+ return NS_ERROR_INVALID_ARG;
>+ }
>+
>+ hal::AdjustSystemClock(JS_DoubleToInt32(dateMSec - nowMSec));
>+ return NS_OK;
>diff --git a/dom/time/TimeManager.h b/dom/time/TimeManager.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/TimeManager.h
>@@ -0,0 +1,52 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_time_TimeManager_h
>+#define mozilla_dom_time_TimeManager_h
>+
>+#include "mozilla/HalTypes.h"
>+#include "nsIDOMTimeManager.h"
>+#include "nsDOMEventTargetHelper.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "mozilla/Observer.h"
>+#include "nsDOMEventTargetHelper.h"
>+
>+class nsPIDOMWindow;
>+
>+namespace mozilla {
>+
>+typedef Observer<hal::SystemTimeChange> SystemTimeObserver;
I think this typedef should go inside the mozilla::dom::time namespace, unless
you're copying code from somewhere else.
>+
>+namespace dom {
>+namespace time {
>+class TimeManager : public nsDOMEventTargetHelper,
It's not clear to me why you're using nsDOMEventTargetHelper. That class has a lot of things you don't need.
Instead, couldn't you just fire the event onto the window yourself? See how code uses nsContentUtils::DispatchTrustedEvent().
>+ public nsIDOMMozTimeManager,
>+ public SystemTimeObserver
>+{
>+public:
>+ NS_DECL_ISUPPORTS
>+ NS_DECL_NSIDOMMOZTIMEMANAGER
>+ NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::)
>+
>+ TimeManager();
>+ void Init(nsPIDOMWindow *aWindow);
If you didn't inherit from nsDOMEventTargetHelper, you could pass in the window to the constructor. You also could take an nsIDOMWindow instead of an nsPIDOMWindow.
>+ void Shutdown();
>+
>+ // For IObserver.
>+ void Notify(const hal::SystemTimeChange& aReason);
You mean, for SystemTimeObserver? Also, this should be virtual.
>+
>+ nsresult DispatchTrustedEventToSelf();
Why is this public?
Let's take this for another round.
Attachment #640606 -
Flags: review?(justin.lebar+bug) → review-
Assignee | ||
Comment 75•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #73)
> It's an aesthetic thing, but can we put this code in mozilla::dom instead of
> adding a new mozilla::dom::time namespace? Ehsan wrote to the list a while
> ago about how we have too many namespaces. I don't entirely agree with
> that, but in this case, the namespace adds very little value.
Should I delete mozilla::dom::time? I traced some folders in dom and found that they have this kind of namespace architecture. So that I added this namespace. If it is not needed, I will delete it in next version.
Comment 76•12 years ago
|
||
> Should I delete mozilla::dom::time?
No, I realized during the review that, as you say, it matches the existing namespace structure. Which I think is silly, but whatever. :)
Assignee | ||
Comment 77•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #74)
> Comment on attachment 640606 [details] [diff] [review]
> TimeManager implementation
>
> >+NS_IMETHODIMP
> >+Navigator::GetMozTime(nsIDOMMozTimeManager** aTime)
> >+{
> >+ if (!mTimeManager) {
> >+ *aTime = nsnull;
> >+ nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
> >+ NS_ENSURE_TRUE(win && win->GetDocShell(), NS_OK);
>
> Why are you checking that the window has a docshell?
>
> Also, shouldn't there be a permission check somewhere?
I copied the code from battery things. So that I do not need docshell here, right?
I don't know how to check permission. Can you give me some key words that I can search?
Thanks~
Comment 78•12 years ago
|
||
> I copied the code from battery things. So that I do not need docshell here, right?
Correct, you don't need to ensure that there's a docshell. (FWIW Navigator::GetMozPower doesn't check for the docshell either.)
> I don't know how to check permission. Can you give me some key words that I can search?
See PowerManager::CheckPermission(). You can either check for permission in GetMozTime (as I suggested in the review) or in the ::Set method (like PowerManager does). Either way is fine with me.
Assignee | ||
Comment 79•12 years ago
|
||
Attachment #640606 -
Attachment is obsolete: true
Attachment #640994 -
Flags: review?(justin.lebar+bug)
Comment 80•12 years ago
|
||
Comment on attachment 640994 [details] [diff] [review]
TimeManager implementation V2
>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp
>@@ -410,16 +413,17 @@ nsEventListenerManager::RemoveEventListe
> --count;
> mNoListenerForEvent = NS_EVENT_TYPE_NULL;
> mNoListenerForEventAtom = nsnull;
>
> if (!deviceType) {
> return;
> }
> --typeCount;
>+ DisableTimeChangeNotify();
If content does
window.addEventListener('moztimechanged', function foo() { ... });
window.addEventListener('moztimechanged', function bar() { ... });
window.removeEventListener('moztimechanged', bar);
then this code will disable timechange events on the window, even though
there's still one event listener, right? Compare this with how existing
"device" event types are handled.
>diff --git a/dom/base/Navigator.cpp b/dom/base/Navigator.cpp
>--- a/dom/base/Navigator.cpp
>+++ b/dom/base/Navigator.cpp
>+//*****************************************************************************
>+// Navigator::nsIDOMNavigatorTime
>+//*****************************************************************************
>+NS_IMETHODIMP
>+Navigator::GetMozTime(nsIDOMMozTimeManager** aTime)
>+{
>+ if (!nsContentUtils::IsCallerChrome()) {
>+ return NS_ERROR_DOM_SECURITY_ERR;
>+ }
This security check limits this API to chrome code only, but don't you want to access this API from non-chrome code (i.e., Gaia)?
How have you been testing your code?
See PowerManager::CheckPermission, and note that one has permission to touch the power manager if the caller is chrome /or/ if the window's URI is part of a whitelist.
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -217,16 +217,17 @@
> #include "mozAutoDocUpdate.h"
>
> #include "mozilla/Telemetry.h"
> #include "nsLocation.h"
> #include "nsWrapperCacheInlines.h"
> #include "nsDOMEventTargetHelper.h"
> #include "nsIAppsService.h"
> #include "prrng.h"
>+#include "TimeChangeObserver.h"
>
> #ifdef ANDROID
> #include <android/log.h>
> #endif
>
> #ifdef PR_LOGGING
> static PRLogModuleInfo* gDOMLeakPRLog;
> #endif
>@@ -10602,16 +10603,30 @@ nsGlobalWindow::GetURL(nsIDOMMozURLPrope
> mURLProperty = new nsDOMMozURLProperty(this);
> }
>
> NS_ADDREF(*aURL = mURLProperty);
>
> return NS_OK;
> }
>
>+void
>+nsGlobalWindow::EnableTimeChangeNotify()
>+{
>+ nsSystemTimeChangeObserver* observer = nsSystemTimeChangeObserver::GetInstance();
Could you just do
nsSystemTimeChangeObserver::GetInstance()->AddWindowListener(this);
?
Also, nit: We try (and do not globally succeed) to have methods which can
return NULL start with "Get", and methods which can't return NULL leave off the
"Get". Since GetInstance here never returns NULL, we should call it
"Instance()", although I'd prefer "Singleton()" myself. (Whatever you prefer
is fine.)
>+ observer->AddWindowListener(this);
>+}
>+
>+void
>+nsGlobalWindow::DisableTimeChangeNotify()
>+{
>+ nsSystemTimeChangeObserver* observer = nsSystemTimeChangeObserver::GetInstance();
>+ observer->RemoveWindowListener(this);
>+}
I don't see this being called on window destruction; can you get dangling pointers here? See comments below.
>diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h
>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>@@ -524,16 +524,19 @@ public:
> virtual void SetReadyForFocus();
> virtual void PageHidden();
> virtual nsresult DispatchAsyncHashchange(nsIURI *aOldURI, nsIURI *aNewURI);
> virtual nsresult DispatchSyncPopState();
>
> virtual void EnableDeviceSensor(PRUint32 aType);
> virtual void DisableDeviceSensor(PRUint32 aType);
>
>+ virtual void EnableTimeChangeNotify();
>+ virtual void DisableTimeChangeNotify();
Nit: {Enable,Disable}TimeChangeNotifications would be a better name, I think.
>diff --git a/dom/time/TimeChangeObserver.cpp b/dom/time/TimeChangeObserver.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/TimeChangeObserver.cpp
>@@ -0,0 +1,78 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "TimeChangeObserver.h"
>+#include "nsPIDOMWindow.h"
>+#include "nsDOMEvent.h"
>+#include "nsContentUtils.h"
>+
>+using namespace mozilla::hal;
>+using namespace mozilla;
>+
>+static nsSystemTimeChangeObserver *sObserver;
>+
>+nsSystemTimeChangeObserver* nsSystemTimeChangeObserver::GetInstance()
>+{
>+ if (sObserver == NULL) {
As a matter of convention, we avoid explicit comparisons to NULL. Do instead
if (!sObserver).
>+ sObserver = new nsSystemTimeChangeObserver();
>+ }
>+ return sObserver;
>+}
>+
>+void
>+nsSystemTimeChangeObserver::Notify(const SystemTimeChange& aReason)
>+{
>+ for (PRUint32 i = mWindowListeners.Length(); i > 0 ; ) {
>+ --i;
>+ nsCOMPtr<nsPIDOMWindow> pwindow = do_QueryInterface(mWindowListeners[i]);
>+ if (!pwindow || !pwindow->GetOuterWindow()) {
>+ continue;
>+ }
We can check for null before putting the window into the array (in AddWindowListener). Is there a reason you're checking that the window has an outer window? Also, since you don't use pwindow later on, the QI is unnecessary; you should be able to remove this whole block.
>+ nsCOMPtr<nsIDOMDocument> domdoc;
>+ mWindowListeners[i]->GetDocument(getter_AddRefs(domdoc));
>+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(domdoc));
>+
>+ if (domdoc) {
>+ bool defaultActionEnabled = true;
>+ nsContentUtils::DispatchTrustedEvent(doc, mWindowListeners[i],
>+ NS_LITERAL_STRING("moztimechange"), /* bubbles = */ true,
>+ /* canceable = */ false, &defaultActionEnabled);
You can just leave off this last argument.
>+ }
>+ }
Notifying observers is the bane of many a DOM programmer. The problem is, your observer list can change /while you iterate over it/, because DispatchTrustedEvent ends up running code from the web. (Note too that windows can be destroyed during an event!)
Since timechange is inherently a time-sensitive event, I'm OK if we do the simple thing here and copy mWindowListeners at the beginning of Notify() and iterate over that. (So long as Mounir is OK with this.) But you'll still need to use a memory-safe reference to the windows, as windows might be destroyed during the loop.
>+}
>+
>+NS_IMETHODIMP
>+nsSystemTimeChangeObserver::AddWindowListener(nsIDOMWindow* aWindow)
>+{
Please add null-check on aWindow so that we don't have to check during nsSystemTimeChangeObserver.
>+ if (mWindowListeners.IndexOf(aWindow) != nsTArray<nsIDOMWindow*>::NoIndex ) {
>+ return NS_OK ;
>+ }
>+
>+ if (mWindowListeners.Length() == 0) {
>+ RegisterSystemTimeChangeNotify(sObserver);
I know sObserver == this, but could you please use |this| here? That would be clearer.
>+ }
>+
>+ mWindowListeners.AppendElement(aWindow);
>+ return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsSystemTimeChangeObserver::RemoveWindowListener(nsIDOMWindow *aWindow)
>+{
>+ if (mWindowListeners.IndexOf(aWindow) == nsTArray<nsIDOMWindow*>::NoIndex) {
>+ return NS_OK;
>+ }
>+
>+ mWindowListeners.RemoveElement(aWindow);
>+
>+ if (mWindowListeners.Length() == 0) {
>+ UnregisterSystemTimeChangeNotify(sObserver);
|this| would be clearer.
>+ delete sObserver;
>+ sObserver = NULL;
>+ }
Suppose code does the following:
nsSystemTimeChangeObserver* observer = nsSystemTimeChangeObserver::GetInstance();
observer->AddWindowListener(win);
observer->RemoveWindowListener(win); // now mWindowListeners.Length() == 0
observer->AddWindowListener(win);
During the RemoveWindowListener call, we just deleted |observer|, right? So now the last Add call is running on a dangling pointer.
A common solution to this is to leave singletons around until shutdown. You could do the following
nsAutoPtr<nsSystemTimeChangeObserver> sObserver;
GetInstance() {
if (!sObserver) {
sObserver = new nsSystemTimeChangeObserver();
ClearOnShutdown(&sObserver);
}
return sObserver;
}
An nsAutoPtr is an owning pointer; it deletes the thing it points to when it no longer points to that thing. And ClearOnShutdown registers an observer so that, at shutdown, we delete this object. That's important because otherwise our tools will likely flag a memory leak.
This solution comes at the cost of a static initializer (to construct
nsAutoPtr), which is becoming increasingly verboten. But I'm going to write a
patch so you can use a raw pointer here, and then we'll be able to get rid of
the static initializers. So for now, let's just use nsAutoPtr.
>diff --git a/dom/time/TimeChangeObserver.h b/dom/time/TimeChangeObserver.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/TimeChangeObserver.h
>@@ -0,0 +1,27 @@
>+class nsSystemTimeChangeObserver : public SystemTimeChangeObserver
>+{
>+public:
>+ static nsSystemTimeChangeObserver* GetInstance();
Since this is a singleton class, please make the constructor private.
>+ void Notify(const mozilla::hal::SystemTimeChange& aReason);
>+ NS_IMETHODIMP AddWindowListener(nsIDOMWindow *aWindow);
>+ NS_IMETHODIMP RemoveWindowListener(nsIDOMWindow *aWindow);
NS_IMETHODIMP is reserved for defining methods declared in IDL; that is, it's not used in class headers. NS_IMETHOD can be used in a class header, when you're declaring a method which is declared in IDL, but we usually use the NS_DECL_NSIFOOBAR macro for that.
What you want here and in the C++ file is a simple nsresult return value. Or, looking at the code, it never fails, so you could just return void.
>diff --git a/dom/time/TimeManager.cpp b/dom/time/TimeManager.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/TimeManager.cpp
>@@ -0,0 +1,59 @@
>+nsresult
>+TimeManager::Set(const JS::Value& date, JSContext* ctx) {
>+ double nowMSec = JS_Now() / 1000;
>+ double dateMSec;
>+
>+ if (date.isObject()) {
>+ JSObject* dateObj = JSVAL_TO_OBJECT(date);
>+
>+ if (JS_ObjectIsDate(ctx, dateObj) && js_DateIsValid(ctx, dateObj)) {
>+ dateMSec = js_DateGetMsecSinceEpoch(ctx, dateObj);
>+ }
>+ else {
>+ NS_ERROR("This is not a Date object");
I think NS_ERROR is too strong; that implies that hitting this is an error in
our code, which it isn't. Can you please make this a warning (and indicate
that it's hit when the object isn't a date, /or/ it's an invalid date)?
Alternatively you could remove it altogether.
>+ return NS_ERROR_INVALID_ARG;
>+ }
>+ } else if (date.isNumber()) {
>+ if (!JS_ValueToNumber(ctx, date, &dateMSec)) {
>+ NS_ERROR("JS_ValueToNumber fail");
>+ return NS_ERROR_FAILURE;
>+ }
Can you use JS::Value::toNumber() instead? See class Value in jsapi.h.
>+ hal::AdjustSystemClock(JS_DoubleToInt32(dateMSec - nowMSec));
Did you forget to change this after changing the IDL?
>diff --git a/dom/time/nsIDOMTimeManager.idl b/dom/time/nsIDOMTimeManager.idl
>--- a/dom/time/nsIDOMTimeManager.idl
>+++ b/dom/time/nsIDOMTimeManager.idl
>@@ -2,11 +2,15 @@
> * License, v. 2.0. If a copy of the MPL was not distributed with this
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> #include "nsISupports.idl"
>
> [scriptable, builtinclass, uuid(d29beaaa-bd54-4fd5-9f18-e0eedb1dc96d)]
> interface nsIDOMMozTimeManager : nsISupports
> {
>- // jsval could be Date object or UTC seconds
>+ /*
>+ * time could be
>+ * - "UTC seconds" means "seconds since the epoch, midnight UTC on January 1, 1970".
>+ * - If |time| is a date object, set(time) is equivalent to set(time.getTime()).
>+ */
> [implicit_jscontext] void set(in jsval time);
> };
Two general comments here:
1) This patch holds onto windows using raw pointers, mostly.
In general, we try to avoid raw pointers in Mozilla code, because they're error-prone. It's too easy to leave a dangling pointer around and cause a critical security bug. (This is evidenced by the two dangling pointer bugs in this patch!)
If we want to be safe and not use raw pointers, our options are to take strong references to the window (with nsCOMPtr), or hold weak references (with nsWeakPtr).
I'd prefer the latter, because weak references are not only safe from crashes, but they also won't cause a memory leak.
Unless there's a good reason to use raw pointers here, I'd like you to change the member variables that are currently nsIDOMWindow* to nsWeakPtr. See [1] for our docs.
2) I'd like to see automated tests for this, ideally before the patch lands. I know it's a pain (particularly for this sort of thing), but some of the bugs in this patch should have been caught by an automated test; I'm afraid there may be other bugs I haven't caught that you'll find with your test. And that's to say nothing of regressions...
This is going to have to be a B2G-specific automated test, and I don't know how to write those. malini and jgriffin are the experts, or you can try #b2g.
[1] https://developer.mozilla.org/en/Weak_reference
Attachment #640994 -
Flags: review?(justin.lebar+bug) → review-
Comment 81•12 years ago
|
||
> Notifying observers is the bane of many a DOM programmer. The problem is, your observer list can
> change /while you iterate over it/
See for example how nsDeviceSensors::Notify copies its mWindowListeners array into an nsCOMArray before iterating over it.
Comment 82•12 years ago
|
||
Looking through this code more, I see that GetInstance() is pretty common for the same kind of thing that you have. I shouldn't have suggested breaking convention and using Singleton(); sorry! (Or at least, we should change everything all at once...)
Assignee | ||
Comment 83•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #80)
> We can check for null before putting the window into the array (in
> AddWindowListener). Is there a reason you're checking that the window has
> an outer window? Also, since you don't use pwindow later on, the QI is
> unnecessary; you should be able to remove this whole block.
I thought that I should check outer window(This code is also from nsDeviceSensors).
I am not familiar with the knowledge about the window architecture and navigator.
Can you suggest some links that I can study?
>
>
> Notifying observers is the bane of many a DOM programmer. The problem is,
> your observer list can change /while you iterate over it/, because
> DispatchTrustedEvent ends up running code from the web. (Note too that
> windows can be destroyed during an event!)
>
> Since timechange is inherently a time-sensitive event, I'm OK if we do the
> simple thing here and copy mWindowListeners at the beginning of Notify() and
> iterate over that. (So long as Mounir is OK with this.) But you'll still
> need to use a memory-safe reference to the windows, as windows might be
> destroyed during the loop.
>
> A common solution to this is to leave singletons around until shutdown. You
> could do the following
>
> nsAutoPtr<nsSystemTimeChangeObserver> sObserver;
>
> GetInstance() {
> if (!sObserver) {
> sObserver = new nsSystemTimeChangeObserver();
> ClearOnShutdown(&sObserver);
> }
> return sObserver;
> }
>
> An nsAutoPtr is an owning pointer; it deletes the thing it points to when it
> no longer points to that thing. And ClearOnShutdown registers an observer
> so that, at shutdown, we delete this object. That's important because
> otherwise our tools will likely flag a memory leak.
>
> This solution comes at the cost of a static initializer (to construct
> nsAutoPtr), which is becoming increasingly verboten. But I'm going to write
> a
> patch so you can use a raw pointer here, and then we'll be able to get rid of
> the static initializers. So for now, let's just use nsAutoPtr.
>
It's really awesome!! I should use this way to handle the resource here.
> >+ hal::AdjustSystemClock(JS_DoubleToInt32(dateMSec - nowMSec));
>
> Did you forget to change this after changing the IDL?
I am not sure what you mean here. AdjustSystemClock can only input time difference
so that I should calculate it here.
>
> 2) I'd like to see automated tests for this, ideally before the patch lands.
> I know it's a pain (particularly for this sort of thing), but some of the
> bugs in this patch should have been caught by an automated test; I'm afraid
> there may be other bugs I haven't caught that you'll find with your test.
> And that's to say nothing of regressions...
>
> This is going to have to be a B2G-specific automated test, and I don't know
> how to write those. malini and jgriffin are the experts, or you can try
> #b2g.
>
Do you mean the tests like https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=749551&attachment=639599?
(In reply to Justin Lebar [:jlebar] from comment #81)
> > Notifying observers is the bane of many a DOM programmer. The problem is, your observer list can
> > change /while you iterate over it/
>
> See for example how nsDeviceSensors::Notify copies its mWindowListeners
> array into an nsCOMArray before iterating over it.
If I use nsWeakPtr to store mWindowListerners, should I copy them to an
nsCOMArray?
Many thanks for your reply.
Comment 84•12 years ago
|
||
> I thought that I should check outer window(This code is also from
> nsDeviceSensors).
nsDeviseSensors::Notify does
if (!pwindow ||
!pwindow->GetOuterWindow() ||
pwindow->GetOuterWindow()->IsBackground())
It has to check that there's an outer window because it wants to check
OuterWindow->IsBackground(). You're not doing something like that here, as far
as I can tell.
> I am not familiar with the knowledge about the window architecture and
> navigator. Can you suggest some links that I can study?
Unfortunately we don't have any good docs that I'm aware of.
To explain this inner/outer window business briefly (it's not at all obvious,
and I guess I should copy this somewhere):
<iframe> and tab has two nsGlobalWindow instances associated with it: An inner
and an outer window. When the page inside the iframe changes, the inner window
also changes, but the outer window stays the same.
When you do |var w = iframe.contentWindow;|, you're getting a reference to the
iframe's outer window. That's why |w| continues to point to the page currently
inside the iframe.
> It's really awesome!! I should use this way to handle the resource here.
The patch is in bug 772987; once that lands (it might be before this code
lands), you'll be able to replace |static nsAutoPtr| with |static
mozilla::StaticAutoPtr|, which will be great.
> > >+ hal::AdjustSystemClock(JS_DoubleToInt32(dateMSec - nowMSec));
> >
> > Did you forget to change this after changing the IDL?
>
> I am not sure what you mean here. AdjustSystemClock can only input time
> difference so that I should calculate it here.
Oh, I understand now. This code is right; sorry, my bad!
> Do you mean the tests like
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=749551&attachment=639599?
Those appear to be normal mochitests, so I don't think so.
The problem is, if we ran this code in a mochitest, we'd actually change the
system time on the test machine! We don't want to do that, so I think you
should write a test for the B2G emulator.
But again, I don't really know how this works, so you should find malini or
jgriffin and explain what you're trying to do.
> If I use nsWeakPtr to store mWindowListerners, should I copy them to an
> nsCOMArray?
You could, but it's not necessary, so long as you copy the array of weak
pointers. As I recall, that code copies into an nsCOMArray because it has an
array of raw pointers; it wants to make sure that none of the pointers becomes
stale while it's iterating over the array, so it takes a strong ref to all the
windows. But if you have weak refs to the windows, it's not a problem if one
of the windows gets free'd while you're iterating over your list.
Assignee | ||
Comment 85•12 years ago
|
||
Attachment #640994 -
Attachment is obsolete: true
Attachment #641797 -
Flags: review?(justin.lebar+bug)
Comment 86•12 years ago
|
||
Comment on attachment 641797 [details] [diff] [review]
TimeManager implementation V3
>@@ -1129,8 +1135,34 @@ nsEventListenerManager::UnmarkGrayJSList
> if (jsl) {
> xpc_UnmarkGrayObject(jsl->GetHandler());
> xpc_UnmarkGrayObject(jsl->GetEventScope());
> } else if (ls.mListenerType == eWrappedJSListener) {
> xpc_TryUnmarkWrappedGrayObject(ls.mListener);
> }
> }
> }
>+
>+void
>+nsEventListenerManager::EnableTimeChangeNotifications()
>+{
>+ nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mTarget);
>+
>+ if (!window) {
>+ return;
>+ }
Super nit: No newline before this |if|, if you please.
>+void
>+nsEventListenerManager::DisableTimeChangeNotifications()
>+{
>+ nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mTarget);
>+
>+ if (!window) {
>+ return;
>+ }
Also here.
>@@ -1347,16 +1358,34 @@ Navigator::OnNavigation()
> // Inform MediaManager in case there are live streams or pending callbacks.
> #ifdef MOZ_MEDIA_NAVIGATOR
> MediaManager *manager = MediaManager::Get();
> nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
> return manager->OnNavigation(win->WindowID());
> #endif
> }
>
>+bool
>+Navigator::CheckPermission(const char* aPref)
>+{
>+ if (!nsContentUtils::IsCallerChrome()) {
>+ nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>+ NS_ENSURE_TRUE(win, false);
>+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(win->GetExtantDocument());
>+ NS_ENSURE_TRUE(doc, false);
>+ nsCOMPtr<nsIURI> uri;
>+ doc->NodePrincipal()->GetURI(getter_AddRefs(uri));
>+ if (!nsContentUtils::URIIsChromeOrInPref(uri, aPref)) {
>+ return false;
>+ }
Nit: how about |return nsContentUtils::URIIsChromeOrInPref(uri, aPref)|?
>+ }
>+
>+ return true;
>+}
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -217,16 +217,17 @@
> #include "mozAutoDocUpdate.h"
>
> #include "mozilla/Telemetry.h"
> #include "nsLocation.h"
> #include "nsWrapperCacheInlines.h"
> #include "nsDOMEventTargetHelper.h"
> #include "nsIAppsService.h"
> #include "prrng.h"
>+#include "TimeChangeObserver.h"
>
> #ifdef ANDROID
> #include <android/log.h>
> #endif
>
> #ifdef PR_LOGGING
> static PRLogModuleInfo* gDOMLeakPRLog;
> #endif
>@@ -10602,16 +10603,28 @@ nsGlobalWindow::GetURL(nsIDOMMozURLPrope
> mURLProperty = new nsDOMMozURLProperty(this);
> }
>
> NS_ADDREF(*aURL = mURLProperty);
>
> return NS_OK;
> }
>
>+void
>+nsGlobalWindow::EnableTimeChangeNotifications()
>+{
>+ nsSystemTimeChangeObserver::GetInstance()->AddWindowListener(this);
>+}
>+
>+void
>+nsGlobalWindow::DisableTimeChangeNotifications()
>+{
>+ nsSystemTimeChangeObserver::GetInstance()->RemoveWindowListener(this);
>+}
You need to remove the window listener when the window is destroyed, right?
Even though you're using weak references, you don't want to leak them.
>diff --git a/dom/time/Makefile.in b/dom/time/Makefile.in
>--- a/dom/time/Makefile.in
>+++ b/dom/time/Makefile.in
>@@ -11,17 +11,25 @@ include $(DEPTH)/config/autoconf.mk
>-CPPSRCS = $(NULL)
>+CPPSRCS = \
>+ TimeManager.cpp \
>+ TimeChangeObserver.cpp \
>+ $(NULL)
>+
>+EXPORTS = \
>+ TimeChangeObserver.h \
>+ $(NULL)
Nit: Please match the other indentation in the Makefile.
>
> XPIDLSRCS = \
> nsIDOMNavigatorTime.idl \
> nsIDOMTimeManager.idl \
> $(NULL)
>
> include $(topsrcdir)/config/config.mk
>+include $(topsrcdir)/ipc/chromium/chromium-config.mk
I presume you actualy need this?
>diff --git a/dom/time/TimeChangeObserver.cpp b/dom/time/TimeChangeObserver.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/TimeChangeObserver.cpp
>@@ -0,0 +1,89 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "TimeChangeObserver.h"
>+#include "mozilla/ClearOnShutdown.h"
>+#include "nsPIDOMWindow.h"
>+#include "nsDOMEvent.h"
>+#include "nsContentUtils.h"
>+
>+using namespace mozilla::hal;
>+using namespace mozilla;
>+
>+nsAutoPtr<nsSystemTimeChangeObserver> sObserver;
>+
>+nsSystemTimeChangeObserver* nsSystemTimeChangeObserver::GetInstance()
>+{
>+ if (!sObserver) {
>+ sObserver = new nsSystemTimeChangeObserver();
>+ ClearOnShutdown(&sObserver);
>+ }
>+ return sObserver;
>+}
>+
>+void
>+nsSystemTimeChangeObserver::Notify(const SystemTimeChange& aReason)
>+{
>+ for (PRUint32 i = mWindowListeners.Length(); i > 0 ; ) {
>+ --i;
>+ nsCOMPtr<nsIDOMDocument> domdoc;
>+ nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(mWindowListeners[i]);
>+ if (!window) {
>+ return;
>+ }
>+
>+ window->GetDocument(getter_AddRefs(domdoc));
>+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(domdoc));
>+ if (!domdoc) {
>+ return;
>+ }
>+
>+ nsContentUtils::DispatchTrustedEvent(doc, window,
>+ NS_LITERAL_STRING("moztimechange"), /* bubbles = */ true,
>+ /* canceable = */ false);
>+ }
>+}
Remember we said you had to copy the list you iterate over while you iterate
over it, because the list might be modified during DispatchTrustedEvent, which
can run arbitrary content code.
Also, for good measure, you should remove any dead weak refs from the list.
You could do this while you copy the list (since you'd be doing
do_QueryReferent on each element, you might as well copy the windows into an
nsCOMArray, or an nsTArray<nsCOMPtr<T>>).
>+bool
>+nsSystemTimeChangeObserver::FindWindow(nsIDOMWindow* aWindow)
>+{
>+ return mWindowListeners.IndexOf(NS_GetWeakReference(aWindow)) !=
>+ nsTArray<nsIDOMWindow*>::NoIndex);
>+}
Since this doesn't return the index of the window, I don't think "FindWindow"
is a good name. Actually, I don't think you need this helper at all; see
below.
>+nsresult
>+nsSystemTimeChangeObserver::AddWindowListener(nsIDOMWindow* aWindow)
>+{
>+ if (!aWindow) {
>+ return NS_ERROR_ILLEGAL_VALUE;
>+ }
>+
>+ if (FindWindow(aWindow)) {
>+ return NS_OK;
>+ }
>+
>+ if (mWindowListeners.Length() == 0) {
>+ RegisterSystemTimeChangeNotify(this);
>+ }
>+
>+ mWindowListeners.AppendElement(NS_GetWeakReference(aWindow));
>+ return NS_OK;
>+}
>+
>+nsresult
>+nsSystemTimeChangeObserver::RemoveWindowListener(nsIDOMWindow *aWindow)
>+{
>+ if (!FindWindow(aWindow)) {
>+ return NS_OK;
>+ }
>+
>+ mWindowListeners.RemoveElement(NS_GetWeakReference(aWindow));
It's OK to do RemoveElement if the element isn't in the list. So you don't
need to check FindWindow above. And then you could inline FindWindow into
AddWindowListener...
>+
>+ if (mWindowListeners.Length() == 0) {
>+ UnregisterSystemTimeChangeNotify(this);
>+ }
>+
>+ return NS_OK;
>+}
We're almost there with this patch! r- because of the nsGlobalWindow changes
and the nsSystemTimeChangeObserver::Notify changes. But even once I r+ this
patch, I'd really like to see some tests before we land, if that's feasible.
Attachment #641797 -
Flags: review?(justin.lebar+bug) → review-
Comment 87•12 years ago
|
||
In case anyone missed it in bug 285615, Sean says:
"To resolve, the timezone change notification system from Bug 714358 must land (initially just for B2G), and then the event must be propagated through to JS_ClearDateCaches()."
Assignee | ||
Comment 88•12 years ago
|
||
Hi Justin,
Sorry for late reply. I was trapped in FM control support.
(In reply to Justin Lebar [:jlebar] from comment #86)
> >+void
> >+nsGlobalWindow::DisableTimeChangeNotifications()
> >+{
> >+ nsSystemTimeChangeObserver::GetInstance()->RemoveWindowListener(this);
> >+}
>
> You need to remove the window listener when the window is destroyed, right?
> Even though you're using weak references, you don't want to leak them.
>
> >diff --git a/dom/time/Makefile.in b/dom/time/Makefile.in
> >--- a/dom/time/Makefile.in
> >+++ b/dom/time/Makefile.in
> > XPIDLSRCS = \
> > nsIDOMNavigatorTime.idl \
> > nsIDOMTimeManager.idl \
> > $(NULL)
> >
> > include $(topsrcdir)/config/config.mk
> >+include $(topsrcdir)/ipc/chromium/chromium-config.mk
>
> I presume you actualy need this?
I need to include it. If not, I cannot include PHal.h and basictypes.h.
>
> >diff --git a/dom/time/TimeChangeObserver.cpp b/dom/time/TimeChangeObserver.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/time/TimeChangeObserver.cpp
> >@@ -0,0 +1,89 @@
> >+void
> >+nsSystemTimeChangeObserver::Notify(const SystemTimeChange& aReason)
> >+{
> >+ for (PRUint32 i = mWindowListeners.Length(); i > 0 ; ) {
> >+ --i;
> >+ nsCOMPtr<nsIDOMDocument> domdoc;
> >+ nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(mWindowListeners[i]);
> >+ if (!window) {
> >+ return;
> >+ }
> >+
> >+ window->GetDocument(getter_AddRefs(domdoc));
> >+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(domdoc));
> >+ if (!domdoc) {
> >+ return;
> >+ }
> >+
> >+ nsContentUtils::DispatchTrustedEvent(doc, window,
> >+ NS_LITERAL_STRING("moztimechange"), /* bubbles = */ true,
> >+ /* canceable = */ false);
> >+ }
> >+}
>
> Remember we said you had to copy the list you iterate over while you iterate
> over it, because the list might be modified during DispatchTrustedEvent,
> which
> can run arbitrary content code.
In the end of comment #84, I thought that I don't need to copy the windows.
Do I misunderstand it? If yes, I will copy the list in the next version.
>
> Also, for good measure, you should remove any dead weak refs from the list.
> You could do this while you copy the list (since you'd be doing
> do_QueryReferent on each element, you might as well copy the windows into an
> nsCOMArray, or an nsTArray<nsCOMPtr<T>>).
I will remove dear weak reference in the next version.
Comment 89•12 years ago
|
||
> In the end of comment #84, I thought that I don't need to copy the windows.
> Do I misunderstand it? If yes, I will copy the list in the next version.
Ah, it's a little unclear there.
You need to copy the array so that the array does not get modified as you iterate over it.
But what I was saying at the end of comment 84 was, because your array is of nsWeakRefs, not of raw pointers, you do not need to copy the array /to an nsCOMArray/ (or alternatively to nsTArray<nsCOMPtr<T>>>). Copying to strong pointers would ensure that no window gets destroyed as you iterate over the list, but since you have weak refs, it's safe if a window gets destroyed while you iterate.
However, I realized later that, since you need to clean up the weak refs, you might as well copy to an array of strong pointers while you're at it.
Does that make sense?
Assignee | ||
Comment 90•12 years ago
|
||
Attachment #641797 -
Attachment is obsolete: true
Attachment #645795 -
Flags: review?(justin.lebar+bug)
Comment 91•12 years ago
|
||
Comment on attachment 645795 [details] [diff] [review]
TimeManager implementation V4
Global nit: There's a lot of trailing whitespace in this patch. Git can fix it
up for you (git rebase --whitespace=fix). Otherwise you can sed over the patch...
>diff --git a/dom/time/TimeChangeObserver.cpp b/dom/time/TimeChangeObserver.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/TimeChangeObserver.cpp
>@@ -0,0 +1,87 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "TimeChangeObserver.h"
>+#include "mozilla/ClearOnShutdown.h"
>+#include "nsPIDOMWindow.h"
>+#include "nsDOMEvent.h"
>+#include "nsContentUtils.h"
>+
>+using namespace mozilla::hal;
>+using namespace mozilla;
>+
>+nsAutoPtr<nsSystemTimeChangeObserver> sObserver;
Make this StaticAutoPtr, please (I just landed this yesterday).
>+nsSystemTimeChangeObserver* nsSystemTimeChangeObserver::GetInstance()
>+{
>+ if (!sObserver) {
>+ sObserver = new nsSystemTimeChangeObserver();
>+ ClearOnShutdown(&sObserver);
>+ }
>+ return sObserver;
>+}
>+
>+void
>+nsSystemTimeChangeObserver::Notify(const SystemTimeChange& aReason)
>+{
>+ // Copy mWindowListeners for preventing that the array may be modified
>+ // by others when we iterate it.
"Copy mWindowListeners and iterate over windowListeners instead because
mWindowListeners may be modified while we loop." or something like that.
>+ nsTArray<nsWeakPtr> windowListeners;
>+ for (PRUint32 i = 0; i < mWindowListeners.Length(); i++) {
>+ windowListeners.AppendElement(mWindowListeners.SafeElementAt(i));
>+ }
>+
>+ for (PRUint32 i = windowListeners.Length(); i > 0 ; ) {
Nit: I'd prefer
for (i = length - 1; i >= 0; i--)
But at the very least, please remove the space before the final semicolon in
your loop.
>+ --i;
>+ nsCOMPtr<nsIDOMDocument> domdoc;
Nit: Please move domdoc down to immediately before it's used.
>+ nsCOMPtr<nsIDOMWindow> window = do_QueryReferent(windowListeners[i]);
>+ if (!window) {
>+ mWindowListeners.RemoveElement(windowListeners[i]);
>+ return;
>+ }
>+
>+ window->GetDocument(getter_AddRefs(domdoc));
>+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(domdoc));
>+ if (!domdoc) {
>+ return;
>+ }
>+
>+ nsContentUtils::DispatchTrustedEvent(doc, window,
>+ NS_LITERAL_STRING("moztimechange"), /* bubbles = */ true,
>+ /* canceable = */ false);
>+ }
>+}
>+nsresult
>+nsSystemTimeChangeObserver::AddWindowListener(nsIDOMWindow* aWindow)
>+{
>+ if (!aWindow) {
>+ return NS_ERROR_ILLEGAL_VALUE;
>+ }
>+
>+ if (mWindowListeners.IndexOf(NS_GetWeakReference(aWindow)) !=
>+ nsTArray<nsIDOMWindow*>::NoIndex) {
>+ return NS_OK;
>+ }
>+
>+ if (mWindowListeners.Length() == 0) {
>+ RegisterSystemTimeChangeNotify(this);
This should be RegisterSytemTimeObserver, to be consistent with the other methods in Hal.cpp.
(I'd actually prefer if you renamed the observer to SystemTimeChangeObserver, and then did RegisterSystemTimeChangeObserver. That way all the names are consistent.)
>diff --git a/dom/time/TimeChangeObserver.h b/dom/time/TimeChangeObserver.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/TimeChangeObserver.h
>@@ -0,0 +1,30 @@
>+class nsSystemTimeChangeObserver : public SystemTimeChangeObserver
>+{
>+public:
>+ static nsSystemTimeChangeObserver* GetInstance();
>+ void Notify(const mozilla::hal::SystemTimeChange& aReason);
>+ nsresult AddWindowListener(nsIDOMWindow *aWindow);
>+ nsresult RemoveWindowListener(nsIDOMWindow *aWindow);
>+private:
>+ nsSystemTimeChangeObserver() {};
>+ bool FindWindow(nsIDOMWindow* aWindow);
Looks like this function doesn't exist anymore.
>+ nsTArray<nsWeakPtr> mWindowListeners;
>+};
>+
>+#endif //_mozilla_time_change_observer_h_
>diff --git a/dom/time/nsIDOMTimeManager.idl b/dom/time/nsIDOMTimeManager.idl
>--- a/dom/time/nsIDOMTimeManager.idl
>+++ b/dom/time/nsIDOMTimeManager.idl
> [scriptable, builtinclass, uuid(d29beaaa-bd54-4fd5-9f18-e0eedb1dc96d)]
> interface nsIDOMMozTimeManager : nsISupports
> {
>- // jsval could be Date object or UTC seconds
>+ /*
>+ * time could be
Nit: s/could/can/. (This is an extremely fine point, but we use the indicative
"can" instead of the subjunctive "could" because this is not a hypothetical;
|time| definitely can take on either of these two classes of values. We would
use "could" in a hypothetical such as "If the sky were green, |time| could be X
or Y.")
>+ * - "UTC seconds" means "seconds since the epoch, midnight UTC on January 1, 1970".
>+ * - If |time| is a date object, set(time) is equivalent to set(time.getTime()).
Another fine point: If you say "time could be: X or Y", then the sentences
"time could be X" and "time could be Y" must be gramatically correct. In this
case, it's not correct to say "time could be 'UTC seconds' means '...'". (You
could say "'UTC seconds', /which/ means".) Nor would we say "time could be if
|time| is a date object...".
How about the following:
> * Set the system time.
> *
> * The |time| argument can be either a Date object or a number.
> *
> * - If |time| is a number, it's interpreted as seconds since the epoch (midnight UTC on January 1, 1970)
> * - If |time| is a Date object, |set(time)| is equivalent to |set(time.getTime())|.
r=me with these changes! Let me know and I can check this in for you.
Attachment #645795 -
Flags: review?(justin.lebar+bug) → review+
Updated•12 years ago
|
Summary: Notification when system clock is recalibrated and when the timezone changes → Time API: notification and datetime set
Comment 92•12 years ago
|
||
Steven, we need this bug and the bugs it blocks fixed by the end of the month. Do you think you'll have time sometime soon to spin a patch with my review comments addressed? I gave you r+, so you don't need another review; we can check in just as soon as you write the patch.
Comment 93•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #92)
> Steven, we need this bug and the bugs it blocks fixed by the end of the
> month. Do you think you'll have time sometime soon to spin a patch with my
> review comments addressed? I gave you r+, so you don't need another review;
> we can check in just as soon as you write the patch.
I'd like to ping on this bug as well. The Alarm API also needs the timezone-changed observer part to dynamically adjust the alarm-clock time whenever the timezone is changed. No worries. Just a reminder. ;)
Assignee | ||
Comment 94•12 years ago
|
||
Hi Justin,
Please check this patch.
Thanks.
Attachment #645795 -
Attachment is obsolete: true
Attachment #649681 -
Flags: review+
Comment 95•12 years ago
|
||
Sorry to be picky, but one nit on the new patch:
+ /* Set the system time.
+ *
+ * The |time| argument can be either a Date object or a number.
+ *
+ * - If |time| is a number, it's interpreted as seconds since the epoch (midnight UTC on January 1, 1970)
+ * - If |time| is a Date object, |set(time)| is equivalent to |set(time.getTime())|.
+ */
+
[implicit_jscontext] void set(in jsval time);
The comment should be in Javadoc form:
/**
* Comment comment comment
*/
[implicit_jscontext] void foo();
The lines should be wrapped at 80 characters, as in
/**
* - option 1 ......
* more of option 1
*
* - option 2
* more of option 2
*/
And there should not be a blank line between the end of the comment and the function name.
Assignee | ||
Comment 96•12 years ago
|
||
Attachment #649681 -
Attachment is obsolete: true
Attachment #649930 -
Flags: review+
Assignee | ||
Comment 97•12 years ago
|
||
rebase
Attachment #630516 -
Attachment is obsolete: true
Attachment #649932 -
Flags: review+
Comment 98•12 years ago
|
||
Would you like me to check these in? (If so, the canonical way to do it is to set checkin-needed in the keywords or set checkin? on the attachments. But pinging me in the bug works too.)
Assignee | ||
Comment 99•12 years ago
|
||
rebase
Attachment #638939 -
Attachment is obsolete: true
Attachment #649937 -
Flags: superreview+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 100•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a92558a8dec
https://hg.mozilla.org/integration/mozilla-inbound/rev/983f76488e59
https://hg.mozilla.org/integration/mozilla-inbound/rev/5439489dc320
\o/
(In the future, it would be helpful if you'd name your patches "part 1", "part 2", etc. so it's clear what order they go in.)
Updated•12 years ago
|
Keywords: checkin-needed
Comment 101•12 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e001a7b3b817 - debug builds asserted about !sHasShutDown a la https://tbpl.mozilla.org/php/getParsedLog.php?id=14212910&tree=Mozilla-Inbound, debug tests leaked a few hundred nsWeakReferences and a nsTArray_base a la https://tbpl.mozilla.org/php/getParsedLog.php?id=14213093&tree=Mozilla-Inbound
Comment 102•12 years ago
|
||
These patches also caused a regression of +1 in the "Number of constructors" (static initializers) benchmark; is there a good way to avoid that?
Comment 103•12 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #102)
> These patches also caused a regression of +1 in the "Number of constructors"
> (static initializers) benchmark; is there a good way to avoid that?
We should test whether creating a StaticAutoPtr<T> regresses this number. The whole point of that guy is not to regress that benchmark...
Updated•12 years ago
|
Priority: -- → P1
Comment 104•12 years ago
|
||
Philor identified two failures in comment 101.
1) We hit an assertion about !sHasShutDown [1] in the ClearOnShutdown code. I think this means that you're calling ClearOnShutdown() after XPCOM shutdown.
2) leaks [2]. This may be a result of (1), because the ClearOnShutdown call is not working properly.
I would try to figure out (1) first, because we even have a stack in the log:
0 XUL!mozilla::ClearOnShutdown<mozilla::StaticAutoPtr<nsSystemTimeChangeObserver> > [ClearOnShutdown.h : 76 + 0x0]
1 XUL!nsSystemTimeChangeObserver::GetInstance [TimeChangeObserver.cpp : 22 + 0xb]
2 XUL!nsGlobalWindow::DisableTimeChangeNotifications [nsGlobalWindow.cpp : 10634 + 0x4]
3 XUL!nsGlobalWindow::CleanUp [nsGlobalWindow.cpp : 1059 + 0xc]
So it looks like you're calling nsSystemTimeChangeObserver::GetInstance after shutdown. But that's problematic, because the instance is cleared on shutdown, so GetInstance creates a new instance, which isn't what we want.
I can think of two ways to do this:
a) We could make the add/remove listener methods (and maybe other methods) static. The static methods would bail if you called them after shutdown.
b) We could make GetInstance() return null if called after shutdown. But this is kind of a footgun, because you might forget to null-check the result. So maybe (a) is better.
Is that enough for you to start on?
[1] https://tbpl.mozilla.org/php/getParsedLog.php?id=14212910&tree=Mozilla-Inbound
[2] https://tbpl.mozilla.org/php/getParsedLog.php?id=14213093&tree=Mozilla-Inbound
Assignee | ||
Comment 106•12 years ago
|
||
Hi Justin,
I tried to use static functions for AddWindowListener/RemoveWindowListener but I still had
leaking error. I am not sure where the problem is.
Here is the log on try server.
https://tbpl.mozilla.org/?tree=Try&rev=774a3fd1299b
Please help me check it. Thanks.
Comment 107•12 years ago
|
||
Andrew can you help?
Comment 108•12 years ago
|
||
The fact that you're leaking two nsTArray's in every debug test probably indicates that you're leaking some global, static data.
Comment 109•12 years ago
|
||
Oh, yes, the leak is very likely a result of the problem from comment 104. If you fix that problem, the leak will probably go away.
The problem is likely that the following occurs:
1) We shut down.
2) KillClearOnShutdown runs, clearing sObserver and deleting that object.
3) Then we call DisableTimeChangeNotifications, which calls nsSystemTimeChangeObserver::GetInstance().
4) Then we create a new instance of nsSystemTimeChangeObserver. This one leaks, because KillClearOnShutdown has already run.
5) Then we crash because you're calling ClearOnShutdown() after KillClearOnShutdown has already run. But even if we didn't crash, we'd leak.
The solution is not to call GetInstance() in DisableTimeChangeNotifications. If sObserver is null, DisableTimeChangeNotifications should do nothing.
Comment 110•12 years ago
|
||
> 1) We shut down.
I should say, we start shutting down.
Comment 111•12 years ago
|
||
Without looking at the code, Justin's assessment sounds on the money for these sorts of weird leaks of a few types of objects.
Comment 112•12 years ago
|
||
We had another bug just like this, but I can't seem to find it atm.
Assignee | ||
Comment 113•12 years ago
|
||
My modifications are in the test patch, https://hg.mozilla.org/try/rev/c0f10bdaf216. It does
not call GetInstance() in DisableTimeChangeNotifications. Since RemoveWindowListener() is an
static function, it calls nsSystemTimeChangeObserver::RemoveWindowListener(). So that I think
re-create the object does not happen in this patch. :(
Comment 114•12 years ago
|
||
Ah, bug 776132.
Assignee | ||
Comment 115•12 years ago
|
||
Thanks, I will check it first.
Comment 116•12 years ago
|
||
Looking through your patches, it seems most likely that you're leaking one or more instances of SystemTimeChangeObserver. That's the only place where you have a member or global nsTArray instance.
Can you put a printf in the constructor and destructor and check whether this hypothesis is true? You may not even need to push to try -- it looks like opening the browser and then closing it may be enough to trigger this leak.
Assignee | ||
Comment 117•12 years ago
|
||
Hi Justin,
I added the log to constructor and destructor. I found only one constructor log when I started
b2g. When I opened and closed the browser, or other OOP apps like email, there was no
constructor/destructor log shown. Is there any way that can make b2g go the shutdown path? I
tried "restart" from power button and stop b2g from shell. They cannot make b2g go the shutdown
path.
Assignee | ||
Comment 118•12 years ago
|
||
I tried on desktop browser. The destructor is called when the browser is close.
Comment 119•12 years ago
|
||
> Is there any way that can make b2g go the shutdown path?
I don't know if you were testing on device or on desktop, but a debug build is probably necessary for it to shut down cleanly. Perhaps you have a release build?
Comment 120•12 years ago
|
||
On desktop, I can reproduce the leak by doing
$ TEST_PATH=dom/browser-element/mochitest make mochitest-plain
but not by simply opening and closing the browser.
I'll keep digging. I imagine valgrind will point us straight to the leak.
Assignee | ||
Comment 121•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #119)
> I don't know if you were testing on device or on desktop, but a debug build
> is probably necessary for it to shut down cleanly. Perhaps you have a
> release build?
I tried both device and desktop.
On desktop, I found that the destructor is called when I closed the browser.
I just want to verify if the destructor is called so that I tried only release
version.
I will also reproduce it by debug version on my site, too.
Thanks for your suggestions.
Comment 122•12 years ago
|
||
I think the problem is that you have a class-static nsTArray.
That will always leak. I'm writing a patch to fix it right now; I'll post it in few minutes.
Comment 123•12 years ago
|
||
The static nsTArray is a problem, but apparently not the only problem. I'm having trouble figuring out what else is going on, but I'm going to try bisecting the diff.
Comment 124•12 years ago
|
||
Okay, so your other problem is another static nsTArray, but it was hiding as
static ObserverList<SystemTimeChange> sSystemTimeObserver;
Get rid of the static nsTArrays and you'll get rid of the leaks. :)
Comment 125•12 years ago
|
||
To be clear, the first of the two static TArray's was introduced in the patch you pushed to try (comment 113).
Updated•12 years ago
|
Whiteboard: [WebAPI:P1]
Comment 126•12 years ago
|
||
Comment on attachment 649932 [details] [diff] [review]
Implement the observer that will notify apps when system changes - V5.1
Review of attachment 649932 [details] [diff] [review]:
-----------------------------------------------------------------
::: hal/gonk/GonkHal.cpp
@@ +616,5 @@
> +IsSameTimeZone(const nsCString& aTimezoneSpec)
> +{
> + char timezone[32];
> + property_get("persist.sys.timezone", timezone, "");
> + return aTimezoneSpec.EqualsASCII(timezone);
Hi Steven,
Just a reminder for this part. Recently, an hal::GetTimezone() has just been supported, which might be reused here. You can probably include that when rebasing codes next time. ;)
Attachment #649932 -
Flags: feedback+
Assignee | ||
Comment 127•12 years ago
|
||
rebase
Attachment #649937 -
Attachment is obsolete: true
Attachment #659702 -
Flags: superreview+
Assignee | ||
Comment 128•12 years ago
|
||
(In reply to Gene Lian [:gene] from comment #126)
> Hi Steven,
>
> Just a reminder for this part. Recently, an hal::GetTimezone() has just been
> supported, which might be reused here. You can probably include that when
> rebasing codes next time. ;)
Hi Gene,
Thanks for your reminder. I've updated my patch.
Assignee | ||
Comment 129•12 years ago
|
||
Rebase and fix the memory leaking problem of sSystemTimeObserver.
Attachment #649932 -
Attachment is obsolete: true
Attachment #659749 -
Flags: review+
Attachment #659749 -
Flags: feedback?(mounir)
Assignee | ||
Comment 130•12 years ago
|
||
Rebase and fix the memory leak problem of mWindowListeners.
Attachment #649930 -
Attachment is obsolete: true
Attachment #659750 -
Flags: review+
Attachment #659750 -
Flags: feedback?(justin.lebar+bug)
Comment 131•12 years ago
|
||
Comment on attachment 659750 [details] [diff] [review]
TimeManager implementation V7
Did you fix any leaks in this patch? All I see is rebase changes.
Please re-flag for review if there's anything interesting changing here; if it's just rebase changes, I don't need to look at it...
Attachment #659750 -
Flags: feedback?(justin.lebar+bug)
Assignee | ||
Comment 132•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #131)
> Comment on attachment 659750 [details] [diff] [review]
> TimeManager implementation V7
>
> Did you fix any leaks in this patch? All I see is rebase changes.
>
> Please re-flag for review if there's anything interesting changing here; if
> it's just rebase changes, I don't need to look at it...
There are 3 files I modified for fixing the leak problem, nsGlobalWindow.cpp,
TimeChangeObserver.cpp and TimeChangeObserver.h.
I changed
1. nsSystemTimeChangeObserver::AddWindowListener and
nsSystemTimeChangeObserver::RemoveWindowListener from non-static to static.
2. nsSystemTimeChangeObserver::mWindowListeners from static nsTArray<nsWeakPtr>
to static nsTArray<nsWeakPtr>* that I can delete it in destructor.
3. the way to call nsSystemTimeChangeObserver::RemoveWindowListener in
nsGlobalWindow::DisableTimeChangeNotifications. To prevent re-allocate
nsSystemTimeChangeObserver in nsGlobalWindow::DisableTimeChangeNotifications.
The other parts of modifications are just rebase. I want to know if these changes
are acceptable.
Comment 133•12 years ago
|
||
Comment on attachment 659750 [details] [diff] [review]
TimeManager implementation V7
Ah, I see; for some reason, interdiff was missing the mWindowListeners change. I have to do a diff of diffs, so sorry that the context below is ugly. :(
> +void
> +nsSystemTimeChangeObserver::Notify(const SystemTimeChange& aReason)
> +{
>-+ // Copy mWindowListeners for preventing that the array may be modified
>-+ // by others when we iterate it.
>++ //Copy mWindowListeners and iterate over windowListeners instead because
>++ //mWindowListeners may be modified while we loop.
> + nsTArray<nsWeakPtr> windowListeners;
>-+ for (PRUint32 i = 0; i < mWindowListeners.Length(); i++) {
>-+ windowListeners.AppendElement(mWindowListeners.SafeElementAt(i));
>++ for (PRUint32 i = 0; i < mWindowListeners->Length(); i++) {
We changed from PRUint32 to uint32_t while you were writing this patch. :)
Please switch over to the new style. Similarly for the other PR types.
> diff --git a/dom/time/TimeChangeObserver.h b/dom/time/TimeChangeObserver.h
> +class nsSystemTimeChangeObserver : public SystemTimeChangeObserver
> +{
> +public:
> + static nsSystemTimeChangeObserver* GetInstance();
>++ virtual ~nsSystemTimeChangeObserver();
> + void Notify(const mozilla::hal::SystemTimeChange& aReason);
>-+ nsresult AddWindowListener(nsIDOMWindow *aWindow);
>-+ nsresult RemoveWindowListener(nsIDOMWindow *aWindow);
>++ static nsresult AddWindowListener(nsIDOMWindow *aWindow);
>++ static nsresult RemoveWindowListener(nsIDOMWindow *aWindow);
Nit: nsIDOMWindow*, please.
> +private:
>-+ nsSystemTimeChangeObserver() {};
>-+ bool FindWindow(nsIDOMWindow* aWindow);
>-+ nsTArray<nsWeakPtr> mWindowListeners;
>++ nsSystemTimeChangeObserver()
>++ {
>++ mWindowListeners = new nsTArray<nsWeakPtr>;
>++ };
>++ static nsTArray<nsWeakPtr>* mWindowListeners;
Okay, this fixes the leak, but there are a few problems with this approach as
written.
First, the "m" means "member", but "mWindowListeners" is in
fact a static variable. It should be sWindowListeners ("s" for "static").
But more importantly, mWindowListeners really should be a member variable,
because it belongs to the singleton class. Singleton classes should not make
all their member variables static -- although that may work, it has its own set
of problems, including the creation of static initializers for member
variables, and, as you discovered, the requirement that in the singleton
destructor we manually delete all our faux-member variables, which is
error-prone.
Last, making it a raw pointer (as opposed to StaticAutoPtr) makes this variable
prone to memory leaks. If we were to run the nsSystemTimeChangeObserver()
constructor twice, we'd leak the first mWindowListeners tarray.
Instead of making mWindowListeners a static variable, I think a better approach
to would be to leave it as normal member variable and do the following:
* Add private nsresult AddWindowListenerImpl(nsIDOMwindow* aWindow).
* Modify static AddWindowListener() so that it does
GetInstance()->AddWindowListenerImpl(aWindow).
* Add private nsresult RemoveWindowListenerImpl(nsIDOMWindow* aWindow).
* Modify static RemoveWindowListner() so that it does
if (!sObserver) {
return NS_OK;
}
GetInstance()->RemoveWindowListener();
Attachment #659750 -
Flags: feedback-
Comment 134•12 years ago
|
||
Comment on attachment 659749 [details] [diff] [review]
Implement the observer that will notify apps when system changes - V6
> +StaticAutoPtr<ObserverList<SystemTimeChange>> sSystemTimeObserver;
static StaticAutoPtr<...>, please. (Otherwise this variable isn't actually file-static, and gets exported from the object file, can conflict with variables in other object files, etc.)
r=me with that change.
Attachment #659749 -
Flags: feedback?(mounir) → feedback+
Assignee | ||
Comment 135•12 years ago
|
||
Hi Justin,
I made some modifications mentioned in comment 133. Please check it.
Thanks for your suggestions.
Attachment #659750 -
Attachment is obsolete: true
Attachment #660011 -
Flags: review+
Attachment #660011 -
Flags: feedback?(justin.lebar+bug)
Assignee | ||
Comment 136•12 years ago
|
||
Attachment #659749 -
Attachment is obsolete: true
Attachment #660016 -
Flags: review+
Comment 137•12 years ago
|
||
Are these patches actually using JS_ClearDateCaches from bug 285615?
Comment 138•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #137)
> Are these patches actually using JS_ClearDateCaches from bug 285615?
We're firing a notification which someone could then listen to.
Comment 139•12 years ago
|
||
Comment on attachment 660011 [details] [diff] [review]
TimeManager implementation V7
Looks good to me!
Attachment #660011 -
Flags: feedback?(justin.lebar+bug) → feedback+
Assignee | ||
Comment 140•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #139)
> Comment on attachment 660011 [details] [diff] [review]
> TimeManager implementation V7
>
> Looks good to me!
\o/
Justin, Mounir,
Thanks for your help in this bug!
Keywords: checkin-needed
Comment 141•12 years ago
|
||
Have you pushed the latest patches to try (even if just -b d -p linux64)?
Assignee | ||
Comment 142•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #141)
> Have you pushed the latest patches to try (even if just -b d -p linux64)?
Here it is.
https://tbpl.mozilla.org/?tree=Try&rev=c3ea86a47497
Comment 143•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ecd4ed6331e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d059d752c212
https://hg.mozilla.org/integration/mozilla-inbound/rev/9efd6fc80405
Can we get a follow-up filed to call JS_ClearDateCaches? I'd be OK even if hal made that call.
Updated•12 years ago
|
Keywords: checkin-needed
Comment 144•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1ecd4ed6331e
https://hg.mozilla.org/mozilla-central/rev/d059d752c212
https://hg.mozilla.org/mozilla-central/rev/9efd6fc80405
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•12 years ago
|
Assignee | ||
Comment 145•12 years ago
|
||
Regression tscroll Row Major increase 0.17% on XP Mozilla-Inbound-Non-PGO
----------------------------------------------------------------------------
Previous: avg 10769.113 stddev 8.270 of 30 runs up to revision ef085eb72cd8
New : avg 10787.400 stddev 2.656 of 5 runs since revision 9efd6fc80405
Change : +18.287 (0.17% / z=2.211)
Graph : http://mzl.la/Q0r62Z
Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ef085eb72cd8&tochange=9efd6fc80405
Changesets:
* http://hg.mozilla.org/integration/mozilla-inbound/rev/1ecd4ed6331e
: Steven Lee <slee@mozilla.com> - Bug 714358: Time manager interface. f=mounir, sr=mounir
: http://bugzilla.mozilla.org/show_bug.cgi?id=714358
* http://hg.mozilla.org/integration/mozilla-inbound/rev/d059d752c212
: Steven Lee <slee@mozilla.com> - Bug 714358: Time manager implementation. r=jlebar
: http://bugzilla.mozilla.org/show_bug.cgi?id=714358
* http://hg.mozilla.org/integration/mozilla-inbound/rev/9efd6fc80405
: Steven Lee <slee@mozilla.com> - Bug 714358 System time change implementation, r=mounir
: http://bugzilla.mozilla.org/show_bug.cgi?id=714358
Bugs:
* http://bugzilla.mozilla.org/show_bug.cgi?id=714358 - Time API: notification and datetime set
Comment 146•12 years ago
|
||
> Regression tscroll Row Major increase 0.17% on XP Mozilla-Inbound-Non-PGO
I think this is benign because
* It's a tiny regression
* It's on Non-PGO only, which we don't ship.
Updated•12 years ago
|
Depends on: 793558
Comment 147•12 years ago
|
||
Doc: https://developer.mozilla.org/en-US/docs/DOM/TimeManager
Can someone review this page and tell me if there is something obviously wrong or missing in that page? Thanks.
Keywords: dev-doc-needed → dev-doc-complete
Comment 148•12 years ago
|
||
Thanks for asking for feedback here. There were a lot of things I thought could to be improved about that page. In particular,
* I removed the contrived syntax example ("var Time = navigator.mozTime") and replaced it with something that actually uses the API.
* I added an explanation of how setTime(x) interprets its argument if x is a number. This information was in the IDL, so I think it really should have been in the docs.
There's also some confusion here about what the security model is around the API. It is true that setTime() is available only to certified apps. But any app can listen to moztimechanged.
We should also explain exactly how this security restriction is enforced. Does a non-certified see navigator.mozTime as null? Does it get an exception when it tries to access it? Does setTime() throw? These are all pretty important parts of the API, and they're trivial to test out. I waved my hands on this part, but it would be helpful if you could do some testing.
Comment 149•12 years ago
|
||
One other thing when you have method "foo(x)", it's important to say what it does.
The docs said
setTime(time) - The time argument can be either a Date object or a number.
Okay, but what does setTime() actually do?
Comment 150•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #148)
> These are all
> pretty important parts of the API, and they're trivial to test out. I waved
> my hands on this part, but it would be helpful if you could do some testing.
You're right. I shouldn't have asked for feedback for an half-baked doc and without testing. I apologize. It won't happen again.
Thanks for the improvements and advice. I've made a couple of changes accordingly. (there is an error regarding a missing template. It'll be fixed as soon as I acquire back the ability to create templates)
> Does a non-certified see navigator.mozTime as null? Does it get an
> exception when it tries to access it? Does setTime() throw?
I see it throw and I'm surprised by this behavior. I'm taking this to dev-webapi to not further pollute this bug.
You need to log in
before you can comment on or make changes to this bug.
Description
•