aTimeAsTimeStamp might be a null timestamp after calling IsTimeNewerThanTimestamp
Categories
(Core :: Widget, defect, P3)
Tracking
()
People
(Reporter: valentin, Unassigned)
References
Details
Attachments
(1 obsolete file)
STR:
create a test.txt file containing:
<html>
<body>
<h1>Test</h1>
<script>
alert('JavaScript Executed');
</script>
</body>
</html>
Drag and drop this file into a debug Firefox (tested on Ubuntu linux)
Result: Assertion failure: !aOther.IsNull() (Cannot compute with aOther null value), at /obj-x86_64-pc-linux-gnu/dist/include/mozilla/TimeStamp.h:495
#8 mozilla::TimeStamp::operator- (this=0x7ffd4a1048a8, aOther=...)
at /obj-x86_64-pc-linux-gnu/dist/include/mozilla/TimeStamp.h:495
#9 mozilla::SystemTimeConverter<unsigned int, mozilla::TimeStamp>::GetTimeStampFromSystemTime<mozilla::CurrentX11TimeGetter> (this=0x717223a571b0 <TimeConverter()::sTimeConverterSingleton>,
aTime=4082416864, aCurrentTimeGetter=...)
at /widget/SystemTimeConverter.h:108
#10 0x000071721eb6486d in nsWindow::GetEventTimeStamp (this=this@entry=0x5cabd64bb730,
aEventTime=0, aEventTime@entry=4082416864)
at /widget/gtk/nsWindow.cpp:4946
#11 0x000071721eb66917 in nsWindow::GetWidgetEventTime (this=0x5cabd64bb730, aEventTime=4082416864)
at /widget/gtk/nsWindow.cpp:4914
#12 nsWindow::DispatchDragEvent (this=0x5cabd64bb730, aMsg=<optimized out>, aRefPoint=...,
aTime=4082416864) at /widget/gtk/nsWindow.cpp:5395
#13 0x000071721ebe2acc in nsDragSession::DispatchDropEvent (this=0x5cabda871ad0)
at /widget/gtk/nsDragService.cpp:2895
#14 nsDragSession::RunScheduledTask (this=this@entry=0x5cabda871ad0)
at /widget/gtk/nsDragService.cpp:2762
#15 0x000071721ebe233c in nsDragSession::TaskDispatchCallback (data=0x5cabda871ad0)
at /widget/gtk/nsDragService.cpp:2650
I looked into what's causing it and found this code
if (aTimeAsTimeStamp) {
*aTimeAsTimeStamp =
mReferenceTimeStamp + TimeDuration::FromMilliseconds(wholeMillis);
}
It seems that aTimeAsTimeStamp could be set to null even when mReferenceTimeStamp isn't null because of this:
https://searchfox.org/mozilla-central/rev/9fe73403523732f57cd82d30590ecc272fb0b165/mozglue/misc/TimeStamp.h#524-529
// Check for underflow.
// (We don't check for overflow because it's not obvious what the error
// behavior should be in that case.)
if (aOther.mValue < 0 && value > mValue) {
value = 0;
}
Maybe some small fix like this is necessary:
if (aTimeAsTimeStamp->IsNull()) {
*aTimeAsTimeStamp = mReferenceTimeStamp;
}
|
||
Comment 2•7 months ago
|
||
I was originally trying to debug the crash of Bug 1863047 in debug mode.
Investigating the MOZ_ASSERT with rr and pernosco lead me to believe
that something is wrong with this condition.
I still can't wrap my head around this code entirely. Therefore I didn't
update the test cases yet.
|
||
Updated•7 months ago
|
|
||
Comment 3•7 months ago
|
||
Thank you for the analysis. That confirms my suspicions in https://phabricator.services.mozilla.com/D207842#7137803
There is also a pernosco recording with analysis in bug 1874084 comment 4.
Apparently a foreign application is sending a bad Time on an event and our TimeStamp code detects this as bad because it indicates a time from before the system started.
In this case I assume the foreign app is a file manager or window manager. What application is involved?
(In reply to Valentin Gosu [:valentin] (he/him) from comment #0)
Maybe some small fix like this is necessary:
if (aTimeAsTimeStamp->IsNull()) { *aTimeAsTimeStamp = mReferenceTimeStamp; }
If trying to guess what timestamp might correspond to the Time that should have been on the event, then I guess the mReferenceTimeStamp is strictly less than now and newer than some event in the past. I don't think the API requires that times are always increasing, but perhaps that may be expected from the current callers. Still mReferenceTimeStamp could be an arbitrarily bad estimate.
I think it's best to keep a null TimeStamp to indicate that we know we don't have a good TimeStamp for this event and the client can then decide on the appropriate course of action.
The code could be adjusted to avoid the assertion failures, generate a null TimeStamp via other means, and log when this happens.
Currently, if falling through the asserts, IsTimeNewerThanTimestamp() returns false and roughlyNow - timeAsTimeStamp would be > kTolerance. That's all fine.
aTime - mLastBackwardsSkewCheck > kBackwardsSkewCheckInterval would be true and so set mLastBackwardsSkewCheck to the bad aTime. That should remedy quickly with subsequent events.
|
Reporter | |
Comment 4•7 months ago
|
||
In this case I assume the foreign app is a file manager or window manager. What application is involved?
It's the ubuntu file manager, Nautilus.
if (roughlyNow - timeAsTimeStamp <= kTolerance) {
As long as we add some code to avoid the debug assertion here, I'll be very happy. Thanks! 🙂
|
||
Comment 5•6 months ago
|
||
The severity field is not set for this bug.
:spohl, could you have a look please?
For more information, please visit BugBot documentation.
|
||
Updated•6 months ago
|
Description
•