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

ASSIGNED
Assigned to

Status

()

Core
DOM: Events
ASSIGNED
13 years ago
7 months ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

(Blocks: 2 bugs)

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

13 years ago
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;
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?
Anyone wanna whip up a patch for this?
(Assignee)

Comment 3

13 years ago
Created attachment 146266 [details] [diff] [review]
per comment 1, also strips trailing whitespace from nsGUIEvent.h
(Assignee)

Updated

13 years ago
Attachment #146266 - Flags: superreview?(jst)
Attachment #146266 - Flags: review?(jst)
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 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.
Attachment #146266 - Flags: superreview?(jst)
Attachment #146266 - Flags: superreview+
Attachment #146266 - Flags: review?(jst)
Attachment #146266 - Flags: review+
(Assignee)

Comment 6

13 years ago
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.
Status: NEW → ASSIGNED

Updated

9 years ago
Blocks: 424780

Comment 7

9 years ago
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

9 years ago
Created attachment 315353 [details] [diff] [review]
Jesse's patch

Comment 9

9 years ago
Smaug, jst thought you might know more about whether event timestamps should use PR_IntervalNow (for accurate intervals) or PR_Now (for accurate dates).
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

9 years ago
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?
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).

Updated

9 years ago
Depends on: 77992
QA Contact: ian → events

Comment 13

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

Updated

8 years ago
Blocks: 509254

Updated

8 years ago
No longer blocks: 424780
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).
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.)
And if SMIL relies on timestamps, I wonder how? Does it add some event listeners?
If so, where? Or do SMIL elements implement PostHandleEvent?
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.
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.
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).
(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?)
(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.
Blocks: 485157
Blocks: 585874
No longer blocks: 485157

Updated

6 years ago
Duplicate of this bug: 702015
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

4 years ago
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

4 years ago
Created attachment 740562 [details]
This file 'FF-no-timeStamp-demo.html' demonstrates this problem.

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

Updated

4 years ago
Attachment #740562 - Attachment mime type: text/plain → text/html

Comment 26

4 years ago
(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

4 years ago
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.
Attachment #740562 - Attachment is obsolete: true

Updated

4 years ago
Attachment #740592 - Attachment mime type: text/plain → text/html
Blocks: 778617
Duplicate of this bug: 960988
Duplicate of this bug: 960459
Blocks: 881512
No longer blocks: 778617

Comment 30

2 years ago
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

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

Comment 32

2 years ago
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

11 months ago
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

7 months ago
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?
You need to log in before you can comment on or make changes to this bug.