Last Comment Bug 238041 - nsDOMEvent::AllocateEvent assigns a PR_Now() into a PRUint32
: nsDOMEvent::AllocateEvent assigns a PR_Now() into a PRUint32
Status: ASSIGNED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 16 votes (vote)
: ---
Assigned To: timeless
:
: Andrew Overholt [:overholt]
Mentors:
: 702015 960459 960988 (view as bug list)
Depends on: 77992 116088
Blocks: 509254 585874 881512
  Show dependency treegraph
 
Reported: 2004-03-19 14:49 PST by timeless
Modified: 2016-09-28 11:30 PDT (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
per comment 1, also strips trailing whitespace from nsGUIEvent.h (2.36 KB, patch)
2004-04-16 07:29 PDT, timeless
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Jesse's patch (9.03 KB, patch)
2008-04-13 02:01 PDT, Jesse Ruderman
no flags Details | Diff | Splinter Review
This file 'FF-no-timeStamp-demo.html' demonstrates this problem. (2.46 KB, text/html)
2013-04-22 17:22 PDT, maratbn
no flags Details
FF-no-timeStamp-demo.html (3.10 KB, text/html)
2013-04-22 18:47 PDT, maratbn
no flags Details

Description timeless 2004-03-19 14:49:59 PST
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 [:bz] (still a bit busy) 2004-03-30 18:22:21 PST
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, jst@mozilla.com) 2004-04-15 18:31:22 PDT
Anyone wanna whip up a patch for this?
Comment 3 timeless 2004-04-16 07:29:48 PDT
Created attachment 146266 [details] [diff] [review]
per comment 1, also strips trailing whitespace from nsGUIEvent.h
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2004-04-16 07:56:21 PDT
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, jst@mozilla.com) 2004-04-16 10:07:25 PDT
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 2004-04-16 10:27:39 PDT
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 2008-04-13 02:00:48 PDT
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 2008-04-13 02:01:15 PDT
Created attachment 315353 [details] [diff] [review]
Jesse's patch
Comment 9 Jesse Ruderman 2008-04-15 11:55:35 PDT
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] 2008-04-15 12:12:47 PDT
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 2008-04-15 13:15:15 PDT
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, jst@mozilla.com) 2008-04-15 13:28:52 PDT
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 2008-09-17 15:56:43 PDT
I won't be able to make those larger changes, and this bug blocks a leak fix (bug 424780) :(
Comment 14 Brian Birtles (:birtles) 2010-07-13 19:52:16 PDT
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] 2010-07-14 04:45:52 PDT
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] 2010-07-14 04:47:35 PDT
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) 2010-07-14 05:04:52 PDT
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) 2010-07-14 06:38:28 PDT
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 [:bz] (still a bit busy) 2010-07-14 08:20:40 PDT
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) 2010-07-14 12:58:04 PDT
(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) 2010-07-18 02:32:54 PDT
(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 22 :Ms2ger (⌚ UTC+1/+2) 2011-11-12 07:32:48 PST
*** Bug 702015 has been marked as a duplicate of this bug. ***
Comment 23 Masatoshi Kimura [:emk] 2011-11-12 07:45:50 PST
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 2013-04-22 15:59:10 PDT
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 2013-04-22 17:22:30 PDT
Created attachment 740562 [details]
This file 'FF-no-timeStamp-demo.html' demonstrates this problem.

Attaching 'FF-no-timeStamp-demo.html' that demonstrates this problem.
Comment 26 maratbn 2013-04-22 18:43:11 PDT
(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 2013-04-22 18:47:30 PDT
Created attachment 740592 [details]
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 28 Matthew Gregan [:kinetik] 2014-04-06 20:02:01 PDT
*** Bug 960988 has been marked as a duplicate of this bug. ***
Comment 29 Matthew Gregan [:kinetik] 2014-04-06 20:02:08 PDT
*** Bug 960459 has been marked as a duplicate of this bug. ***
Comment 30 Artem Skoretskiy 2015-08-05 05:12:40 PDT
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 2015-08-05 05:14:15 PDT
Reproduced on OSX 64-bit (10.9.5 to be precise) -- so it is not x86 or Windows-specific issue
Comment 32 Artem Skoretskiy 2015-08-05 05:29:55 PDT
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 2016-05-25 07:33:40 PDT
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 2016-09-28 11:30:50 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.