238041 - nsDOMEvent::AllocateEvent assigns a PR_Now() into a PRUint32

Status: NEW

(Core :: DOM: Events, defect, P3)

Description

[timeless]

bz for bug 116088 added:
>     mEvent->time = PR_Now();

I have a few bones to pick with this;

first my compiler doesn't like it:
mozilla\content\events\src\nsDOMEvent.cpp(1638) : warning C4244: '=' : 
conversion from 'PRTime' to 'PRUint32', possible loss of data

PRTime is a 64bit creature (sometimes a struct).

Second, dreftool doesn't like it
mozhack@boffo:~/obj-i686-pc-linux-gnu-xlib/dist/bin$ ./run-
mozilla.sh ./dreftool -d ~/mozilla/content/events/src/ -e 
~/mozilla/tools/dreftool/nullcall.lst -f ~/mozilla/tools/dreftool/nullfunc.lst

http://bonsai.mozilla.org/cvsblame.cgi?
file=mozilla/content/events/src/nsDOMEvent.cpp&rev=1.169&mark=1627#1622   Deref-
error: "mEvent"

Summary for /mnt/ibm/mozhack/mozilla/content/events/src/:
===============================
Files:  6
Lines:  9372
Errors: 1

third, the comment in the member variable definition doesn't match the critter 
which is being stuffed into it:
http://bonsai.mozilla.org/cvsblame.cgi?
file=mozilla/widget/public/nsGUIEvent.h&rev=3.107&mark=352-353#331
   /// elapsed time, in milliseconds, from the time the system was started to 
the time the message was created
   PRUint32    time;

Comment 1

[Boris Zbarsky [:bzbarsky]]

The right thing to be storing in there should be the time in milliseconds since
the epoch, not the time since system start.

So I think simply dividing PR_Now() by PR_USEC_PER_MSEC should be what we want
here, no?

Comment 2

[Johnny Stenback (:jst)]

Anyone wanna whip up a patch for this?

Comment 3

[timeless]

Attachment: per comment 1, also strips trailing whitespace from nsGUIEvent.h

Comment 4

[Boris Zbarsky [:bzbarsky]]

http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#823
probably needs changing.

So do a whole bunch of places in widget/src/gtk/nsWidget.cpp and
widget/src/gtk/nsWindow.cpp that assign event.time.

In fact, I bet almost every single line listed in
http://lxr.mozilla.org/seamonkey/search?string=event-%3Etime needs changing....

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 146266 [details] [diff] [review]
per comment 1, also strips trailing whitespace from nsGUIEvent.h

- In nsDOMEvent::AllocateEvent():

+    LL_DIV(now, now, microtomilli);
+    mEvent->time = now;

Why not:

+    LL_DIV(mEvent->time, now, microtomilli);

in stead?

r+sr=jst for the patch so far, but as bz pointed out, there's more to be done
here.

Comment 6

[timeless]

in certain cases LL_DIV will access the first param a lot, which isn't cheap.
i'd rather do local operations and then only access the pointer once.

Comment 7

[Jesse Ruderman]

It should probably use PR_IntervalNow() rather than PR_Now(), since it is mostly used to compute differences, and since it might be compared against non-script-generated events, which use PR_IntervalNow().  And it definitely needs to be milliseconds, not microseconds, since using microseconds breaks the mochitest in bug 424780.

http://lxr.mozilla.org/mozilla/source/dom/public/idl/events/nsIDOMEvent.idl#119
should be milliseconds, any epoch is ok

http://mxr.mozilla.org/mozilla/source/content/events/src/nsDOMEvent.cpp#121
set in microseconds (wrong)

http://mxr.mozilla.org/mozilla/source/widget/src/cocoa/nsChildView.mm#1593
set in Intervals (which happen to be milliseconds on Mac and Windows!?)

Comment 8

[Jesse Ruderman]

Attachment: Jesse's patch

Comment 9

[Jesse Ruderman]

Smaug, jst thought you might know more about whether event timestamps should use PR_IntervalNow (for accurate intervals) or PR_Now (for accurate dates).

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

As per spec I'd say PR_Now.

In practice, what is the difference between PR_IntervalNow and PR_Now in this
case?

Comment 11

[Jesse Ruderman]

With PR_Now(), if the computer's clock is reset while you're typing into a <select>, the wrong item might be selected (nsListControlFrame.cpp).

http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event-timeStamp says we can choose the epoch, so PR_IntervalNow() is also correct per spec, isn't it?

Comment 12

[Johnny Stenback (:jst)]

I think I'd prefer to stay away from PR_IntervalNow() as the interval units you get back from it are platform dependent and have an unknown and in deterministic epoch. nsDOMEvent::GetTimeStamp() already returns a PRUint64, which is what you get back from PR_Now() and also matches our definition of nsIDOMEvent.timeStamp, so I think I'd prefer to use that across the board, even if that means changing other event internals (i.e. widget code etc).

Comment 13

[Jesse Ruderman]

I won't be able to make those larger changes, and this bug blocks a leak fix (bug 424780) :(

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Any chance attachment 146266 [details] [diff] [review] is going to be landed? It would be nice to implement timestamps consistently so we can use them for SMIL (which relies on them to avoid synchronisation slew between animations).

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

SMIL relies on DOMEvent timestamps?
Note, per DOM Events spec it is ok to have 0 as the timestamp.

(But sure, we should handle time stamps consistently.)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

And if SMIL relies on timestamps, I wonder how? Does it add some event listeners?
If so, where? Or do SMIL elements implement PostHandleEvent?

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Sorry, I posted the link in bug 77992 but not here.
Here it is:
http://www.w3.org/TR/SMIL/smil-timing.html#q135

Specifically:
"In order to make the model operate consistently and remove the effects of synchronization slew in a chain of event times, the timestamp value associated with events such as the beginEvent,  endEvent, and repeat events is not (necessarily) the actual time that the event is raised, nor is it the time when a time dependent is actually notified of the event."

When you read the following descriptions of click events and so on it appears that what is expected is that the animation should be considered to start at the time the click was generated rather than when the click event was received. In order to do that we need to preserve the time the event was generated in its timestamp. This time is then preserved in SMIL's own TimeEvents' timestamps (although an offset may or may not be added, hence the somewhat confusing sentence about the timestamp of the propagated TimeEvent not necessarily being the same as the time the original event was generated).

Or at least that's my understanding.

It's all about removing synchronisation slew that would otherwise be introduced by the time taken to propagate events.

SMIL will add event listeners when we come to implement event-timing (bug 485157) which we're hoping to get in for Firefox 4.

However if we choose not to fix this then the result is simply that SMIL animations get a little out of sync which may be acceptable for the next release anyway. However, it could possibly complicate testing a little if we can't rely on the animations being synchronised.

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Come to think of it, my previous message was half wrong.

We need to store timestamps correctly for the events generated by SMIL (TimeEvents). To get everything perfectly synced we should also do it for other events too (e.g. click etc.) but for the examples given in the linked document that's not necessary.

What is necessary though is to have some sensible (and preferably consistent) way of storing timestamps and using 64-bit int would sure make that a lot more straightforward. I'm not overly fussed about whether it's an interval time or epoch time although an epoch time would simplify the SMIL implementation.

Comment 19

[Boris Zbarsky [:bzbarsky]]

If we want to store timestamps (as opposed to wall-clock times) we should be using mozilla::TimeStamp.  That's what it's designed for.  It guarantees things like timestamps being nondecreasing (which PR_Now() does NOT guarantee).

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to comment #17)
> When you read the following descriptions of click events and so on it appears
> that what is expected is that the animation should be considered to start at
> the time the click was generated rather than when the click event was received.

Is that the time that the user clicked or the time that the DOM click event was generated (when the browser received the OS click event)?

(In reply to comment #17)
> It's all about removing synchronisation slew that would otherwise be introduced
> by the time taken to propagate events.

Is this (only) the slew that would be introduced propagating a single event?
Or are relative times of related events also important?
(e.g. should buttonup and click [or keydown and keypress] have the same timestamp if they are produced by the same user action?)

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #20)
> Is that the time that the user clicked or the time that the DOM click event was
> generated (when the browser received the OS click event)?

I don't know enough about our event handling to answer that. SMIL needs to be able to register as a listener of arbitrary events. If it's possible and advantageous to do that in a way that doesn't rely on DOM events being created (i.e. just nsEvents) then I guess we need the timestamp in the nsEvent to be set appropriately.

For events generated outside SMIL, the time itself isn't so important. What's important is simply synchronisation so that two animations keyed off the same click event are perfectly in sync.

For the events generated by SMIL itself, the time itself is important. We need to set a timestamp in the nsEvent that we can later pick up and convert to a time in SMIL's timeline that matches the time the event was generated.

> (In reply to comment #17)
> Is this (only) the slew that would be introduced propagating a single event?
> Or are relative times of related events also important?
> (e.g. should buttonup and click [or keydown and keypress] have the same
> timestamp if they are produced by the same user action?)

From my understanding that is not necessary. The main use case is that if two animations are keyed off the same event they should be in sync.

Comment 23

[Masatoshi Kimura [:emk]]

DOM4 does no longer allow 0 nor milliseconds from system start.
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-event-timestamp
> The timeStamp attribute must return the value it was initialized to. When an event is created the attribute must be initialized to the number of milliseconds that has passed since 00:00:00 UTC on 1 January 1970.

Comment 24

[maratbn]

Having the 'timeStamp' field set to 0 for the 'mouseleave' and 'mouseout' DOM events on Firefox ESR 17.0.5 on 64-bit Gentoo.

The 'timeStamp' field does contain a valid value for the 'mouseup' DOM event.

Comment 25

[maratbn]

Attachment: This file 'FF-no-timeStamp-demo.html' demonstrates this problem.

Attaching 'FF-no-timeStamp-demo.html' that demonstrates this problem.

Comment 26

[maratbn]

(In reply to maratbn from comment #24)
> Having the 'timeStamp' field set to 0 for the 'mouseleave' and 'mouseout'
> DOM events on Firefox ESR 17.0.5 on 64-bit Gentoo.
> 
> The 'timeStamp' field does contain a valid value for the 'mouseup' DOM event.

Actually noticed that the 'timeStamp' for the 'mouseup' event is also invalid, but non-0.

Comment 27

[maratbn]

Attachment: FF-no-timeStamp-demo.html

Attaching 'FF-no-timeStamp-demo.html' that demonstrates this problem, as well as compares the invalid timestamps with the actual current time.

Comment 30

[Artem Skoretskiy]

Is there any progress on this ticket?

Some events still have event.timeStamp = 0 or some small number instead of microseconds since 1970-01-01 00:00:00 UTC.

What is interesting -- events, generated on media element (e.g. onplay, onpause) -- are giving correct numbers while common events (onclick, onmouseover) -- are giving these crazy results.

Comment 31

[Artem Skoretskiy]

Reproduced on OSX 64-bit (10.9.5 to be precise) -- so it is not x86 or Windows-specific issue

Comment 32

[Artem Skoretskiy]

Is it correct that it is simply int32 issue? I.e. we try to push very long number into 32-bit variable and we loose data or break it then?

Example: timeStamp = 1438777470568000 # (new Date()).getTime() * 1000

But maximum value for uint32 is: 4294967295.

P.S. It is very strange to see this bug open for 11.5 years while it is obvious bug and it is in the core of the platform.

Comment 33

[mccainz]

Stumbled upon this bug report after a failing test which relied on event.timeStamp from a blur event (FF 46.0.1, Windows 7 enterprise 64 bit).

Comment 34

[Michael Holroyd]

Bewildering that this major issue has been open since 2004! Is the standard workaround something like

if(firefox){ e.timeStamp = Date.now(); }

at the top of every event handler?

Comment 35

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I don't think this is valid anymore with dom.event.highrestimestamp.enabled - what do you think Brian?

Also: Do we want to even support turning dom.event.highrestimestamp.enabled off anymore?

Comment 36

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Tom Ritter [:tjr] from comment #35)
> I don't think this is valid anymore with dom.event.highrestimestamp.enabled
> - what do you think Brian?
> 
> Also: Do we want to even support turning dom.event.highrestimestamp.enabled
> off anymore?

Right, we want to drop that pref and that member altogether. See bug 1026809.

Ben Tian was going to work on bug 1026806 to unblock that, but he no longer appears to be working on it.

It shouldn't be too hard to fix those two bugs -- there are (very old) WIP patches for both. The hardest part is following up on any regressions that come from switching any remaining internal uses of mTime to mTimeStamp (bug 1026806).

Comment 37

[Curtis - www.HawaiianHope.org]

Aloha !
We are implementing jQuery into our new products. I was really excited to learn about :

• jQuery event.timeStamp
  But, this apparently does not work in FireFox as this bug was reported 15 years ago.

https://api.jquery.com/event.timeStamp/
and
https://www.w3schools.com/jquery/tryit.asp?filename=tryjquery_event_timestamp
(These work in Internet Explorer and on an android)

In another language (Mumps - M), I have used this type of date serial number extensively (called $H there) and it is a critical component of the language. It would be nice to see this (event.timeStamp) working properly with FireFox

http://mumps.sourceforge.net/docs.html#VHOROLOG

THANK YOU !

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

This bug is not about the current Event.timeStamp.
Event.timeStamp returns these days high resolution time stamp.
See comment 35.

Please file a new bug with some test case attached to it, thanks.

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.