905405 - Data race on nsAppShell::mLastNativeEventScheduled

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Many threads write this value, here's the main thread:

  xul.dll!ScheduleNativeEventCallback - nsappshell.cpp:167
  xul.dll!ProcessNextNativeEvent - nsappshell.cpp:243
  xul.dll!DoProcessNextNativeEvent - nsbaseappshell.cpp:139
  xul.dll!OnProcessNextEvent - nsbaseappshell.cpp:286
  xul.dll!ProcessNextEvent - nsthread.cpp:595
  xul.dll!NS_ProcessNextEvent - nsthreadutils.cpp:238
  xul.dll!Run - messagepump.cpp:81

And here's the timer thread (though any thread that calls Dispatch on the main thread will end up here I think):

  xul.dll!ScheduleNativeEventCallback - nsappshell.cpp:167
  xul.dll!OnDispatchedEvent - nsbaseappshell.cpp:235
  xul.dll!PutEvent - nsthread.cpp:362
  xul.dll!Dispatch - nsthread.cpp:404
  xul.dll!PostTimerEvent - nstimerimpl.cpp:670
  xul.dll!Run - timerthread.cpp:232

TimeStamp is > 128 bits on windows so there's no way this assignment is atomic. If we read at a bad time here http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsAppShell.cpp#241 then I'm not sure if we're going to mess up our reference count somehow.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

I don't think this matters all that much. The check in ProcessNextNativeEvent is really only a heuristic.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4ddfbf313fc

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out due to build failure.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ebacd4d0b4a

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f52d046c2e7

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f52d046c2e7